These patches do a little minor cleanup and close a small race left
after the vfs delegation patches.
--b.
From: "J. Bruce Fields" <[email protected]>
If a file is unlinked or renamed between the time when we do the local
open and the time when we get the delegation, then we will return to the
client indicating that it holds a delegation even though the file no
longer exists under the name it was open under.
But a client performing an open-by-name, when it is returned a
delegation, must be able to assume that the file is still linked at the
name it was opened under.
So, hold the parent i_mutex for longer to prevent concurrent renames or
unlinks.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4proc.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 844813a..ef76ba6 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -279,11 +279,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
if (open->op_createmode == NFS4_CREATE_EXCLUSIVE && status == 0)
open->op_bmval[1] = (FATTR4_WORD1_TIME_ACCESS |
FATTR4_WORD1_TIME_MODIFY);
- } else {
+ } else
+ /*
+ * Note this may exit with the parent still locked.
+ * We will hold the lock until nfsd4_open's final
+ * lookup, to prevent renames or unlinks until we've had
+ * a chance to an acquire a delegation if appropriate.
+ */
status = nfsd_lookup(rqstp, current_fh,
open->op_fname.data, open->op_fname.len, *resfh);
- fh_unlock(current_fh);
- }
if (status)
goto out;
status = nfsd_check_obj_isreg(*resfh);
--
1.7.9.5
From: "J. Bruce Fields" <[email protected]>
This is basically a no-op, to simplify a following patch.
Acked-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4proc.c | 37 ++++++++++++++++++++-----------------
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index dadff09..844813a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -230,17 +230,16 @@ static void nfsd4_set_open_owner_reply_cache(struct nfsd4_compound_state *cstate
}
static __be32
-do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open)
+do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh)
{
struct svc_fh *current_fh = &cstate->current_fh;
- struct svc_fh *resfh;
int accmode;
__be32 status;
- resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
- if (!resfh)
+ *resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
+ if (!*resfh)
return nfserr_jukebox;
- fh_init(resfh, NFS4_FHSIZE);
+ fh_init(*resfh, NFS4_FHSIZE);
open->op_truncate = 0;
if (open->op_create) {
@@ -265,12 +264,12 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
*/
status = do_nfsd_create(rqstp, current_fh, open->op_fname.data,
open->op_fname.len, &open->op_iattr,
- resfh, open->op_createmode,
+ *resfh, open->op_createmode,
(u32 *)open->op_verf.data,
&open->op_truncate, &open->op_created);
if (!status && open->op_label.len)
- nfsd4_security_inode_setsecctx(resfh, &open->op_label, open->op_bmval);
+ nfsd4_security_inode_setsecctx(*resfh, &open->op_label, open->op_bmval);
/*
* Following rfc 3530 14.2.16, use the returned bitmask
@@ -282,29 +281,26 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
FATTR4_WORD1_TIME_MODIFY);
} else {
status = nfsd_lookup(rqstp, current_fh,
- open->op_fname.data, open->op_fname.len, resfh);
+ open->op_fname.data, open->op_fname.len, *resfh);
fh_unlock(current_fh);
}
if (status)
goto out;
- status = nfsd_check_obj_isreg(resfh);
+ status = nfsd_check_obj_isreg(*resfh);
if (status)
goto out;
if (is_create_with_attrs(open) && open->op_acl != NULL)
- do_set_nfs4_acl(rqstp, resfh, open->op_acl, open->op_bmval);
+ do_set_nfs4_acl(rqstp, *resfh, open->op_acl, open->op_bmval);
- nfsd4_set_open_owner_reply_cache(cstate, open, resfh);
+ nfsd4_set_open_owner_reply_cache(cstate, open, *resfh);
accmode = NFSD_MAY_NOP;
if (open->op_created ||
open->op_claim_type == NFS4_OPEN_CLAIM_DELEGATE_CUR)
accmode |= NFSD_MAY_OWNER_OVERRIDE;
- status = do_open_permission(rqstp, resfh, open, accmode);
+ status = do_open_permission(rqstp, *resfh, open, accmode);
set_change_info(&open->op_cinfo, current_fh);
- fh_dup2(current_fh, resfh);
out:
- fh_put(resfh);
- kfree(resfh);
return status;
}
@@ -357,6 +353,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd4_open *open)
{
__be32 status;
+ struct svc_fh *resfh = NULL;
struct nfsd4_compoundres *resp;
struct net *net = SVC_NET(rqstp);
struct nfsd_net *nn = net_generic(net, nfsd_net_id);
@@ -423,7 +420,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
switch (open->op_claim_type) {
case NFS4_OPEN_CLAIM_DELEGATE_CUR:
case NFS4_OPEN_CLAIM_NULL:
- status = do_open_lookup(rqstp, cstate, open);
+ status = do_open_lookup(rqstp, cstate, open, &resfh);
if (status)
goto out;
break;
@@ -439,6 +436,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = do_open_fhandle(rqstp, cstate, open);
if (status)
goto out;
+ resfh = &cstate->current_fh;
break;
case NFS4_OPEN_CLAIM_DELEG_PREV_FH:
case NFS4_OPEN_CLAIM_DELEGATE_PREV:
@@ -458,9 +456,14 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
* successful, it (1) truncates the file if open->op_truncate was
* set, (2) sets open->op_stateid, (3) sets open->op_delegation.
*/
- status = nfsd4_process_open2(rqstp, &cstate->current_fh, open);
+ status = nfsd4_process_open2(rqstp, resfh, open);
WARN_ON(status && open->op_created);
out:
+ if (resfh && resfh != &cstate->current_fh) {
+ fh_dup2(&cstate->current_fh, resfh);
+ fh_put(resfh);
+ kfree(resfh);
+ }
nfsd4_cleanup_open_state(open, status);
if (open->op_openowner && !nfsd4_has_session(cstate))
cstate->replay_owner = &open->op_openowner->oo_owner;
--
1.7.9.5
From: "J. Bruce Fields" <[email protected]>
As far as I can tell, this list is used only under the state lock, so we
may as well do this in the simpler order.
Acked-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 5795d5f..ed3085b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3043,18 +3043,18 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
if (!fl)
return -ENOMEM;
fl->fl_file = find_readable_file(fp);
- list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
- if (status) {
- list_del_init(&dp->dl_perclnt);
- locks_free_lock(fl);
- return status;
- }
+ if (status)
+ goto out_free;
+ list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
fp->fi_lease = fl;
fp->fi_deleg_file = get_file(fl->fl_file);
atomic_set(&fp->fi_delegees, 1);
list_add(&dp->dl_perfile, &fp->fi_delegations);
return 0;
+out_free:
+ locks_free_lock(fl);
+ return status;
}
static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
--
1.7.9.5
On Sun, Jan 26, 2014 at 03:59:21PM -0500, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <[email protected]>
>
> If a file is unlinked or renamed between the time when we do the local
> open and the time when we get the delegation, then we will return to the
> client indicating that it holds a delegation even though the file no
> longer exists under the name it was open under.
>
> But a client performing an open-by-name, when it is returned a
> delegation, must be able to assume that the file is still linked at the
> name it was opened under.
>
> So, hold the parent i_mutex for longer to prevent concurrent renames or
> unlinks.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfsd/nfs4proc.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 844813a..ef76ba6 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -279,11 +279,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> if (open->op_createmode == NFS4_CREATE_EXCLUSIVE && status == 0)
> open->op_bmval[1] = (FATTR4_WORD1_TIME_ACCESS |
> FATTR4_WORD1_TIME_MODIFY);
> - } else {
> + } else
> + /*
> + * Note this may exit with the parent still locked.
> + * We will hold the lock until nfsd4_open's final
> + * lookup, to prevent renames or unlinks until we've had
> + * a chance to an acquire a delegation if appropriate.
> + */
> status = nfsd_lookup(rqstp, current_fh,
> open->op_fname.data, open->op_fname.len, *resfh);
> - fh_unlock(current_fh);
> - }
> if (status)
> goto out;
> status = nfsd_check_obj_isreg(*resfh);
One last-minute fix: we can now end up taking two i_mutexes. The
locking's still correct but we need the following annotation on the
parent directory.
(I haven't actually seen any lockdep warning trigger here.)
--b.
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index e85b463..a41302a 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -207,7 +207,12 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
goto out_nfserr;
}
} else {
- fh_lock(fhp);
+ /*
+ * In the nfsd4_open() case, this may be held across
+ * subsequent open and delegation acquisition which may
+ * need to take the child's i_mutex:
+ */
+ fh_lock_nested(fhp, I_MUTEX_PARENT);
dentry = lookup_one_len(name, dparent, len);
host_err = PTR_ERR(dentry);
if (IS_ERR(dentry))