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

[Geliang Tang <geliang@kernel.org>]

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;
>