On Fri, Nov 17, 2023 at 01:18:47PM +1100, NeilBrown wrote: > The protocol for creating a new state in nfsd is to allocated the state > leaving it largely uninitialised, add that state to the ->cl_stateids > idr so as to reserve a state id, then complete initialisation of the > state and only set ->sc_type to non-zero once the state is fully > initialised. > > If a state is found in the idr with ->sc_type == 0, it is ignored. > The ->cl_lock list is used to avoid races - it is held while checking s/->cl_lock list/->cl_lock lock > sc_type during lookup, In particular, find_stateid_locked(), but yet, not in nfs4_find_file() Can you help me understand why it's not needed in the latter case? > and held when a non-zero value is stored in ->sc_type. I see a few additional spots where an sc_type value is set and cl_lock is not held: init_open_stateid nfsd4_process_open2 > ... except... hash_delegation_locked() finalises the initialisation of a > delegation state, but does NOT hold ->cl_lock. > > So this patch takes ->cl_lock at the appropriate time w.r.t other locks, > and so ensures there are no races (which are extremely unlikely in any > case). I would have expected that cl_lock should be taken first. Can the patch description provide some rationale for the lock ordering you chose? Jeff asks in another email whether this fix should get copied to stable. Since the race is unlikely, I'm inclined to wait for an explicit problem report. > Signed-off-by: NeilBrown <neilb@xxxxxxx> > --- > fs/nfsd/nfs4state.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 65fd5510323a..6368788a7d4e 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1317,6 +1317,7 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp) > > lockdep_assert_held(&state_lock); > lockdep_assert_held(&fp->fi_lock); > + lockdep_assert_held(&clp->cl_lock); > > if (nfs4_delegation_exists(clp, fp)) > return -EAGAIN; > @@ -5609,12 +5610,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > goto out_unlock; > > spin_lock(&state_lock); > + spin_lock(&clp->cl_lock); > spin_lock(&fp->fi_lock); > if (fp->fi_had_conflict) > status = -EAGAIN; > else > status = hash_delegation_locked(dp, fp); > spin_unlock(&fp->fi_lock); > + spin_unlock(&clp->cl_lock); > spin_unlock(&state_lock); > > if (status) > -- > 2.42.0 > -- Chuck Lever
