sysfs: fix namespace refcnt leak

PATCH Superseded Li Zefan 2 participants 4 messages Feb 17, 2014 – Feb 20, 2014

Subsystems (3)

Driver core cgroup kernfs

lore

Share & tools

lore

Li Zefan

author

2014-02-17 03:54 UTC #0 lore raw

PATCH 1/1 sysfs: fix namespace refcnt leak

Hi Tejun,

I think we have to fix kernfs in order to fix refcnt leak in sysfs and cgroupfs. This fix is for 3.14, but it creates conflicts for cgroup-next.

====================

As mount() and kill_sb() is not a one-to-one match, we shoudn't get ns refcnt unconditionally in sysfs_mount(), and instead we should get the refcnt only when kernfs_mount() allocated a new superblock.

Signed-off-by Li Zefan <[email protected]>

---
 fs/kernfs/mount.c      | 7 ++++++-
 fs/sysfs/mount.c       | 5 +++--
 include/linux/kernfs.h | 9 +++++----
 3 files changed, 14 insertions(+), 7 deletions(-)

fs/kernfs/mount.c +6 -1

index 405279b..53ce420 100644
@@ -132,6 +132,7 @@ const void *kernfs_super_ns(struct super_block *sb)
  * @flags: mount flags specified for the mount
  * @root: kernfs_root of the hierarchy being mounted
  * @ns: optional namespace tag of the mount
+ * @new: tell the caller if we allocated a new superblock
  *
  * This is to be called from each kernfs user's file_system_type->mount()
  * implementation, which should pass through the specified @fs_type and

@@ -141,12 +142,15 @@ const void *kernfs_super_ns(struct super_block *sb)
  * The return value can be passed to the vfs layer verbatim.
  */
 struct dentry *kernfs_mount_ns(struct file_system_type *fs_type, int flags,
-			       struct kernfs_root *root, const void *ns)
+			       struct kernfs_root *root, const void *ns,
+			       bool *new_sb)
 {
 	struct super_block *sb;
 	struct kernfs_super_info *info;
 	int error;
 
+	*new_sb = false;
+
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return ERR_PTR(-ENOMEM);

@@ -166,6 +170,7 @@ struct dentry *kernfs_mount_ns(struct file_system_type *fs_type, int flags,
 			return ERR_PTR(error);
 		}
 		sb->s_flags |= MS_ACTIVE;
+		*new_sb = true;
 	}
 
 	return dget(sb->s_root);

fs/sysfs/mount.c +3 -2

index 5c7fdd9..f5bea79 100644
@@ -27,6 +27,7 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
 {
 	struct dentry *root;
 	void *ns;
+	bool new_sb;
 
 	if (!(flags & MS_KERNMOUNT)) {
 		if (!capable(CAP_SYS_ADMIN) && !fs_fully_visible(fs_type))

@@ -37,8 +38,8 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
 	}
 
 	ns = kobj_ns_grab_current(KOBJ_NS_TYPE_NET);
-	root = kernfs_mount_ns(fs_type, flags, sysfs_root, ns);
-	if (IS_ERR(root))
+	root = kernfs_mount_ns(fs_type, flags, sysfs_root, ns, &new_sb);
+	if (IS_ERR(root) || !new_sb)
 		kobj_ns_drop(KOBJ_NS_TYPE_NET, ns);
 	return root;
 }

include/linux/kernfs.h +5 -4

index 649497a..1b1849f 100644
@@ -279,7 +279,8 @@ void kernfs_notify(struct kernfs_node *kn);
 
 const void *kernfs_super_ns(struct super_block *sb);
 struct dentry *kernfs_mount_ns(struct file_system_type *fs_type, int flags,
-			       struct kernfs_root *root, const void *ns);
+			       struct kernfs_root *root, const void *ns,
+			       bool *new_sb);
 void kernfs_kill_sb(struct super_block *sb);
 
 void kernfs_init(void);

@@ -372,7 +373,7 @@ static inline const void *kernfs_super_ns(struct super_block *sb)
 
 static inline struct dentry *
 kernfs_mount_ns(struct file_system_type *fs_type, int flags,
-		struct kernfs_root *root, const void *ns)
+		struct kernfs_root *root, const void *ns, bool *new_sb)
 { return ERR_PTR(-ENOSYS); }
 
 static inline void kernfs_kill_sb(struct super_block *sb) { }

@@ -430,9 +431,9 @@ static inline int kernfs_rename(struct kernfs_node *kn,
 
 static inline struct dentry *
 kernfs_mount(struct file_system_type *fs_type, int flags,
-	     struct kernfs_root *root)
+	     struct kernfs_root *root, bool *new_sb)
 {
-	return kernfs_mount_ns(fs_type, flags, root, NULL);
+	return kernfs_mount_ns(fs_type, flags, root, NULL, new_sb);
 }
 
 #endif	/* __LINUX_KERNFS_H */

Signature · 1 line

1.8.0.2

Tejun Heo

maintainer

2014-02-18 23:12 UTC #1 lore raw

Hey,

Quoted 9 lines · Li Zefan

I think we have to fix kernfs in order to fix refcnt leak in sysfs and cgroupfs. This fix is for 3.14, but it creates conflicts for cgroup-next.
====================
As mount() and kill_sb() is not a one-to-one match, we shoudn't get ns refcnt unconditionally in sysfs_mount(), and instead we should get the refcnt only when kernfs_mount() allocated a new superblock.

Ugh... nasty :(

fs/kernfs/mount.c const void *kernfs_super_ns(struct super_block *sb) · line 135

Quoted 5 lines

fs/kernfs/mount.c +1 -0

@@ -132,6 +132,7 @@ const void *kernfs_super_ns(struct super_block *sb)
  * @flags: mount flags specified for the mount
  * @root: kernfs_root of the hierarchy being mounted
  * @ns: optional namespace tag of the mount
+ * @new: tell the caller if we allocated a new superblock

Maybe something like @new_sb_created is better?

fs/kernfs/mount.c const void *kernfs_super_ns(struct super_block *sb) · line 152

Quoted 5 lines

fs/kernfs/mount.c +1 -0

 	struct super_block *sb;
 	struct kernfs_super_info *info;
 	int error;
 
+	*new_sb = false;

Can we make it optional so that users who don't care about it can ignore it?

include/linux/kernfs.h static inline int kernfs_rename(struct kernfs_node *kn, · line 436

Quoted 9 lines

include/linux/kernfs.h +2 -2

@@ -430,9 +431,9 @@ static inline int kernfs_rename(struct kernfs_node *kn,
 
 static inline struct dentry *
 kernfs_mount(struct file_system_type *fs_type, int flags,
-	     struct kernfs_root *root)
+	     struct kernfs_root *root, bool *new_sb)
 {
-	return kernfs_mount_ns(fs_type, flags, root, NULL);
+	return kernfs_mount_ns(fs_type, flags, root, NULL, new_sb);

And let kernfs_mount() just use NULL for the parameter?

Thanks.

Signature · 1 line

tejun

Li Zefan

author

2014-02-19 01:39 UTC #2 lore raw

Quoted 23 lines · Tejun Heo

Hey,
Quoted 9 lines · Li Zefan
I think we have to fix kernfs in order to fix refcnt leak in sysfs and cgroupfs. This fix is for 3.14, but it creates conflicts for cgroup-next.
====================
As mount() and kill_sb() is not a one-to-one match, we shoudn't get ns refcnt unconditionally in sysfs_mount(), and instead we should get the refcnt only when kernfs_mount() allocated a new superblock.
Ugh... nasty :(
fs/kernfs/mount.c const void *kernfs_super_ns(struct super_block *sb) · line 135
Quoted 5 lines
fs/kernfs/mount.c +1 -0
@@ -132,6 +132,7 @@ const void *kernfs_super_ns(struct super_block *sb)
  * @flags: mount flags specified for the mount
  * @root: kernfs_root of the hierarchy being mounted
  * @ns: optional namespace tag of the mount
+ * @new: tell the caller if we allocated a new superblock
Maybe something like @new_sb_created is better?

Quoted 9 lines

fs/kernfs/mount.c const void *kernfs_super_ns(struct super_block *sb) · line 152
Quoted 5 lines
fs/kernfs/mount.c +1 -0
 	struct super_block *sb;
 	struct kernfs_super_info *info;
 	int error;
 
+	*new_sb = false;
Can we make it optional so that users who don't care about it can ignore it?

cgroupfs also needs this to fix refcnt leak.

Because success in finding an existing cgroup_root doesn't mean no new superblock is needed. For example:

  # mount -t cgroup -o cpuacct xxx /cgroup
  # mkdir /cgroup/tmp
  # umount /cgroup    <--- sb will be freed but cgroup_root won't

// this will allocate new sb, but we find the cgroup_root is there.

  # mount -t cgroup -o cpuacct xxx /cgroup

But debugfs won't need this if it's converted to kernfs.

How about I keep kernfs_mount() API intact, and when this fix gets merged into mainline, you merge the fix into cgroup-next, and then I make a fix for cgroup by changing kernfs_mount()?

Quoted 12 lines

include/linux/kernfs.h static inline int kernfs_rename(struct kernfs_node *kn, · line 436

Quoted 9 lines

include/linux/kernfs.h +2 -2

@@ -430,9 +431,9 @@ static inline int kernfs_rename(struct kernfs_node *kn,
 
 static inline struct dentry *
 kernfs_mount(struct file_system_type *fs_type, int flags,
-	     struct kernfs_root *root)
+	     struct kernfs_root *root, bool *new_sb)
 {
-	return kernfs_mount_ns(fs_type, flags, root, NULL);
+	return kernfs_mount_ns(fs_type, flags, root, NULL, new_sb);

And let kernfs_mount() just use NULL for the parameter?

Tejun Heo

maintainer

2014-02-20 14:30 UTC #3 lore raw

Hello, Li.

Quoted 4 lines · Li Zefan

Quoted 2 lines
Can we make it optional so that users who don't care about it can ignore it?
cgroupfs also needs this to fix refcnt leak.

Ah, okay.

Quoted 7 lines

Because success in finding an existing cgroup_root doesn't mean no new superblock is needed. For example:
  # mount -t cgroup -o cpuacct xxx /cgroup
  # mkdir /cgroup/tmp
  # umount /cgroup    <--- sb will be freed but cgroup_root won't

Quoted 8 lines

// this will allocate new sb, but we find the cgroup_root is there.
  # mount -t cgroup -o cpuacct xxx /cgroup
But debugfs won't need this if it's converted to kernfs.
How about I keep kernfs_mount() API intact, and when this fix gets merged into mainline, you merge the fix into cgroup-next, and then I make a fix for cgroup by changing kernfs_mount()?

If we need kernfs_mount() modified anyway, let's do it in this patch. I still think it'd be better to allow the parameter to be NULL but other than that, no objection.

Thanks!

Signature · 1 line

tejun