| Message ID | 4FA345DA4F4AE44899BD2B03EEEC2FA9235DC4AF@SACEXCMBX04-PRD.hq.netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 22 Feb 2013 18:02:06 +0000 "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > On Tue, 2013-02-19 at 08:59 -0500, Dave Wysochanski wrote: > > Commit 73ca100 broke the code that prevents the client from deleting > > a silly renamed dentry. This affected "delete on last close" > > semantics as after that commit, nothing prevented removal of > > silly-renamed files. As a result, a process holding a file open > > could easily get an ESTALE on the file in a directory where some > > other process issued 'rm -rf some_dir_containing_the_file' twice. > > Before the commit, any attempt at unlinking silly renamed files would > > fail inside may_delete() with -EBUSY because of the > > DCACHE_NFSFS_RENAMED flag. The following testcase demonstrates > > the problem: > > tail -f /nfsmnt/dir/file & > > rm -rf /nfsmnt/dir > > rm -rf /nfsmnt/dir > > # second removal does not fail, 'tail' process receives ESTALE > > Hi Dave, > > I'm not sure I understand why we must do the dropping/moving of the > dentries inside nfs_async_rename_done. Why isn't something like the > attached patch sufficient? > > Cheers, > Trond > That looks like a much simpler alternative. Nice work. Acked-by: Jeff Layton <jlayton@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-02-22 at 18:02 +0000, Myklebust, Trond wrote: > On Tue, 2013-02-19 at 08:59 -0500, Dave Wysochanski wrote: > > Commit 73ca100 broke the code that prevents the client from deleting > > a silly renamed dentry. This affected "delete on last close" > > semantics as after that commit, nothing prevented removal of > > silly-renamed files. As a result, a process holding a file open > > could easily get an ESTALE on the file in a directory where some > > other process issued 'rm -rf some_dir_containing_the_file' twice. > > Before the commit, any attempt at unlinking silly renamed files would > > fail inside may_delete() with -EBUSY because of the > > DCACHE_NFSFS_RENAMED flag. The following testcase demonstrates > > the problem: > > tail -f /nfsmnt/dir/file & > > rm -rf /nfsmnt/dir > > rm -rf /nfsmnt/dir > > # second removal does not fail, 'tail' process receives ESTALE > > Hi Dave, > > I'm not sure I understand why we must do the dropping/moving of the > dentries inside nfs_async_rename_done. Why isn't something like the > attached patch sufficient? > As far as I can tell, it is sufficient. My solution was overly complex and your refinement is much more elegant. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From 90769817a805292646623cedd9d5455d31b63bc2 Mon Sep 17 00:00:00 2001 From: Trond Myklebust <Trond.Myklebust@netapp.com> Date: Fri, 22 Feb 2013 12:53:43 -0500 Subject: [PATCH] NFS: Don't allow NFS silly-renamed files to be deleted, no signal Commit 73ca100 broke the code that prevents the client from deleting a silly renamed dentry. This affected "delete on last close" semantics as after that commit, nothing prevented removal of silly-renamed files. As a result, a process holding a file open could easily get an ESTALE on the file in a directory where some other process issued 'rm -rf some_dir_containing_the_file' twice. Before the commit, any attempt at unlinking silly renamed files would fail inside may_delete() with -EBUSY because of the DCACHE_NFSFS_RENAMED flag. The following testcase demonstrates the problem: tail -f /nfsmnt/dir/file & rm -rf /nfsmnt/dir rm -rf /nfsmnt/dir # second removal does not fail, 'tail' process receives ESTALE The problem with the above commit is that it unhashes the old and new dentries from the lookup path, even in the normal case when a signal is not encountered and it would have been safe to call d_move. Unfortunately the old dentry has the special DCACHE_NFSFS_RENAMED flag set on it. Unhashing has the side-effect that future lookups call d_alloc(), allocating a new dentry without the special flag for any silly-renamed files. As a result, subsequent calls to unlink silly renamed files do not fail but allow the removal to go through. This will result in ESTALE errors for any other process doing operations on the file. To fix this, go back to using d_move on success. For the signal case, it's unclear what we may safely do beyond d_drop. Reported-by: Dave Wysochanski <dwysocha@redhat.com> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> Cc: stable@vger.kernel.org --- fs/nfs/unlink.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c index d26a32f..6a8368e 100644 --- a/fs/nfs/unlink.c +++ b/fs/nfs/unlink.c @@ -335,20 +335,14 @@ static void nfs_async_rename_done(struct rpc_task *task, void *calldata) struct inode *old_dir = data->old_dir; struct inode *new_dir = data->new_dir; struct dentry *old_dentry = data->old_dentry; - struct dentry *new_dentry = data->new_dentry; if (!NFS_PROTO(old_dir)->rename_done(task, old_dir, new_dir)) { rpc_restart_call_prepare(task); return; } - if (task->tk_status != 0) { + if (task->tk_status != 0) nfs_cancel_async_unlink(old_dentry); - return; - } - - d_drop(old_dentry); - d_drop(new_dentry); } /** @@ -549,6 +543,15 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry) error = rpc_wait_for_completion_task(task); if (error == 0) error = task->tk_status; + switch (error) { + case 0: + nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); + d_move(dentry, sdentry); + break; + case -ERESTARTSYS: + d_drop(dentry); + d_drop(sdentry); + } rpc_put_task(task); out_dput: dput(sdentry); -- 1.8.1.2