Skip to content

Commit 1e7107c

Browse files
Paul Gortmakerhtejun
authored andcommitted
cgroup1: fix leaked context root causing sporadic NULL deref in LTP
Richard reported sporadic (roughly one in 10 or so) null dereferences and other strange behaviour for a set of automated LTP tests. Things like: BUG: kernel NULL pointer dereference, address: 0000000000000008 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 0 P4D 0 Oops: 0000 [Rust-for-Linux#1] PREEMPT SMP PTI CPU: 0 PID: 1516 Comm: umount Not tainted 5.10.0-yocto-standard Rust-for-Linux#1 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014 RIP: 0010:kernfs_sop_show_path+0x1b/0x60 ...or these others: RIP: 0010:do_mkdirat+0x6a/0xf0 RIP: 0010:d_alloc_parallel+0x98/0x510 RIP: 0010:do_readlinkat+0x86/0x120 There were other less common instances of some kind of a general scribble but the common theme was mount and cgroup and a dubious dentry triggering the NULL dereference. I was only able to reproduce it under qemu by replicating Richard's setup as closely as possible - I never did get it to happen on bare metal, even while keeping everything else the same. In commit 71d883c ("cgroup_do_mount(): massage calling conventions") we see this as a part of the overall change: -------------- struct cgroup_subsys *ss; - struct dentry *dentry; [...] - dentry = cgroup_do_mount(&cgroup_fs_type, fc->sb_flags, root, - CGROUP_SUPER_MAGIC, ns); [...] - if (percpu_ref_is_dying(&root->cgrp.self.refcnt)) { - struct super_block *sb = dentry->d_sb; - dput(dentry); + ret = cgroup_do_mount(fc, CGROUP_SUPER_MAGIC, ns); + if (!ret && percpu_ref_is_dying(&root->cgrp.self.refcnt)) { + struct super_block *sb = fc->root->d_sb; + dput(fc->root); deactivate_locked_super(sb); msleep(10); return restart_syscall(); } -------------- In changing from the local "*dentry" variable to using fc->root, we now export/leave that dentry pointer in the file context after doing the dput() in the unlikely "is_dying" case. With LTP doing a crazy amount of back to back mount/unmount [testcases/bin/cgroup_regression_5_1.sh] the unlikely becomes slightly likely and then bad things happen. A fix would be to not leave the stale reference in fc->root as follows: --------------                 dput(fc->root); + fc->root = NULL;                 deactivate_locked_super(sb); -------------- ...but then we are just open-coding a duplicate of fc_drop_locked() so we simply use that instead. Cc: Al Viro <[email protected]> Cc: Tejun Heo <[email protected]> Cc: Zefan Li <[email protected]> Cc: Johannes Weiner <[email protected]> Cc: [email protected] # v5.1+ Reported-by: Richard Purdie <[email protected]> Fixes: 71d883c ("cgroup_do_mount(): massage calling conventions") Signed-off-by: Paul Gortmaker <[email protected]> Signed-off-by: Tejun Heo <[email protected]>
1 parent 8cae8cd commit 1e7107c

File tree

3 files changed

+2
-4
lines changed

3 files changed

+2
-4
lines changed

fs/internal.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@ extern void __init chrdev_init(void);
6161
*/
6262
extern const struct fs_context_operations legacy_fs_context_ops;
6363
extern int parse_monolithic_mount_data(struct fs_context *, void *);
64-
extern void fc_drop_locked(struct fs_context *);
6564
extern void vfs_clean_context(struct fs_context *fc);
6665
extern int finish_clean_context(struct fs_context *fc);
6766

include/linux/fs_context.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ extern int vfs_get_tree(struct fs_context *fc);
141141
extern void put_fs_context(struct fs_context *fc);
142142
extern int vfs_parse_fs_param_source(struct fs_context *fc,
143143
struct fs_parameter *param);
144+
extern void fc_drop_locked(struct fs_context *fc);
144145

145146
/*
146147
* sget() wrappers to be called from the ->get_tree() op.

kernel/cgroup/cgroup-v1.c

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1221,9 +1221,7 @@ int cgroup1_get_tree(struct fs_context *fc)
12211221
ret = cgroup_do_get_tree(fc);
12221222

12231223
if (!ret && percpu_ref_is_dying(&ctx->root->cgrp.self.refcnt)) {
1224-
struct super_block *sb = fc->root->d_sb;
1225-
dput(fc->root);
1226-
deactivate_locked_super(sb);
1224+
fc_drop_locked(fc);
12271225
ret = 1;
12281226
}
12291227

0 commit comments

Comments
 (0)