Skip to contentSkip to conversation

procfs: Fix refcnt leak on proc_self_follow_link() error path

PATCH In review OGAWA Hirofumi 3 participants 5 messages Jan 11, 2010 – Feb 4, 2010

Subsystems (2)
VFS Proc filesystem
lore
Share & tools
lore

OGAWA Hirofumi

author
#0 lore raw

PATCH 1/1 procfs: Fix refcnt leak on proc_self_follow_link() error path

If ->follow_link handler return the error, it should decrement nd->path refcnt.

This patch fix it.

Signed-off-by OGAWA Hirofumi <[email protected]>

---

 fs/proc/base.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/proc/base.c renamed from fs/proc/base.c~namei-procfs-follow_link-fix +3 -1
diff -puN fs/proc/base.c~namei-procfs-follow_link-fix fs/proc/base.c
@@ -2371,8 +2371,10 @@ static void *proc_self_follow_link(struc
 	struct pid_namespace *ns = dentry->d_sb->s_fs_info;
 	pid_t tgid = task_tgid_nr_ns(current, ns);
 	char tmp[PROC_NUMBUF];
-	if (!tgid)
+	if (!tgid) {
+		path_put(&nd->path);
 		return ERR_PTR(-ENOENT);
+	}
 	sprintf(tmp, "%d", task_tgid_nr_ns(current, ns));
 	return ERR_PTR(vfs_follow_link(nd,tmp));
 }

_

Signature · 1 line
OGAWA Hirofumi <[email protected]>
maintainer
#1 lore raw
Quoted 5 lines · OGAWA Hirofumi

If ->follow_link handler return the error, it should decrement nd->path refcnt.

This patch fix it.

It's OK for -stable, but for the next tree... not really. I'd rather kill vfs_follow_link() uses here and in gfs2; see #untested in vfs-2.6.git for details.

#2 lore raw
Quoted 10 lines · On Wed, 13 Jan 2010 19:43:01 +0000 Al Viro <[email protected]> wrote:
Quoted 5 lines · OGAWA Hirofumi

If ->follow_link handler return the error, it should decrement nd->path refcnt.

This patch fix it.

It's OK for -stable, but for the next tree... not really. I'd rather kill vfs_follow_link() uses here and in gfs2; see #untested in vfs-2.6.git for details.

Confused. Is #untested planned for 2.6.33? If not, how do we fix this bug in .33?

maintainer
#3 lore raw
Quoted 8 lines · Andrew Morton
Quoted 5 lines · On Wed, 13 Jan 2010 19:43:01 +0000 Al Viro <[email protected]> wrote:
Quoted 4 lines · OGAWA Hirofumi

If ->follow_link handler return the error, it should decrement nd->path refcnt.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

This patch fix it.

It's OK for -stable, but for the next tree... not really. I'd rather kill vfs_follow_link() uses here and in gfs2; see #untested in vfs-2.6.git for details.

Confused. Is #untested planned for 2.6.33? If not, how do we fix this bug in .33?

My preference would be a backport of corresponding bits - they _are_ safe. I can live with procfs/gfs2 stuff getting into the tree as-is to be converted later (and note that at least procfs one is fairly old - it goes back to commit 488e5bc4560d0b510c1ddc451c51a6cc14e3a930

Author: Eric W. Biederman <[email protected]>
Date:   Fri Feb 8 04:18:34 2008 -0800

proc: proper pidns handling for /proc/self
so it's prime -stable fodder, whatever form of fix we prefer for that).

For post-2.6.33 we definitely have good reasons to fix that stuff by providing ->put_link() and switching to nd_set_link() for those. It allows to kill fsckloads of symlink body copying when doing open() on pathname that resolves to a symlink, which is worth a lot.

I'm still not 100% sure which way to go for -stable (and .33 - these are equivalent in that respect). Posted patches touch only the codepaths where we are currently guaranteed to leak and all things equal that'd be an argument in their favour. OTOH, the minimal alternative for e.g. gfs2 would be to make buffer allocation unconditional, use nd_set_link() instead of vfs_follow_link() and leave freeing to put_link(). Which can be followed by killing special gfs2 ->readlink(), since now generic_readlink() will work just fine (and gfs2_readlinki() can be folded into gfs2_follow_link(), simplifying things even more). In other words, long-term variant is not much more intrusive and it's just as safe. So that argument in favour of posted variant is not particulary strong, especially wrt 2.6.33.

So basically it boils down to doing truly minimal fix vs. doing the same thing (almost as trivial) we'll do past .33.

FWIW, it may be as simple as "is posted gfs2 patch already in a published gfs2 tree and if it is, how inconvenient for gfs2 folks would it be to replace it?"; I'm really ambivalent about that one...

#4 lore raw
Quoted 24 lines · On Thu, 14 Jan 2010 01:55:57 +0000 Al Viro <[email protected]> wrote:
Quoted 8 lines · Andrew Morton
Quoted 5 lines · On Wed, 13 Jan 2010 19:43:01 +0000 Al Viro <[email protected]> wrote:
Quoted 4 lines · OGAWA Hirofumi

If ->follow_link handler return the error, it should decrement nd->path refcnt.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

This patch fix it.

It's OK for -stable, but for the next tree... not really. I'd rather kill vfs_follow_link() uses here and in gfs2; see #untested in vfs-2.6.git for details.

Confused. Is #untested planned for 2.6.33? If not, how do we fix this bug in .33?

My preference would be a backport of corresponding bits - they _are_ safe. I can live with procfs/gfs2 stuff getting into the tree as-is to be converted later (and note that at least procfs one is fairly old - it goes back to commit 488e5bc4560d0b510c1ddc451c51a6cc14e3a930

Author: Eric W. Biederman <[email protected]>
Date:   Fri Feb 8 04:18:34 2008 -0800
Quoted 7 lines

proc: proper pidns handling for /proc/self
so it's prime -stable fodder, whatever form of fix we prefer for that).

For post-2.6.33 we definitely have good reasons to fix that stuff by providing ->put_link() and switching to nd_set_link() for those. It allows to kill fsckloads of symlink body copying when doing open() on pathname that resolves to a symlink, which is worth a lot.

I see this in linux-next:

commit 1427acc5655198f0196178846599a62656e92df0

Author:     Al Viro <[email protected]>
AuthorDate: Thu Jan 14 01:03:28 2010 -0500
Commit:     Al Viro <[email protected]>
CommitDate: Sat Jan 30 00:03:55 2010 -0500

Switch proc/self to nd_set_link()

Signed-off-by Al Viro <[email protected]>
fs/proc/base.c +19 -5
index e42bbd8..58324c2 100644
@@ -2369,16 +2369,30 @@ static void *proc_self_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	struct pid_namespace *ns = dentry->d_sb->s_fs_info;
 	pid_t tgid = task_tgid_nr_ns(current, ns);
-	char tmp[PROC_NUMBUF];
-	if (!tgid)
-		return ERR_PTR(-ENOENT);
-	sprintf(tmp, "%d", task_tgid_nr_ns(current, ns));
-	return ERR_PTR(vfs_follow_link(nd,tmp));
+	char *name = ERR_PTR(-ENOENT);
+	if (tgid) {
+		name = __getname();
+		if (!name)
+			name = ERR_PTR(-ENOMEM);
+		else
+			sprintf(name, "%d", tgid);
+	}
+	nd_set_link(nd, name);
+	return NULL;
+}
+
+static void proc_self_put_link(struct dentry *dentry, struct nameidata *nd,
+				void *cookie)
+{
+	char *s = nd_get_link(nd);
+	if (!IS_ERR(s))
+		__putname(s);
 }
 
 static const struct inode_operations proc_self_inode_operations = {
 	.readlink	= proc_self_readlink,
 	.follow_link	= proc_self_follow_link,
+	.put_link	= proc_self_put_link,
 };
 
 /*

which I assume fixes the bug?

Quoted 12 lines

I'm still not 100% sure which way to go for -stable (and .33 - these are equivalent in that respect). Posted patches touch only the codepaths where we are currently guaranteed to leak and all things equal that'd be an argument in their favour. OTOH, the minimal alternative for e.g. gfs2 would be to make buffer allocation unconditional, use nd_set_link() instead of vfs_follow_link() and leave freeing to put_link(). Which can be followed by killing special gfs2 ->readlink(), since now generic_readlink() will work just fine (and gfs2_readlinki() can be folded into gfs2_follow_link(), simplifying things even more). In other words, long-term variant is not much more intrusive and it's just as safe. So that argument in favour of posted variant is not particulary strong, especially wrt 2.6.33.

Quoted 6 lines

So basically it boils down to doing truly minimal fix vs. doing the same thing (almost as trivial) we'll do past .33.

FWIW, it may be as simple as "is posted gfs2 patch already in a published gfs2 tree and if it is, how inconvenient for gfs2 folks would it be to replace it?"; I'm really ambivalent about that one...

But nothing for 2.6.33 or -stable yet?

What we could do is to merge the above into 2.6.34-rc1 with a Cc:[email protected] in the changelog and when it's had a bit of testing, the -stable guys could merge it back into 2.6.33.x, 2.6.32.x, etc?