Re: [PATCH 1/1] nfsd: fix possible badness in FREE_STATEID

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 2024-10-05 at 10:53 -0400, Chuck Lever wrote:
> On Fri, Oct 04, 2024 at 06:04:03PM -0400, Olga Kornievskaia wrote:
> > When multiple FREE_STATEIDs are sent for the same delegation stateid,
> > it can lead to a possible either use-after-tree or counter refcount
> > underflow errors.
> > 
> > In nfsd4_free_stateid() under the client lock we find a delegation
> > stateid, however the code drops the lock before calling nfs4_put_stid(),
> > that allows another FREE_STATE to find the stateid again. The first one
> > will proceed to then free the stateid which leads to either
> > use-after-free or decrementing already zerod counter.
> > 
> > CC: stable@xxxxxxxxxxxxxxx
> 
> I assume that the broken commit is pretty old, but this fix does not
> apply before v6.9 (where sc_status is introduced). I can add
> "# v6.9+" to the Cc: stable tag.
> 

I don't know. It looks like nfsd4_free_stateid always returned
NFS4ERR_LOCKS_HELD on a delegation stateid until 3f29cc82a84c.

> But what do folks think about a Fixes: tag?
> 
> Could be e1ca12dfb1be ("NFSD: added FREE_STATEID operation"), but
> that doesn't have the switch statement, which was added by
> 2da1cec713bc ("nfsd4: simplify free_stateid").
> 
> 

Maybe this one?

    3f29cc82a84c nfsd: split sc_status out of sc_type

That particular bit of the code (and the SC_STATUS_CLOSED flag) was
added in that patch, and I don't think you'd want to apply this patch
to anything that didn't have it.

> > Signed-off-by: Olga Kornievskaia <okorniev@xxxxxxxxxx>
> > ---
> >  fs/nfsd/nfs4state.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index ac1859c7cc9d..56b261608af4 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -7154,6 +7154,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	switch (s->sc_type) {
> >  	case SC_TYPE_DELEG:
> >  		if (s->sc_status & SC_STATUS_REVOKED) {
> > +			s->sc_status |= SC_STATUS_CLOSED;
> >  			spin_unlock(&s->sc_lock);
> >  			dp = delegstateid(s);
> >  			list_del_init(&dp->dl_recall_lru);
> > -- 
> > 2.43.5
> > 
> 

-- 
Jeff Layton <jlayton@xxxxxxxxxx>





[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux