Re: [PATCH 1/1] nfsd: fix race between laundromat and free_stateid

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2024-10-11 at 10:39 -0400, Chuck Lever wrote:
> On Thu, Oct 10, 2024 at 06:18:01PM -0400, Olga Kornievskaia wrote:
> > There is a race between laundromat handling of revoked delegations
> > and a client sending free_stateid operation. Laundromat thread
> > finds that delegation has expired and needs to be revoked so it
> > marks the delegation stid revoked and it puts it on a reaper list
> > but then it unlock the state lock and the actual delegation
> > revocation
> > happens without the lock. Once the stid is marked revoked a racing
> > free_stateid processing thread does the following (1) it calls
> > list_del_init() which removes it from the reaper list and (2) frees
> > the delegation stid structure. The laundromat thread ends up not
> > calling the revoke_delegation() function for this particular
> > delegation
> > but that means it will no release the lock lease that exists on
> > the file.
> > 
> > Now, a new open for this file comes in and ends up finding that
> > lease list isn't empty and calls nfsd_breaker_owns_lease() which
> > ends
> > up trying to derefence a freed delegation stateid. Leading to the
> > followint use-after-free KASAN warning:
> > 
> > kernel:
> > ==================================================================
> > kernel: BUG: KASAN: slab-use-after-free in
> > nfsd_breaker_owns_lease+0x140/0x160 [nfsd]
> > kernel: Read of size 8 at addr ffff0000e73cd0c8 by task nfsd/6205
> > kernel:
> > kernel: CPU: 2 UID: 0 PID: 6205 Comm: nfsd Kdump: loaded Not
> > tainted 6.11.0-rc7+ #9
> > kernel: Hardware name: Apple Inc. Apple Virtualization Generic
> > Platform, BIOS 2069.0.0.0.0 08/03/2024
> > kernel: Call trace:
> > kernel: dump_backtrace+0x98/0x120
> > kernel: show_stack+0x1c/0x30
> > kernel: dump_stack_lvl+0x80/0xe8
> > kernel: print_address_description.constprop.0+0x84/0x390
> > kernel: print_report+0xa4/0x268
> > kernel: kasan_report+0xb4/0xf8
> > kernel: __asan_report_load8_noabort+0x1c/0x28
> > kernel: nfsd_breaker_owns_lease+0x140/0x160 [nfsd]
> > kernel: leases_conflict+0x68/0x370
> > kernel: __break_lease+0x204/0xc38
> > kernel: nfsd_open_break_lease+0x8c/0xf0 [nfsd]
> > kernel: nfsd_file_do_acquire+0xb3c/0x11d0 [nfsd]
> > kernel: nfsd_file_acquire_opened+0x84/0x110 [nfsd]
> > kernel: nfs4_get_vfs_file+0x634/0x958 [nfsd]
> > kernel: nfsd4_process_open2+0xa40/0x1a40 [nfsd]
> > kernel: nfsd4_open+0xa08/0xe80 [nfsd]
> > kernel: nfsd4_proc_compound+0xb8c/0x2130 [nfsd]
> > kernel: nfsd_dispatch+0x22c/0x718 [nfsd]
> > kernel: svc_process_common+0x8e8/0x1960 [sunrpc]
> > kernel: svc_process+0x3d4/0x7e0 [sunrpc]
> > kernel: svc_handle_xprt+0x828/0xe10 [sunrpc]
> > kernel: svc_recv+0x2cc/0x6a8 [sunrpc]
> > kernel: nfsd+0x270/0x400 [nfsd]
> > kernel: kthread+0x288/0x310
> > kernel: ret_from_fork+0x10/0x20
> > 
> > Proposing to have laundromat thread hold the state_lock over both
> > marking thru revoking the delegation as well as making free_stateid
> > acquire state_lock before accessing the list. Making sure that
> > revoke_delegation() (ie kernel_setlease(unlock)) is called for
> > every delegation that was revoked and added to the reaper list.
> > 
> > CC: stable@vger.kernel.org
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > 
> > --- I can't figure out the Fixes: tag. Laundromat's behaviour has
> > been like that forever. But the free_stateid bits wont apply before
> > the 1e3577a4521e ("SUNRPC: discard sv_refcnt, and
> > svc_get/svc_put").
> > But we used that fixes tag already with a previous fix for a
> > different
> > problem.
> > ---
> >  fs/nfsd/nfs4state.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 9c2b1d251ab3..c97907d7fb38 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -6605,13 +6605,13 @@ nfs4_laundromat(struct nfsd_net *nn)
> >   unhash_delegation_locked(dp, SC_STATUS_REVOKED);
> >   list_add(&dp->dl_recall_lru, &reaplist);
> >   }
> > - spin_unlock(&state_lock);
> >   while (!list_empty(&reaplist)) {
> >   dp = list_first_entry(&reaplist, struct nfs4_delegation,
> >   dl_recall_lru);
> >   list_del_init(&dp->dl_recall_lru);
> >   revoke_delegation(dp);
> >   }
> > + spin_unlock(&state_lock);
> 
> Code review suggests revoke_delegation() (and in particular,
> destroy_unhashed_deleg(), must not be called while holding
> state_lock().
> 
> 
> >   spin_lock(&nn->client_lock);
> >   while (!list_empty(&nn->close_lru)) {
> > @@ -7213,7 +7213,9 @@ nfsd4_free_stateid(struct svc_rqst *rqstp,
> > struct nfsd4_compound_state *cstate,
> >   if (s->sc_status & SC_STATUS_REVOKED) {
> >   spin_unlock(&s->sc_lock);
> >   dp = delegstateid(s);
> > + spin_lock(&state_lock);
> >   list_del_init(&dp->dl_recall_lru);
> > + spin_unlock(&state_lock);
> 
> Existing code is inconsistent about how manipulation of
> dl_recall_lru is protected. Most instances do use state_lock for
> this purpose, but a few, including this one, use cl->cl_lock. Does
> the other instance using cl_lock need review and correction as well?
> 
> I'd prefer to see this fix make the protection of dl_recall_lru
> consistent everywhere.

The problem appears to be that the same list entry field dp-
>dl_recall_lru is being reused for several completely different lists:
 * clp->cl_revoked
 * nn->del_recall_lru
and the occasional private list.

It looks as if the intention is to protect clp->cl_revoked with a
spinlock that is local to the client, whereas nn->del_recall_lru is
protected by a global lock (i.e. state_lock).

In most cases where unhash_delegation_locked() is being called, then it
is obvious that the dl_recall_lru field is assigned to the nn-
>del_recall_lru list, and so state_lock needs to be held.

However the two calls to destroy_delegation() don't appear to guarantee
anything w.r.t. what dl_recall_lru is being used for. So perhaps those
calls ought to grab dp->dl_stid.sc_client->cl_lock before calling
unhash_delegation_locked()?

The other thing is the issue of dl_recall_lru being put on private
lists (including in nfs4_laundromat(), nfs4_state_shutdown_net() and
__destroy_client()). I'd suggest that practice is only safe if the call
to unhash_delegation_locked() is actually successful. Otherwise, there
is a danger of a race where the delegation can be freed while it is on
the private list.

> 
> 
> >   spin_unlock(&cl->cl_lock);
> >   nfs4_put_stid(s);
> >   ret = nfs_ok;
> > -- 
> > 2.43.5
> > 
> 

-- 
Trond Myklebust Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com