Re: [PATCH mptcp-net 2/2] Squash to "bpf: Add bpf_mptcp_sched_ops"

[Matthieu Baerts <matttbe@kernel.org>]

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
-- 
Sponsored by the NGI0 Core fund.