linux-kernel - Re: [PATCH v2] nfsd: put dl_stid if fail to queue dl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b96deb97447fe54722973928bb1be1d7dbfe2aa4.camel@kernel.org>
Date: Thu, 13 Feb 2025 09:26:43 -0500
From: Jeff Layton <jlayton@...nel.org>
To: Li Lingfeng <lilingfeng3@...wei.com>, chuck.lever@...cle.com,
 neilb@...e.de, 	okorniev@...hat.com, Dai.Ngo@...cle.com, tom@...pey.com,
 linux-nfs@...r.kernel.org, 	linux-kernel@...r.kernel.org
Cc: yukuai1@...weicloud.com, houtao1@...wei.com, yi.zhang@...wei.com, 
	yangerkun@...wei.com, lilingfeng@...weicloud.com
Subject: Re: [PATCH v2] nfsd: put dl_stid if fail to queue dl_recall

On Thu, 2025-02-13 at 22:42 +0800, Li Lingfeng wrote:
> Before calling nfsd4_run_cb to queue dl_recall to the callback_wq, we
> increment the reference count of dl_stid.
> We expect that after the corresponding work_struct is processed, the
> reference count of dl_stid will be decremented through the callback
> function nfsd4_cb_recall_release.
> However, if the call to nfsd4_run_cb fails, the incremented reference
> count of dl_stid will not be decremented correspondingly, leading to the
> following nfs4_stid leak:
> unreferenced object 0xffff88812067b578 (size 344):
>   comm "nfsd", pid 2761, jiffies 4295044002 (age 5541.241s)
>   hex dump (first 32 bytes):
>     01 00 00 00 6b 6b 6b 6b b8 02 c0 e2 81 88 ff ff  ....kkkk........
>     00 6b 6b 6b 6b 6b 6b 6b 00 00 00 00 ad 4e ad de  .kkkkkkk.....N..
>   backtrace:
>     kmem_cache_alloc+0x4b9/0x700
>     nfsd4_process_open1+0x34/0x300
>     nfsd4_open+0x2d1/0x9d0
>     nfsd4_proc_compound+0x7a2/0xe30
>     nfsd_dispatch+0x241/0x3e0
>     svc_process_common+0x5d3/0xcc0
>     svc_process+0x2a3/0x320
>     nfsd+0x180/0x2e0
>     kthread+0x199/0x1d0
>     ret_from_fork+0x30/0x50
>     ret_from_fork_asm+0x1b/0x30
> unreferenced object 0xffff8881499f4d28 (size 368):
>   comm "nfsd", pid 2761, jiffies 4295044005 (age 5541.239s)
>   hex dump (first 32 bytes):
>     01 00 00 00 00 00 00 00 30 4d 9f 49 81 88 ff ff  ........0M.I....
>     30 4d 9f 49 81 88 ff ff 20 00 00 00 01 00 00 00  0M.I.... .......
>   backtrace:
>     kmem_cache_alloc+0x4b9/0x700
>     nfs4_alloc_stid+0x29/0x210
>     alloc_init_deleg+0x92/0x2e0
>     nfs4_set_delegation+0x284/0xc00
>     nfs4_open_delegation+0x216/0x3f0
>     nfsd4_process_open2+0x2b3/0xee0
>     nfsd4_open+0x770/0x9d0
>     nfsd4_proc_compound+0x7a2/0xe30
>     nfsd_dispatch+0x241/0x3e0
>     svc_process_common+0x5d3/0xcc0
>     svc_process+0x2a3/0x320
>     nfsd+0x180/0x2e0
>     kthread+0x199/0x1d0
>     ret_from_fork+0x30/0x50
>     ret_from_fork_asm+0x1b/0x30
> Fix it by checking the result of nfsd4_run_cb and call nfs4_put_stid if
> fail to queue dl_recall.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Li Lingfeng <lilingfeng3@...wei.com>
> ---
> Changes in v2:
> - Add a comment to nfs4_free_deleg.
> ---
>  fs/nfsd/nfs4state.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 153eeea2c7c9..d442d61e2947 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1050,6 +1050,12 @@ static struct nfs4_ol_stateid * nfs4_alloc_open_stateid(struct nfs4_client *clp)
>  	return openlockstateid(stid);
>  }
>  
> +/*
> + * As the sc_free callback of deleg, this may be called by nfs4_put_stid
> + * in nfsd_break_one_deleg.
> + * Considering nfsd_break_one_deleg is called with the flc->flc_lock held,
> + * this function mustn't ever sleep.
> + */
>  static void nfs4_free_deleg(struct nfs4_stid *stid)
>  {
>  	struct nfs4_delegation *dp = delegstateid(stid);
> @@ -5414,6 +5420,7 @@ static const struct nfsd4_callback_ops nfsd4_cb_recall_ops = {
>  
>  static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
>  {
> +	bool queued;
>  	/*
>  	 * We're assuming the state code never drops its reference
>  	 * without first removing the lease.  Since we're in this lease
> @@ -5422,7 +5429,10 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
>  	 * we know it's safe to take a reference.
>  	 */
>  	refcount_inc(&dp->dl_stid.sc_count);
> -	WARN_ON_ONCE(!nfsd4_run_cb(&dp->dl_recall));
> +	queued = nfsd4_run_cb(&dp->dl_recall);
> +	WARN_ON_ONCE(!queued);
> +	if (!queued)
> +		nfs4_put_stid(&dp->dl_stid);
>  }
>  
>  /* Called from break_lease() with flc_lock held. */

Works for me for now. Long term, I think we need to consider how to
ensure that the sc_free for lock stateids doesn't sleep.

Reviewed-by: Jeff Layton <jlayton@...nel.org>