nfs_complete_rename() calls d_move() without i_mutex
Subsystems (2)
v4 cover nfs_complete_rename() calls d_move() without i_mutex
Once commit 920b4530fb80430ff30ef83efe21ba1fa5623731 "NFS: nfs_rename() handle -ERESTARTSYS dentry left behind" moved the local d_move() into the RPC asyncronous context, d_move() could be called without holding the directories' i_mutex.
Let's revert that commit, and a follow-up fix for it in 1/2, and then fix the original problem once more by forcing a revalidation of the old and new directories if we notice that the rename was interrupted in 2/2.
v2: Add memory barrier, hold directory i_locks while revalidating, specify
single-bit field width for cancelled flag.
v3: Add some comments, change cancelled flag to bool.
v4: Where old_dir == new_dir, don't bump revalidation counter twice.
Benjamin Coddington (2):
Revert "NFS: nfs_rename() handle -ERESTARTSYS dentry left behind"
NFS: nfs_rename() - revalidate directories on -ERESTARTSYS
fs/nfs/dir.c | 57 +++++++++++++++++++++++++------------------------ fs/nfs/unlink.c | 13 +++++++++++ include/linux/nfs_xdr.h | 1 + 3 files changed, 43 insertions(+), 28 deletions(-)
Signature · 1 line
2.9.3
PATCH v4 2/2 NFS: nfs_rename() - revalidate directories on -ERESTARTSYS
An interrupted rename will leave the old dentry behind if the rename succeeds. Fix this by forcing a lookup the next time through ->d_revalidate.
A previous attempt at solving this problem took the approach to complete the work of the rename asynchronously, however that approach was wrong since it would allow the d_move() to occur after the directory's i_mutex had been dropped by the original process.
--- fs/nfs/dir.c | 6 +++++- fs/nfs/unlink.c | 13 +++++++++++++ include/linux/nfs_xdr.h | 1 + 3 files changed, 19 insertions(+), 1 deletion(-)
fs/nfs/dir.c +5 -1
@@ -2035,7 +2035,11 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry, } error =rpc_wait_for_completion_task (task); -if (error ==0 ) +if (error !=0 ) { + ((struct nfs_renamedata *)task->tk_calldata)->cancelled =1 ; +/* Paired with the atomic_dec_and_test() barrier in rpc_do_put_task() */ +smp_wmb (); + }else error = task->tk_status;rpc_put_task (task);nfs_mark_for_revalidate (old_inode);
fs/nfs/unlink.c +13 -0
@@ -288,6 +288,19 @@ static void nfs_async_rename_release(void *calldata)if (d_really_is_positive (data->old_dentry))nfs_mark_for_revalidate (d_inode (data->old_dentry)); +/* The result of the rename is unknown. Play it safe by +* forcing a new lookup */ +if (data->cancelled) { +spin_lock (&data->old_dir->i_lock); +nfs_force_lookup_revalidate (data->old_dir); +spin_unlock (&data->old_dir->i_lock); +if (data->new_dir != data->old_dir) { +spin_lock (&data->new_dir->i_lock); +nfs_force_lookup_revalidate (data->new_dir); +spin_unlock (&data->new_dir->i_lock); + } + } +dput (data->old_dentry);dput (data->new_dentry);iput (data->old_dir);
include/linux/nfs_xdr.h +1 -0
@@ -1533,6 +1533,7 @@ struct nfs_renamedata {struct nfs_fattr new_fattr;void (*complete)(struct rpc_task *,struct nfs_renamedata *);long timeout; +bool cancelled; };struct nfs_access_entry ;
Signature · 1 line
2.9.3
PATCH v4 1/2 Revert "NFS: nfs_rename() handle -ERESTARTSYS dentry left behind"
In reply to Benjamin CoddingtonThis reverts commit 920b4530fb80430ff30ef83efe21ba1fa5623731 which could call d_move() without holding the directory's i_mutex, and reverts commit d4ea7e3c5c0e341c15b073016dbf3ab6c65f12f3 "NFS: Fix old dentry rehash after move", which was a follow-up fix.
--- fs/nfs/dir.c | 51 ++++++++++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 27 deletions(-)
fs/nfs/dir.c +24 -27
@@ -1946,29 +1946,6 @@ nfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry) }EXPORT_SYMBOL_GPL (nfs_link); -static void -nfs_complete_rename (struct rpc_task *task,struct nfs_renamedata *data) -{ -struct dentry *old_dentry = data->old_dentry; -struct dentry *new_dentry = data->new_dentry; -struct inode *old_inode =d_inode (old_dentry); -struct inode *new_inode =d_inode (new_dentry); - -nfs_mark_for_revalidate (old_inode); - -switch (task->tk_status) { -case 0 : -if (new_inode !=NULL ) -nfs_drop_nlink (new_inode); -d_move (old_dentry, new_dentry); -nfs_set_verifier (new_dentry, -nfs_save_change_attribute (data->new_dir)); -break ; -case -ENOENT : -nfs_dentry_handle_enoent (old_dentry); - } -} -/* * RENAME * FIXME: Some nfsds, like the Linux user space nfsd, may generate a
@@ -1999,7 +1976,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry, {struct inode *old_inode =d_inode (old_dentry);struct inode *new_inode =d_inode (new_dentry); -struct dentry *dentry =NULL ; +struct dentry *dentry =NULL , *rehash =NULL ;struct rpc_task *task;int error = -EBUSY ;
@@ -2022,8 +1999,10 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,* To prevent any new references to the target during the * rename, we unhash the dentry in advance. */ -if (!d_unhashed (new_dentry)) +if (!d_unhashed (new_dentry)) {d_drop (new_dentry); + rehash = new_dentry; + }if (d_count (new_dentry) >2 ) {int err;
@@ -2040,6 +2019,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry, goto out; new_dentry = dentry; + rehash =NULL ; new_inode =NULL ; } }
@@ -2048,8 +2028,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,if (new_inode !=NULL )NFS_PROTO (new_inode)->return_delegation (new_inode); - task =nfs_async_rename (old_dir, new_dir, old_dentry, new_dentry, - nfs_complete_rename); + task =nfs_async_rename (old_dir, new_dir, old_dentry, new_dentry,NULL );if (IS_ERR (task)) { error =PTR_ERR (task); goto out;
@@ -2059,9 +2038,27 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,if (error ==0 ) error = task->tk_status;rpc_put_task (task); +nfs_mark_for_revalidate (old_inode); out: +if (rehash) +d_rehash (rehash);trace_nfs_rename_exit (old_dir, old_dentry, new_dir, new_dentry, error); +if (!error) { +if (new_inode !=NULL ) +nfs_drop_nlink (new_inode); +/* +* The d_move() should be here instead of in an async RPC completion +* handler because we need the proper locks to move the dentry. If +* we're interrupted by a signal, the async RPC completion handler +* should mark the directories for revalidation. +*/ +d_move (old_dentry, new_dentry); +nfs_set_verifier (new_dentry, +nfs_save_change_attribute (new_dir)); + }else if (error == -ENOENT ) +nfs_dentry_handle_enoent (old_dentry); +/* new dentry created? */ if (dentry)dput (dentry);
Signature · 1 line
2.9.3