| Message ID | 20241016-mptcp-sched-find-rcu-v1-2-5e9af4fbce11@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 8e293a58ad78ca2e512f4352e6ba49580e76001f |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: "fix suspicious RCU usage" warnings | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | warning | total: 0 errors, 3 warnings, 0 checks, 22 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
On Wed, 2024-10-16 at 21:05 +0200, Matthieu Baerts (NGI0) wrote: > Similar to the previous commit, this splat can be seen: > > ============================= > WARNING: suspicious RCU usage > 6.12.0-rc2+ #1 Tainted: G OE > ----------------------------- > net/mptcp/sched.c:44 RCU-list traversed in non-reader section!! > > other info that might help us debug this: > > rcu_scheduler_active = 2, debug_locks = 1 > 1 lock held by test_progs/323: > ffff888007e16a40 (&st_map->lock){+.+.}-{3:3}, at: > bpf_struct_ops_map_update_elem (kernel/bpf/bpf_struct_ops.c:632) > > stack backtrace: > CPU: 0 UID: 0 PID: 323 Comm: test_progs Tainted: G > OE 6.12.0-rc2+ #1 > Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE > Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 > Call Trace: > <TASK> > dump_stack_lvl (lib/dump_stack.c:123) > lockdep_rcu_suspicious (kernel/locking/lockdep.c:6822) > mptcp_sched_find (net/mptcp/sched.c:44 (discriminator 7)) > bpf_mptcp_sched_init_member (net/mptcp/bpf.c:128 > net/mptcp/bpf.c:109) > ? btf_type_resolve_ptr (include/linux/btf.h:252 > kernel/bpf/btf.c:637) > bpf_struct_ops_map_update_elem (kernel/bpf/bpf_struct_ops.c:658) > ? __might_fault (mm/memory.c:6700 (discriminator 5) > mm/memory.c:6693 (discriminator 5)) > ? __pfx_bpf_struct_ops_map_update_elem > (kernel/bpf/bpf_struct_ops.c:591) > ? __pfx___might_resched (kernel/sched/core.c:8593) > ? kasan_save_track (arch/x86/include/asm/current.h:49 > (discriminator 1) mm/kasan/common.c:60 (discriminator 1) > mm/kasan/common.c:69 (discriminator 1)) > bpf_map_update_value (kernel/bpf/syscall.c:169) > map_update_elem (kernel/bpf/syscall.c:1627) > ? __pfx_map_update_elem (kernel/bpf/syscall.c:1586) > __sys_bpf (kernel/bpf/syscall.c:5622) > ? __pfx___sys_bpf (kernel/bpf/syscall.c:5596) > __x64_sys_bpf (kernel/bpf/syscall.c:5739) > ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:4347 > kernel/locking/lockdep.c:4406) > do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) > arch/x86/entry/common.c:83 (discriminator 1)) > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) > > Also similar to the previous commit, this can be fixed by adding the > missing rcu_read_lock(). > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > --- > net/mptcp/bpf.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c > index > 6414824402e6449ba01efb9093b2293232a67915..a9d6b5b939a2631f17a468ee6ba > 4867dc33dda63 100644 > --- a/net/mptcp/bpf.c > +++ b/net/mptcp/bpf.c > @@ -113,6 +113,7 @@ static int bpf_mptcp_sched_init_member(const > struct btf_type *t, > const struct mptcp_sched_ops *usched; > struct mptcp_sched_ops *sched; > u32 moff; > + int ret; > > usched = (const struct mptcp_sched_ops *)udata; > sched = (struct mptcp_sched_ops *)kdata; > @@ -123,9 +124,12 @@ static int bpf_mptcp_sched_init_member(const > struct btf_type *t, > if (bpf_obj_name_cpy(sched->name, usched->name, > sizeof(sched->name)) <= 0) > return -EINVAL; > - if (mptcp_sched_find(usched->name)) > - return -EEXIST; This part of mptcp_sched_find() code comes from bpf_tcp_ca_init_member, but it was recently deleted by commit 68b04864ca42 ("bpf: Create links for BPF struct_ops maps."). --- a/net/ipv4/bpf_tcp_ca.c +++ b/net/ipv4/bpf_tcp_ca.c @@ -239,8 +239,6 @@ static int bpf_tcp_ca_init_member(const struct btf_type *t, if (bpf_obj_name_cpy(tcp_ca->name, utcp_ca->name, sizeof(tcp_ca->name)) <= 0) return -EINVAL; - if (tcp_ca_find(utcp_ca->name)) - return -EEXIST; return 1; } So we should also delete this part directly instead of adding rcu_read_lock. .validate interface is added in bpf_struct_ops by commit 68b04864ca42, I'll implement it in both mptcp_sched_ops and mptcp_pm_ops too. Thanks, -Geliang > - return 1; > + > + rcu_read_lock(); > + ret = mptcp_sched_find(usched->name) ? -EEXIST : 1; > + rcu_read_unlock(); > + > + return ret; > } > > return 0; >
Hi Geliang, Thank you for the review! On 17/10/2024 03:29, Geliang Tang wrote: > On Wed, 2024-10-16 at 21:05 +0200, Matthieu Baerts (NGI0) wrote: >> Similar to the previous commit, this splat can be seen: >> >> ============================= >> WARNING: suspicious RCU usage >> 6.12.0-rc2+ #1 Tainted: G OE >> ----------------------------- >> net/mptcp/sched.c:44 RCU-list traversed in non-reader section!! >> >> other info that might help us debug this: >> >> rcu_scheduler_active = 2, debug_locks = 1 >> 1 lock held by test_progs/323: >> ffff888007e16a40 (&st_map->lock){+.+.}-{3:3}, at: >> bpf_struct_ops_map_update_elem (kernel/bpf/bpf_struct_ops.c:632) >> >> stack backtrace: >> CPU: 0 UID: 0 PID: 323 Comm: test_progs Tainted: G >> OE 6.12.0-rc2+ #1 >> Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE >> Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 >> Call Trace: >> <TASK> >> dump_stack_lvl (lib/dump_stack.c:123) >> lockdep_rcu_suspicious (kernel/locking/lockdep.c:6822) >> mptcp_sched_find (net/mptcp/sched.c:44 (discriminator 7)) >> bpf_mptcp_sched_init_member (net/mptcp/bpf.c:128 >> net/mptcp/bpf.c:109) >> ? btf_type_resolve_ptr (include/linux/btf.h:252 >> kernel/bpf/btf.c:637) >> bpf_struct_ops_map_update_elem (kernel/bpf/bpf_struct_ops.c:658) >> ? __might_fault (mm/memory.c:6700 (discriminator 5) >> mm/memory.c:6693 (discriminator 5)) >> ? __pfx_bpf_struct_ops_map_update_elem >> (kernel/bpf/bpf_struct_ops.c:591) >> ? __pfx___might_resched (kernel/sched/core.c:8593) >> ? kasan_save_track (arch/x86/include/asm/current.h:49 >> (discriminator 1) mm/kasan/common.c:60 (discriminator 1) >> mm/kasan/common.c:69 (discriminator 1)) >> bpf_map_update_value (kernel/bpf/syscall.c:169) >> map_update_elem (kernel/bpf/syscall.c:1627) >> ? __pfx_map_update_elem (kernel/bpf/syscall.c:1586) >> __sys_bpf (kernel/bpf/syscall.c:5622) >> ? __pfx___sys_bpf (kernel/bpf/syscall.c:5596) >> __x64_sys_bpf (kernel/bpf/syscall.c:5739) >> ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:4347 >> kernel/locking/lockdep.c:4406) >> do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) >> arch/x86/entry/common.c:83 (discriminator 1)) >> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) >> >> Also similar to the previous commit, this can be fixed by adding the >> missing rcu_read_lock(). >> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >> --- >> net/mptcp/bpf.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c >> index >> 6414824402e6449ba01efb9093b2293232a67915..a9d6b5b939a2631f17a468ee6ba >> 4867dc33dda63 100644 >> --- a/net/mptcp/bpf.c >> +++ b/net/mptcp/bpf.c >> @@ -113,6 +113,7 @@ static int bpf_mptcp_sched_init_member(const >> struct btf_type *t, >> const struct mptcp_sched_ops *usched; >> struct mptcp_sched_ops *sched; >> u32 moff; >> + int ret; >> >> usched = (const struct mptcp_sched_ops *)udata; >> sched = (struct mptcp_sched_ops *)kdata; >> @@ -123,9 +124,12 @@ static int bpf_mptcp_sched_init_member(const >> struct btf_type *t, >> if (bpf_obj_name_cpy(sched->name, usched->name, >> sizeof(sched->name)) <= 0) >> return -EINVAL; >> - if (mptcp_sched_find(usched->name)) >> - return -EEXIST; > > This part of mptcp_sched_find() code comes from bpf_tcp_ca_init_member, When big chunks of code are copied from somewhere else in the kernel, do you mind adding a comment on top of the new code: this would help reviewers, but also developers and maintainers later when there are some adaptations to do, e.g. to stay in sync. In case of issue, we don't lose time trying to understand what's wrong on our side if we can quickly compare to another part of the kernel doing something very similar. > but it was recently deleted by commit 68b04864ca42 ("bpf: Create links > for BPF struct_ops maps."). > > --- a/net/ipv4/bpf_tcp_ca.c > +++ b/net/ipv4/bpf_tcp_ca.c > @@ -239,8 +239,6 @@ static int bpf_tcp_ca_init_member(const struct > btf_type *t, > if (bpf_obj_name_cpy(tcp_ca->name, utcp_ca->name, > sizeof(tcp_ca->name)) <= 0) > return -EINVAL; > - if (tcp_ca_find(utcp_ca->name)) > - return -EEXIST; > return 1; > } > > So we should also delete this part directly instead of adding > rcu_read_lock. > > .validate interface is added in bpf_struct_ops by commit 68b04864ca42, > I'll implement it in both mptcp_sched_ops and mptcp_pm_ops too. Indeed, it looks better to implement this ".validate" interface. If that's OK, I'm planning to add the rcu_read_lock() now to avoid the warning I mentioned. We can move this to the validation part later on. Cheers, Matt
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c index 6414824402e6449ba01efb9093b2293232a67915..a9d6b5b939a2631f17a468ee6ba4867dc33dda63 100644 --- a/net/mptcp/bpf.c +++ b/net/mptcp/bpf.c @@ -113,6 +113,7 @@ static int bpf_mptcp_sched_init_member(const struct btf_type *t, const struct mptcp_sched_ops *usched; struct mptcp_sched_ops *sched; u32 moff; + int ret; usched = (const struct mptcp_sched_ops *)udata; sched = (struct mptcp_sched_ops *)kdata; @@ -123,9 +124,12 @@ static int bpf_mptcp_sched_init_member(const struct btf_type *t, if (bpf_obj_name_cpy(sched->name, usched->name, sizeof(sched->name)) <= 0) return -EINVAL; - if (mptcp_sched_find(usched->name)) - return -EEXIST; - return 1; + + rcu_read_lock(); + ret = mptcp_sched_find(usched->name) ? -EEXIST : 1; + rcu_read_unlock(); + + return ret; } return 0;
Similar to the previous commit, this splat can be seen: ============================= WARNING: suspicious RCU usage 6.12.0-rc2+ #1 Tainted: G OE ----------------------------- net/mptcp/sched.c:44 RCU-list traversed in non-reader section!! other info that might help us debug this: rcu_scheduler_active = 2, debug_locks = 1 1 lock held by test_progs/323: ffff888007e16a40 (&st_map->lock){+.+.}-{3:3}, at: bpf_struct_ops_map_update_elem (kernel/bpf/bpf_struct_ops.c:632) stack backtrace: CPU: 0 UID: 0 PID: 323 Comm: test_progs Tainted: G OE 6.12.0-rc2+ #1 Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 Call Trace: <TASK> dump_stack_lvl (lib/dump_stack.c:123) lockdep_rcu_suspicious (kernel/locking/lockdep.c:6822) mptcp_sched_find (net/mptcp/sched.c:44 (discriminator 7)) bpf_mptcp_sched_init_member (net/mptcp/bpf.c:128 net/mptcp/bpf.c:109) ? btf_type_resolve_ptr (include/linux/btf.h:252 kernel/bpf/btf.c:637) bpf_struct_ops_map_update_elem (kernel/bpf/bpf_struct_ops.c:658) ? __might_fault (mm/memory.c:6700 (discriminator 5) mm/memory.c:6693 (discriminator 5)) ? __pfx_bpf_struct_ops_map_update_elem (kernel/bpf/bpf_struct_ops.c:591) ? __pfx___might_resched (kernel/sched/core.c:8593) ? kasan_save_track (arch/x86/include/asm/current.h:49 (discriminator 1) mm/kasan/common.c:60 (discriminator 1) mm/kasan/common.c:69 (discriminator 1)) bpf_map_update_value (kernel/bpf/syscall.c:169) map_update_elem (kernel/bpf/syscall.c:1627) ? __pfx_map_update_elem (kernel/bpf/syscall.c:1586) __sys_bpf (kernel/bpf/syscall.c:5622) ? __pfx___sys_bpf (kernel/bpf/syscall.c:5596) __x64_sys_bpf (kernel/bpf/syscall.c:5739) ? lockdep_hardirqs_on_prepare (kernel/locking/lockdep.c:4347 kernel/locking/lockdep.c:4406) do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) arch/x86/entry/common.c:83 (discriminator 1)) entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) Also similar to the previous commit, this can be fixed by adding the missing rcu_read_lock(). Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/bpf.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)