Re: [PATCH 01/10] nfsd: fix BUG_ON in nfsd4_alloc_layout_stateid on racing delegation revoke

On Fri, 2026-05-29 at 09:40 +1000, NeilBrown wrote:
> On Fri, 29 May 2026, Jeff Layton wrote:
> > nfsd4_alloc_layout_stateid reads fp->fi_deleg_file without holding
> > fi_lock when the parent stateid is a delegation. A concurrent delegation
> > revoke via the laundromat can clear fi_deleg_file under fi_lock, causing
> > nfsd_file_get() to return NULL and triggering the BUG_ON.
> > 
> > This race is client-reachable: two NFS clients can trigger it by having
> > one hold a delegation while another opens the same file to force a
> > recall. When the first client doesn't respond to the recall, the
> > laundromat revokes it. A concurrent LAYOUTGET from any client using the
> > delegation stateid hits the race window.
> > 
> > Fix this by taking fi_lock around the fi_deleg_file read in the
> > SC_TYPE_DELEG path, and replacing the BUG_ON with a graceful error
> > return that cleans up the partially-initialized layout stateid.
> 
> Replacing the BUG_ON with a graceful error is certainly sensible and
> probably all that is needed to fix the problem.
> 
> I cannot see how the spinlock achieves anything.  If ->fi_deleg_file
> could become NULL at this point, it can become NULL just before we take
> the spinlock.
> 
> We do need to be sure the file (if there is one) doesn't get freed while
> nfsd_file_get() is incrementing the refcount, but rcu_read_lock() is the
> normal tool for that.
> In this case we have
> 
>   		ls->ls_file = nfsd_file_get(rcu_dereference_protected(fp->fi_deleg_file, 1));
> 
> rcu_dereference_protected(...., 1)
> which for me is a warning sign.  What does the '1' mean here?
> Presumably something that we cannot easily assert with a C condition, in
> which case a comment is called for.
> 
> Based on the recent commit which added this I'm guessing 
>    fi_delegees > 0 guarantees stability
> 
> If that is right, then why do we also need a spinlock to guarantee
> stability?
> 
> Confused.
> 
> NeilBrown
> 

Good point. I think the right thing to do here is to use RCU to access
this pointer. Don't think it's protected by anything now. I'll do some
more analysis and put together a v2.

Thanks for the review!
-- 
Jeff Layton <[email protected]>