[PATCH] procfs: Fix refcnt leak on proc_self_follow_link() error path

[PATCH] procfs: Fix refcnt leak on proc_self_follow_link() error path

[OGAWA Hirofumi]

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

This patch fix it.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/proc/base.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff -puN fs/proc/base.c~namei-procfs-follow_link-fix fs/proc/base.c
--- linux-2.6/fs/proc/base.c~namei-procfs-follow_link-fix	2010-01-12 00:15:15.000000000 +0900
+++ linux-2.6-hirofumi/fs/proc/base.c	2010-01-12 00:15:15.000000000 +0900
@@ -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));
 }
_

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] procfs: Fix refcnt leak on proc_self_follow_link() error path

[Al Viro]

On Tue, Jan 12, 2010 at 03:38:36AM +0900, OGAWA Hirofumi wrote:
> 
> 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.

Re: [PATCH] procfs: Fix refcnt leak on proc_self_follow_link() error path

[Andrew Morton]

On Wed, 13 Jan 2010 19:43:01 +0000
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Tue, Jan 12, 2010 at 03:38:36AM +0900, OGAWA Hirofumi wrote:
> > 
> > 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?

Re: [PATCH] procfs: Fix refcnt leak on proc_self_follow_link() error path

[Al Viro]

On Wed, Jan 13, 2010 at 04:25:04PM -0800, Andrew Morton wrote:
> On Wed, 13 Jan 2010 19:43:01 +0000
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > On Tue, Jan 12, 2010 at 03:38:36AM +0900, OGAWA Hirofumi wrote:
> > > 
> > > 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?

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 <ebiederm@xmission.com>
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...

Re: [PATCH] procfs: Fix refcnt leak on proc_self_follow_link() error path

[Andrew Morton]

On Thu, 14 Jan 2010 01:55:57 +0000
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Wed, Jan 13, 2010 at 04:25:04PM -0800, Andrew Morton wrote:
> > On Wed, 13 Jan 2010 19:43:01 +0000
> > Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > 
> > > On Tue, Jan 12, 2010 at 03:38:36AM +0900, OGAWA Hirofumi wrote:
> > > > 
> > > > 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?
> 
> 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 <ebiederm@xmission.com>
> 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 see this in linux-next:

commit 1427acc5655198f0196178846599a62656e92df0
Author:     Al Viro <viro@zeniv.linux.org.uk>
AuthorDate: Thu Jan 14 01:03:28 2010 -0500
Commit:     Al Viro <viro@zeniv.linux.org.uk>
CommitDate: Sat Jan 30 00:03:55 2010 -0500

    Switch proc/self to nd_set_link()
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/fs/proc/base.c b/fs/proc/base.c
index e42bbd8..58324c2 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -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?

> 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...

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:stable@kernel.org 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?