[PATCH] Fix mountpoint reference leakage in linkat

[PATCH] Fix mountpoint reference leakage in linkat

[Oleg Drokin]

Recent changes to retry on ESTALE in linkat
(commit 442e31ca5a49e398351b2954b51f578353fdf210)
introduced a mountpoint reference leak and a small memory
leak in case a filesystem link operation returns ESTALE
which is pretty normal for distributed filesystems like
lustre, nfs and so on.
Free old_path in such a case.

Signed-off-by: Oleg Drokin: <green@linuxhacker.ru>
---
 fs/namei.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/namei.c b/fs/namei.c
index bcb838e..e620937 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3931,6 +3931,7 @@ out_dput:
 			goto retry;
 	}
 	if (retry_estale(error, how)) {
+		path_put(&old_path);
 		how |= LOOKUP_REVAL;
 		goto retry;
 	}
-- 
1.8.5.3

Re: [PATCH] Fix mountpoint reference leakage in linkat

[Jeff Layton]

On Fri, 31 Jan 2014 15:41:58 -0500
Oleg Drokin <green@linuxhacker.ru> wrote:

> Recent changes to retry on ESTALE in linkat
> (commit 442e31ca5a49e398351b2954b51f578353fdf210)
> introduced a mountpoint reference leak and a small memory
> leak in case a filesystem link operation returns ESTALE
> which is pretty normal for distributed filesystems like
> lustre, nfs and so on.
> Free old_path in such a case.
> 
> Signed-off-by: Oleg Drokin: <green@linuxhacker.ru>
> ---
>  fs/namei.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index bcb838e..e620937 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3931,6 +3931,7 @@ out_dput:
>  			goto retry;
>  	}
>  	if (retry_estale(error, how)) {
> +		path_put(&old_path);
>  		how |= LOOKUP_REVAL;
>  		goto retry;
>  	}

Nice catch. This should probably also go to stable as well...

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH] Fix mountpoint reference leakage in linkat

[Al Viro]

On Fri, Jan 31, 2014 at 03:41:58PM -0500, Oleg Drokin wrote:
> Recent changes to retry on ESTALE in linkat
> (commit 442e31ca5a49e398351b2954b51f578353fdf210)
> introduced a mountpoint reference leak and a small memory
> leak in case a filesystem link operation returns ESTALE
> which is pretty normal for distributed filesystems like
> lustre, nfs and so on.
> Free old_path in such a case.
> 
> Signed-off-by: Oleg Drokin: <green@linuxhacker.ru>
> ---
>  fs/namei.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index bcb838e..e620937 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3931,6 +3931,7 @@ out_dput:
>  			goto retry;
>  	}
>  	if (retry_estale(error, how)) {
> +		path_put(&old_path);
>  		how |= LOOKUP_REVAL;
>  		goto retry;
>  	}

Umm...  That obviously can't be right - we have another goto retry
in the same situation (see in your diff context).  I agree that
we have a leak there, but you've fixed only a half of it.

Re: [PATCH] Fix mountpoint reference leakage in linkat

[Oleg Drokin]

On Jan 31, 2014, at 4:03 PM, Al Viro wrote:
>> diff --git a/fs/namei.c b/fs/namei.c
>> index bcb838e..e620937 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -3931,6 +3931,7 @@ out_dput:
>> 			goto retry;
>> 	}
>> 	if (retry_estale(error, how)) {
>> +		path_put(&old_path);
>> 		how |= LOOKUP_REVAL;
>> 		goto retry;
>> 	}
> Umm...  That obviously can't be right - we have another goto retry
> in the same situation (see in your diff context).  I agree that
> we have a leak there, but you've fixed only a half of it.

Hm, you are right, I did not notice this other one somehow.

So, not to take any guesses, should I convert the other goto retry into
retry_deleg similar in style to what happens in rename and unlink, only
make retry)deleg label before call to the security_path_link?
After the call to the security_path_link?
Or would you prefer to just free old_path in both cases?

Bye,
    Oleg

Re: [PATCH] Fix mountpoint reference leakage in linkat

[Jeff Layton]

On Fri, 31 Jan 2014 16:13:19 -0500
Oleg Drokin <green@linuxhacker.ru> wrote:

> 
> On Jan 31, 2014, at 4:03 PM, Al Viro wrote:
> >> diff --git a/fs/namei.c b/fs/namei.c
> >> index bcb838e..e620937 100644
> >> --- a/fs/namei.c
> >> +++ b/fs/namei.c
> >> @@ -3931,6 +3931,7 @@ out_dput:
> >> 			goto retry;
> >> 	}
> >> 	if (retry_estale(error, how)) {
> >> +		path_put(&old_path);
> >> 		how |= LOOKUP_REVAL;
> >> 		goto retry;
> >> 	}
> > Umm...  That obviously can't be right - we have another goto retry
> > in the same situation (see in your diff context).  I agree that
> > we have a leak there, but you've fixed only a half of it.
> 
> Hm, you are right, I did not notice this other one somehow.
> 
> So, not to take any guesses, should I convert the other goto retry into
> retry_deleg similar in style to what happens in rename and unlink, only
> make retry)deleg label before call to the security_path_link?
> After the call to the security_path_link?
> Or would you prefer to just free old_path in both cases?
> 
> Bye,
>     Oleg

Maybe something like this (untested) instead?

--------------------------8<---------------------------

[PATCH] vfs: fix linkat old_path reference leak

Cc: <stable@vger.kernel.org> # v3.8+
Reported-by: Oleg Drokin <green@linuxhacker.ru>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/namei.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 46dbd31..e70dd81 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3927,8 +3927,10 @@ retry:
 	new_dentry = user_path_create(newdfd, newname, &new_path,
 					(how & LOOKUP_REVAL));
 	error = PTR_ERR(new_dentry);
-	if (IS_ERR(new_dentry))
+	if (IS_ERR(new_dentry)) {
+		path_put(&old_path);
 		goto out;
+	}
 
 	error = -EXDEV;
 	if (old_path.mnt != new_path.mnt)
@@ -3942,6 +3944,7 @@ retry:
 	error = vfs_link(old_path.dentry, new_path.dentry->d_inode, new_dentry, &delegated_inode);
 out_dput:
 	done_path_create(&new_path, new_dentry);
+	path_put(&old_path);
 	if (delegated_inode) {
 		error = break_deleg_wait(&delegated_inode);
 		if (!error)
@@ -3952,8 +3955,6 @@ out_dput:
 		goto retry;
 	}
 out:
-	path_put(&old_path);
-
 	return error;
 }
 
-- 
1.8.5.3

Re: [PATCH] Fix mountpoint reference leakage in linkat

[Oleg Drokin]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]

> Maybe something like this (untested) instead?

I think it's just safe to move the out label up along with the old_path freeing.

In all error cases the retry and delegation breaking logic could not be triggered
anyway. 

Something like this (lightly tested):

[-- Attachment #2: 0001-vfs-Fix-mountpoint-reference-leakage-in-linkat.patch --]
[-- Type: application/octet-stream, Size: 1342 bytes --]

From a3b36802a01f4b89a16c773c0645bf374969970a Mon Sep 17 00:00:00 2001
From: Oleg Drokin <green@linuxhacker.ru>
Date: Fri, 31 Jan 2014 15:34:05 -0500
Subject: [PATCH] vfs: Fix mountpoint reference leakage in linkat

Recent changes to retry on ESTALE in linkat
(commit 442e31ca5a49e398351b2954b51f578353fdf210)
introduced a mountpoint reference leak and a small memory
leak in case a filesystem link operation returns ESTALE
which is pretty normal for distributed filesystems like
lustre, nfs and so on.
Free old_path in such a case.
Also fix a similar leak in case of broken delegation.

Signed-off-by: Oleg Drokin: <green@linuxhacker.ru>
---
 fs/namei.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index bcb838e..cc3c6c4 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3925,17 +3925,18 @@ retry:
 	error = vfs_link(old_path.dentry, new_path.dentry->d_inode, new_dentry, &delegated_inode);
 out_dput:
 	done_path_create(&new_path, new_dentry);
+out:
+	path_put(&old_path);
 	if (delegated_inode) {
 		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		if (!error) {
 			goto retry;
+		}
 	}
 	if (retry_estale(error, how)) {
 		how |= LOOKUP_REVAL;
 		goto retry;
 	}
-out:
-	path_put(&old_path);
 
 	return error;
 }
-- 
1.8.5.3

Re: [PATCH] Fix mountpoint reference leakage in linkat

[Al Viro]

On Fri, Jan 31, 2014 at 04:32:31PM -0500, Jeff Layton wrote:

>  	done_path_create(&new_path, new_dentry);
> +	path_put(&old_path);

... and the filesystem in question isn't pinned anymore, so it can be
unmounted, except that

>  	if (delegated_inode) {
>  		error = break_deleg_wait(&delegated_inode);

this does an iput() on delegated_inode.  And umount really doesn't
like finding pinned inodes.  Sorry, no go.

I'm going with Oleg's patch with second path_put() added.

[PATCH v2] vfs: Fix mountpoint reference leakage in linkat

[Oleg Drokin]

Recent changes to retry on ESTALE in linkat
(commit 442e31ca5a49e398351b2954b51f578353fdf210)
introduced a mountpoint reference leak and a small memory
leak in case a filesystem link operation returns ESTALE
which is pretty normal for distributed filesystems like
lustre, nfs and so on.
Free old_path in such a case.
Also fix a similar leak in case of broken delegation.

Signed-off-by: Oleg Drokin: <green@linuxhacker.ru>
---
 fs/namei.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index bcb838e..31ec503 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3927,10 +3927,13 @@ out_dput:
 	done_path_create(&new_path, new_dentry);
 	if (delegated_inode) {
 		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		if (!error) {
+			path_put(&old_path);
 			goto retry;
+		}
 	}
 	if (retry_estale(error, how)) {
+		path_put(&old_path);
 		how |= LOOKUP_REVAL;
 		goto retry;
 	}
-- 
1.8.5.3

Re: [PATCH] Fix mountpoint reference leakage in linkat

[Jeff Layton]

On Fri, 31 Jan 2014 22:30:14 +0000
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Fri, Jan 31, 2014 at 04:32:31PM -0500, Jeff Layton wrote:
> 
> >  	done_path_create(&new_path, new_dentry);
> > +	path_put(&old_path);
> 
> ... and the filesystem in question isn't pinned anymore, so it can be
> unmounted, except that
> 
> >  	if (delegated_inode) {
> >  		error = break_deleg_wait(&delegated_inode);
> 
> this does an iput() on delegated_inode.  And umount really doesn't
> like finding pinned inodes.  Sorry, no go.
> 
> I'm going with Oleg's patch with second path_put() added.

Good point. I hadn't considered that. Oleg's second patch looks good to
me too.

-- 
Jeff Layton <jlayton@redhat.com>