On Tue, 2017-10-17 at 09:55 -0400, Benjamin Coddington wrote:
> While running generic/089 on NFSv4.1, the following on-the-wire exchange
> occurs:
>
> Client Server
> ---------- ----------
> OPEN (owner A) ->
> <- OPEN response: state A1
> CLOSE (state A1)->
> OPEN (owner A) ->
> <- CLOSE response: state A2
> <- OPEN response: state A3
> LOCK (state A3) ->
> <- LOCK response: NFS4_BAD_STATEID
>
> The server should not be returning state A3 in response to the second OPEN
> after processing the CLOSE and sending back state A2. The problem is that
> nfsd4_process_open2() is able to find an existing open state just after
> nfsd4_close() has incremented the state's sequence number but before
> calling unhash_ol_stateid().
>
> Fix this by using the state's sc_type field to identify closed state, and
> protect the update of that field with st_mutex. nfsd4_find_existing_open()
> will return with the st_mutex held, so that the state will not transition
> between being looked up and then updated in nfsd4_process_open2().
>
> Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> ---
> fs/nfsd/nfs4state.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
The problem is real, but I'm not thrilled with the fix. It seems more
lock thrashy...
Could we instead fix this by just unhashing the stateid earlier, before
the nfs4_inc_and_copy_stateid call?
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0c04f81aa63b..17473a092d01 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3503,11 +3503,12 @@ static const struct nfs4_stateowner_operations openowner_ops = {
> static struct nfs4_ol_stateid *
> nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
> {
> - struct nfs4_ol_stateid *local, *ret = NULL;
> + struct nfs4_ol_stateid *local, *ret;
> struct nfs4_openowner *oo = open->op_openowner;
>
> - lockdep_assert_held(&fp->fi_lock);
> -
> +retry:
> + ret = NULL;
> + spin_lock(&fp->fi_lock);
> list_for_each_entry(local, &fp->fi_stateids, st_perfile) {
> /* ignore lock owners */
> if (local->st_stateowner->so_is_open_owner == 0)
> @@ -3518,6 +3519,18 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
> break;
> }
> }
> + spin_unlock(&fp->fi_lock);
> +
> + /* Did we race with CLOSE? */
> + if (ret) {
> + mutex_lock(&ret->st_mutex);
> + if (ret->st_stid.sc_type != NFS4_OPEN_STID) {
> + mutex_unlock(&ret->st_mutex);
> + nfs4_put_stid(&ret->st_stid);
> + goto retry;
> + }
> + }
> +
> return ret;
> }
>
> @@ -3566,7 +3579,6 @@ init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
> mutex_lock(&stp->st_mutex);
>
> spin_lock(&oo->oo_owner.so_client->cl_lock);
> - spin_lock(&fp->fi_lock);
>
> retstp = nfsd4_find_existing_open(fp, open);
> if (retstp)
> @@ -3583,13 +3595,13 @@ init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
> stp->st_deny_bmap = 0;
> stp->st_openstp = NULL;
> list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
> + spin_lock(&fp->fi_lock);
> list_add(&stp->st_perfile, &fp->fi_stateids);
> + spin_unlock(&fp->fi_lock);
>
> out_unlock:
> - spin_unlock(&fp->fi_lock);
> spin_unlock(&oo->oo_owner.so_client->cl_lock);
> if (retstp) {
> - mutex_lock(&retstp->st_mutex);
> /* To keep mutex tracking happy */
> mutex_unlock(&stp->st_mutex);
> stp = retstp;
> @@ -4403,9 +4415,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> status = nfs4_check_deleg(cl, open, &dp);
> if (status)
> goto out;
> - spin_lock(&fp->fi_lock);
> stp = nfsd4_find_existing_open(fp, open);
> - spin_unlock(&fp->fi_lock);
> } else {
> open->op_file = NULL;
> status = nfserr_bad_stateid;
> @@ -4419,7 +4429,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> */
> if (stp) {
> /* Stateid was found, this is an OPEN upgrade */
> - mutex_lock(&stp->st_mutex);
> status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
> if (status) {
> mutex_unlock(&stp->st_mutex);
> @@ -5294,7 +5303,6 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> bool unhashed;
> LIST_HEAD(reaplist);
>
> - s->st_stid.sc_type = NFS4_CLOSED_STID;
> spin_lock(&clp->cl_lock);
> unhashed = unhash_open_stateid(s, &reaplist);
>
> @@ -5335,6 +5343,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> if (status)
> goto out;
> nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
> + stp->st_stid.sc_type = NFS4_CLOSED_STID;
> mutex_unlock(&stp->st_mutex);
>
> nfsd4_close_open_stateid(stp);
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
