| Message ID | 20241024015520.1448198-1-yebin@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | svcrdma: fix miss destroy percpu_counter in svc_rdma_proc_init() | expand |
On Thu, 24 Oct 2024 09:55:20 +0800, Ye Bin wrote: > There's issue as follows: > RPC: Registered rdma transport module. > RPC: Registered rdma backchannel transport module. > RPC: Unregistered rdma transport module. > RPC: Unregistered rdma backchannel transport module. > BUG: unable to handle page fault for address: fffffbfff80c609a > PGD 123fee067 P4D 123fee067 PUD 123fea067 PMD 10c624067 PTE 0 > Oops: Oops: 0000 [#1] PREEMPT SMP KASAN NOPTI > RIP: 0010:percpu_counter_destroy_many+0xf7/0x2a0 > Call Trace: > <TASK> > __die+0x1f/0x70 > page_fault_oops+0x2cd/0x860 > spurious_kernel_fault+0x36/0x450 > do_kern_addr_fault+0xca/0x100 > exc_page_fault+0x128/0x150 > asm_exc_page_fault+0x26/0x30 > percpu_counter_destroy_many+0xf7/0x2a0 > mmdrop+0x209/0x350 > finish_task_switch.isra.0+0x481/0x840 > schedule_tail+0xe/0xd0 > ret_from_fork+0x23/0x80 > ret_from_fork_asm+0x1a/0x30 > </TASK> > > [...] Applied to nfsd-next for v6.13, thanks! [1/1] svcrdma: fix miss destroy percpu_counter in svc_rdma_proc_init() commit: cd0f8809d24b55390eab29a02a595641531e4390 -- Chuck Lever <chuck.lever@oracle.com>
On Thu, Oct 24, 2024 at 09:55:20AM +0800, Ye Bin wrote: > From: Ye Bin <yebin10@huawei.com> > > There's issue as follows: > RPC: Registered rdma transport module. > RPC: Registered rdma backchannel transport module. > RPC: Unregistered rdma transport module. > RPC: Unregistered rdma backchannel transport module. > BUG: unable to handle page fault for address: fffffbfff80c609a > PGD 123fee067 P4D 123fee067 PUD 123fea067 PMD 10c624067 PTE 0 > Oops: Oops: 0000 [#1] PREEMPT SMP KASAN NOPTI > RIP: 0010:percpu_counter_destroy_many+0xf7/0x2a0 > Call Trace: > <TASK> > __die+0x1f/0x70 > page_fault_oops+0x2cd/0x860 > spurious_kernel_fault+0x36/0x450 > do_kern_addr_fault+0xca/0x100 > exc_page_fault+0x128/0x150 > asm_exc_page_fault+0x26/0x30 > percpu_counter_destroy_many+0xf7/0x2a0 > mmdrop+0x209/0x350 > finish_task_switch.isra.0+0x481/0x840 > schedule_tail+0xe/0xd0 > ret_from_fork+0x23/0x80 > ret_from_fork_asm+0x1a/0x30 > </TASK> > > If register_sysctl() return NULL, then svc_rdma_proc_cleanup() will not > destroy the percpu counters which init in svc_rdma_proc_init(). > If CONFIG_HOTPLUG_CPU is enabled, residual nodes may be in the > 'percpu_counters' list. The above issue may occur once the module is > removed. If the CONFIG_HOTPLUG_CPU configuration is not enabled, memory > leakage occurs. > To solve above issue just destroy all percpu counters when > register_sysctl() return NULL. > > Fixes: 1e7e55731628 ("svcrdma: Restore read and write stats") > Fixes: 22df5a22462e ("svcrdma: Convert rdma_stat_sq_starve to a per-CPU counter") > Fixes: df971cd853c0 ("svcrdma: Convert rdma_stat_recv to a per-CPU counter") > Signed-off-by: Ye Bin <yebin10@huawei.com> > --- > net/sunrpc/xprtrdma/svc_rdma.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c > index 58ae6ec4f25b..11dff4d58195 100644 > --- a/net/sunrpc/xprtrdma/svc_rdma.c > +++ b/net/sunrpc/xprtrdma/svc_rdma.c > @@ -233,25 +233,33 @@ static int svc_rdma_proc_init(void) > > rc = percpu_counter_init(&svcrdma_stat_read, 0, GFP_KERNEL); > if (rc) > - goto out_err; > + goto err; > rc = percpu_counter_init(&svcrdma_stat_recv, 0, GFP_KERNEL); > if (rc) > - goto out_err; > + goto err_read; > rc = percpu_counter_init(&svcrdma_stat_sq_starve, 0, GFP_KERNEL); > if (rc) > - goto out_err; > + goto err_recv; > rc = percpu_counter_init(&svcrdma_stat_write, 0, GFP_KERNEL); > if (rc) > - goto out_err; > + goto err_sq; > > svcrdma_table_header = register_sysctl("sunrpc/svc_rdma", > svcrdma_parm_table); > + if (!svcrdma_table_header) Hi Ye Bin, Should rc be set to a negative error value here, as is the case for other error cases above? Flagged by Smatch. > + goto err_write; > + > return 0; > > -out_err: > +err_write: > + percpu_counter_destroy(&svcrdma_stat_write); > +err_sq: > percpu_counter_destroy(&svcrdma_stat_sq_starve); > +err_recv: > percpu_counter_destroy(&svcrdma_stat_recv); > +err_read: > percpu_counter_destroy(&svcrdma_stat_read); > +err: > return rc; > } > > -- > 2.34.1 >
On Fri, Nov 01, 2024 at 11:55:11AM +0000, Simon Horman wrote: > On Thu, Oct 24, 2024 at 09:55:20AM +0800, Ye Bin wrote: > > From: Ye Bin <yebin10@huawei.com> > > > > There's issue as follows: > > RPC: Registered rdma transport module. > > RPC: Registered rdma backchannel transport module. > > RPC: Unregistered rdma transport module. > > RPC: Unregistered rdma backchannel transport module. > > BUG: unable to handle page fault for address: fffffbfff80c609a > > PGD 123fee067 P4D 123fee067 PUD 123fea067 PMD 10c624067 PTE 0 > > Oops: Oops: 0000 [#1] PREEMPT SMP KASAN NOPTI > > RIP: 0010:percpu_counter_destroy_many+0xf7/0x2a0 > > Call Trace: > > <TASK> > > __die+0x1f/0x70 > > page_fault_oops+0x2cd/0x860 > > spurious_kernel_fault+0x36/0x450 > > do_kern_addr_fault+0xca/0x100 > > exc_page_fault+0x128/0x150 > > asm_exc_page_fault+0x26/0x30 > > percpu_counter_destroy_many+0xf7/0x2a0 > > mmdrop+0x209/0x350 > > finish_task_switch.isra.0+0x481/0x840 > > schedule_tail+0xe/0xd0 > > ret_from_fork+0x23/0x80 > > ret_from_fork_asm+0x1a/0x30 > > </TASK> > > > > If register_sysctl() return NULL, then svc_rdma_proc_cleanup() will not > > destroy the percpu counters which init in svc_rdma_proc_init(). > > If CONFIG_HOTPLUG_CPU is enabled, residual nodes may be in the > > 'percpu_counters' list. The above issue may occur once the module is > > removed. If the CONFIG_HOTPLUG_CPU configuration is not enabled, memory > > leakage occurs. > > To solve above issue just destroy all percpu counters when > > register_sysctl() return NULL. > > > > Fixes: 1e7e55731628 ("svcrdma: Restore read and write stats") > > Fixes: 22df5a22462e ("svcrdma: Convert rdma_stat_sq_starve to a per-CPU counter") > > Fixes: df971cd853c0 ("svcrdma: Convert rdma_stat_recv to a per-CPU counter") > > Signed-off-by: Ye Bin <yebin10@huawei.com> > > --- > > net/sunrpc/xprtrdma/svc_rdma.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c > > index 58ae6ec4f25b..11dff4d58195 100644 > > --- a/net/sunrpc/xprtrdma/svc_rdma.c > > +++ b/net/sunrpc/xprtrdma/svc_rdma.c > > @@ -233,25 +233,33 @@ static int svc_rdma_proc_init(void) > > > > rc = percpu_counter_init(&svcrdma_stat_read, 0, GFP_KERNEL); > > if (rc) > > - goto out_err; > > + goto err; > > rc = percpu_counter_init(&svcrdma_stat_recv, 0, GFP_KERNEL); > > if (rc) > > - goto out_err; > > + goto err_read; > > rc = percpu_counter_init(&svcrdma_stat_sq_starve, 0, GFP_KERNEL); > > if (rc) > > - goto out_err; > > + goto err_recv; > > rc = percpu_counter_init(&svcrdma_stat_write, 0, GFP_KERNEL); > > if (rc) > > - goto out_err; > > + goto err_sq; > > > > svcrdma_table_header = register_sysctl("sunrpc/svc_rdma", > > svcrdma_parm_table); > > + if (!svcrdma_table_header) > > Hi Ye Bin, > > Should rc be set to a negative error value here, > as is the case for other error cases above? > > Flagged by Smatch. I can add "rc = -ENOMEM;" to the applied patch. > > + goto err_write; > > + > > return 0; > > > > -out_err: > > +err_write: > > + percpu_counter_destroy(&svcrdma_stat_write); > > +err_sq: > > percpu_counter_destroy(&svcrdma_stat_sq_starve); > > +err_recv: > > percpu_counter_destroy(&svcrdma_stat_recv); > > +err_read: > > percpu_counter_destroy(&svcrdma_stat_read); > > +err: > > return rc; > > } > > > > -- > > 2.34.1 > >
On 2024/11/2 0:27, Chuck Lever wrote: > On Fri, Nov 01, 2024 at 11:55:11AM +0000, Simon Horman wrote: >> On Thu, Oct 24, 2024 at 09:55:20AM +0800, Ye Bin wrote: >>> From: Ye Bin <yebin10@huawei.com> >>> >>> There's issue as follows: >>> RPC: Registered rdma transport module. >>> RPC: Registered rdma backchannel transport module. >>> RPC: Unregistered rdma transport module. >>> RPC: Unregistered rdma backchannel transport module. >>> BUG: unable to handle page fault for address: fffffbfff80c609a >>> PGD 123fee067 P4D 123fee067 PUD 123fea067 PMD 10c624067 PTE 0 >>> Oops: Oops: 0000 [#1] PREEMPT SMP KASAN NOPTI >>> RIP: 0010:percpu_counter_destroy_many+0xf7/0x2a0 >>> Call Trace: >>> <TASK> >>> __die+0x1f/0x70 >>> page_fault_oops+0x2cd/0x860 >>> spurious_kernel_fault+0x36/0x450 >>> do_kern_addr_fault+0xca/0x100 >>> exc_page_fault+0x128/0x150 >>> asm_exc_page_fault+0x26/0x30 >>> percpu_counter_destroy_many+0xf7/0x2a0 >>> mmdrop+0x209/0x350 >>> finish_task_switch.isra.0+0x481/0x840 >>> schedule_tail+0xe/0xd0 >>> ret_from_fork+0x23/0x80 >>> ret_from_fork_asm+0x1a/0x30 >>> </TASK> >>> >>> If register_sysctl() return NULL, then svc_rdma_proc_cleanup() will not >>> destroy the percpu counters which init in svc_rdma_proc_init(). >>> If CONFIG_HOTPLUG_CPU is enabled, residual nodes may be in the >>> 'percpu_counters' list. The above issue may occur once the module is >>> removed. If the CONFIG_HOTPLUG_CPU configuration is not enabled, memory >>> leakage occurs. >>> To solve above issue just destroy all percpu counters when >>> register_sysctl() return NULL. >>> >>> Fixes: 1e7e55731628 ("svcrdma: Restore read and write stats") >>> Fixes: 22df5a22462e ("svcrdma: Convert rdma_stat_sq_starve to a per-CPU counter") >>> Fixes: df971cd853c0 ("svcrdma: Convert rdma_stat_recv to a per-CPU counter") >>> Signed-off-by: Ye Bin <yebin10@huawei.com> >>> --- >>> net/sunrpc/xprtrdma/svc_rdma.c | 18 +++++++++++++----- >>> 1 file changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c >>> index 58ae6ec4f25b..11dff4d58195 100644 >>> --- a/net/sunrpc/xprtrdma/svc_rdma.c >>> +++ b/net/sunrpc/xprtrdma/svc_rdma.c >>> @@ -233,25 +233,33 @@ static int svc_rdma_proc_init(void) >>> >>> rc = percpu_counter_init(&svcrdma_stat_read, 0, GFP_KERNEL); >>> if (rc) >>> - goto out_err; >>> + goto err; >>> rc = percpu_counter_init(&svcrdma_stat_recv, 0, GFP_KERNEL); >>> if (rc) >>> - goto out_err; >>> + goto err_read; >>> rc = percpu_counter_init(&svcrdma_stat_sq_starve, 0, GFP_KERNEL); >>> if (rc) >>> - goto out_err; >>> + goto err_recv; >>> rc = percpu_counter_init(&svcrdma_stat_write, 0, GFP_KERNEL); >>> if (rc) >>> - goto out_err; >>> + goto err_sq; >>> >>> svcrdma_table_header = register_sysctl("sunrpc/svc_rdma", >>> svcrdma_parm_table); >>> + if (!svcrdma_table_header) >> Hi Ye Bin, >> >> Should rc be set to a negative error value here, >> as is the case for other error cases above? >> >> Flagged by Smatch. > I can add "rc = -ENOMEM;" to the applied patch. > I'm sorry for the late reply. Thank you very much for your revision. >>> + goto err_write; >>> + >>> return 0; >>> >>> -out_err: >>> +err_write: >>> + percpu_counter_destroy(&svcrdma_stat_write); >>> +err_sq: >>> percpu_counter_destroy(&svcrdma_stat_sq_starve); >>> +err_recv: >>> percpu_counter_destroy(&svcrdma_stat_recv); >>> +err_read: >>> percpu_counter_destroy(&svcrdma_stat_read); >>> +err: >>> return rc; >>> } >>> >>> -- >>> 2.34.1 >>>
diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c index 58ae6ec4f25b..11dff4d58195 100644 --- a/net/sunrpc/xprtrdma/svc_rdma.c +++ b/net/sunrpc/xprtrdma/svc_rdma.c @@ -233,25 +233,33 @@ static int svc_rdma_proc_init(void) rc = percpu_counter_init(&svcrdma_stat_read, 0, GFP_KERNEL); if (rc) - goto out_err; + goto err; rc = percpu_counter_init(&svcrdma_stat_recv, 0, GFP_KERNEL); if (rc) - goto out_err; + goto err_read; rc = percpu_counter_init(&svcrdma_stat_sq_starve, 0, GFP_KERNEL); if (rc) - goto out_err; + goto err_recv; rc = percpu_counter_init(&svcrdma_stat_write, 0, GFP_KERNEL); if (rc) - goto out_err; + goto err_sq; svcrdma_table_header = register_sysctl("sunrpc/svc_rdma", svcrdma_parm_table); + if (!svcrdma_table_header) + goto err_write; + return 0; -out_err: +err_write: + percpu_counter_destroy(&svcrdma_stat_write); +err_sq: percpu_counter_destroy(&svcrdma_stat_sq_starve); +err_recv: percpu_counter_destroy(&svcrdma_stat_recv); +err_read: percpu_counter_destroy(&svcrdma_stat_read); +err: return rc; }