Return-Path: Received: from fieldses.org ([173.255.197.46]:55034 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750932AbdKJWDS (ORCPT ); Fri, 10 Nov 2017 17:03:18 -0500 Date: Fri, 10 Nov 2017 17:03:18 -0500 From: "J. Bruce Fields" To: Benjamin Coddington Cc: Jeff Layton , linux-nfs@vger.kernel.org Subject: Re: [PATCH] nfsd4: Prevent the reuse of a closed stateid Message-ID: <20171110220318.GQ8773@fieldses.org> References: <2087b4cab6c695a7df01c1135f1773c9b762f0b0.1508248427.git.bcodding@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <2087b4cab6c695a7df01c1135f1773c9b762f0b0.1508248427.git.bcodding@redhat.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: I'm assuming this has been addressed by Trond's series (in linux-next now); but I haven't checked carefully.... --b. On Tue, Oct 17, 2017 at 09:55:05AM -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 > --- > fs/nfsd/nfs4state.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > 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); > -- > 2.9.3