| Message ID | 20250213144220.156089-1-lilingfeng3@huawei.com (mailing list archive) |
|---|---|
| State | In Next |
| Delegated to: | Chuck Lever |
| Headers | show |
| Series | [v2] nfsd: put dl_stid if fail to queue dl_recall | expand |
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@huawei.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@kernel.org>
From: Chuck Lever <chuck.lever@oracle.com> On Thu, 13 Feb 2025 22:42:20 +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. > > [...] I replaced the Fixes: tag with Cc: stable. Applied to nfsd-testing, thanks! [1/1] nfsd: put dl_stid if fail to queue dl_recall commit: d520b70859e0fb6d62ca12eee601a3d6ff4a11b6 -- Chuck Lever
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. */
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@huawei.com> --- Changes in v2: - Add a comment to nfs4_free_deleg. --- fs/nfsd/nfs4state.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)