* [PATCH] Fix mountpoint reference leakage in linkat @ 2014-01-31 20:41 Oleg Drokin 2014-01-31 20:52 ` Jeff Layton 2014-01-31 21:03 ` Al Viro 0 siblings, 2 replies; 9+ messages in thread From: Oleg Drokin @ 2014-01-31 20:41 UTC (permalink / raw) To: linux-kernel, jlayton, viro, linux-fsdevel; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix mountpoint reference leakage in linkat 2014-01-31 20:41 [PATCH] Fix mountpoint reference leakage in linkat Oleg Drokin @ 2014-01-31 20:52 ` Jeff Layton 2014-01-31 21:03 ` Al Viro 1 sibling, 0 replies; 9+ messages in thread From: Jeff Layton @ 2014-01-31 20:52 UTC (permalink / raw) To: Oleg Drokin; +Cc: linux-kernel, viro, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix mountpoint reference leakage in linkat 2014-01-31 20:41 [PATCH] Fix mountpoint reference leakage in linkat Oleg Drokin 2014-01-31 20:52 ` Jeff Layton @ 2014-01-31 21:03 ` Al Viro 2014-01-31 21:13 ` Oleg Drokin 1 sibling, 1 reply; 9+ messages in thread From: Al Viro @ 2014-01-31 21:03 UTC (permalink / raw) To: Oleg Drokin; +Cc: linux-kernel, jlayton, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix mountpoint reference leakage in linkat 2014-01-31 21:03 ` Al Viro @ 2014-01-31 21:13 ` Oleg Drokin 2014-01-31 21:32 ` Jeff Layton 0 siblings, 1 reply; 9+ messages in thread From: Oleg Drokin @ 2014-01-31 21:13 UTC (permalink / raw) To: Al Viro; +Cc: linux-kernel, jlayton, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix mountpoint reference leakage in linkat 2014-01-31 21:13 ` Oleg Drokin @ 2014-01-31 21:32 ` Jeff Layton 2014-01-31 21:50 ` Oleg Drokin 2014-01-31 22:30 ` Al Viro 0 siblings, 2 replies; 9+ messages in thread From: Jeff Layton @ 2014-01-31 21:32 UTC (permalink / raw) To: Oleg Drokin; +Cc: Al Viro, linux-kernel, linux-fsdevel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix mountpoint reference leakage in linkat 2014-01-31 21:32 ` Jeff Layton @ 2014-01-31 21:50 ` Oleg Drokin 2014-01-31 22:30 ` Al Viro 1 sibling, 0 replies; 9+ messages in thread From: Oleg Drokin @ 2014-01-31 21:50 UTC (permalink / raw) To: Jeff Layton; +Cc: Al Viro, linux-kernel, linux-fsdevel [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix mountpoint reference leakage in linkat 2014-01-31 21:32 ` Jeff Layton 2014-01-31 21:50 ` Oleg Drokin @ 2014-01-31 22:30 ` Al Viro 2014-01-31 22:34 ` [PATCH v2] vfs: " Oleg Drokin 2014-01-31 23:11 ` [PATCH] " Jeff Layton 1 sibling, 2 replies; 9+ messages in thread From: Al Viro @ 2014-01-31 22:30 UTC (permalink / raw) To: Jeff Layton; +Cc: Oleg Drokin, linux-kernel, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] vfs: Fix mountpoint reference leakage in linkat 2014-01-31 22:30 ` Al Viro @ 2014-01-31 22:34 ` Oleg Drokin 2014-01-31 23:11 ` [PATCH] " Jeff Layton 1 sibling, 0 replies; 9+ messages in thread From: Oleg Drokin @ 2014-01-31 22:34 UTC (permalink / raw) To: linux-kernel, jlayton, viro, linux-fsdevel; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix mountpoint reference leakage in linkat 2014-01-31 22:30 ` Al Viro 2014-01-31 22:34 ` [PATCH v2] vfs: " Oleg Drokin @ 2014-01-31 23:11 ` Jeff Layton 1 sibling, 0 replies; 9+ messages in thread From: Jeff Layton @ 2014-01-31 23:11 UTC (permalink / raw) To: Al Viro; +Cc: Oleg Drokin, linux-kernel, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-01-31 23:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-31 20:41 [PATCH] Fix mountpoint reference leakage in linkat Oleg Drokin 2014-01-31 20:52 ` Jeff Layton 2014-01-31 21:03 ` Al Viro 2014-01-31 21:13 ` Oleg Drokin 2014-01-31 21:32 ` Jeff Layton 2014-01-31 21:50 ` Oleg Drokin 2014-01-31 22:30 ` Al Viro 2014-01-31 22:34 ` [PATCH v2] vfs: " Oleg Drokin 2014-01-31 23:11 ` [PATCH] " Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).