[PATCH mptcp-net 0/2] mptcp: "fix suspicious RCU usage" warnings

[PATCH mptcp-net 0/2] mptcp: "fix suspicious RCU usage" warnings

[Matthieu Baerts (NGI0)]

Enabling PROVE_RCU_LIST (and RCU_EXPERT) shows list_for_each_entry_rcu()
from mptcp_sched_find() not being used with RCU read lock held.

The first patch is a fix for -net. The other one is for a commit that is
only in our tree.

Link: https://lore.kernel.org/20241016011144.3058445-1-kuba@kernel.org
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (2):
      mptcp: init: protect sched with rcu_read_lock
      Squash to "bpf: Add bpf_mptcp_sched_ops"

 net/mptcp/bpf.c      | 10 +++++++---
 net/mptcp/protocol.c |  2 ++
 2 files changed, 9 insertions(+), 3 deletions(-)
---
base-commit: baccd7675477b1db387aa71b48c3312b5fb67a5d
change-id: 20241016-mptcp-sched-find-rcu-649ce3399334

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-net 1/2] mptcp: init: protect sched with rcu_read_lock

[Matthieu Baerts (NGI0)]

Enabling CONFIG_PROVE_RCU_LIST with its dependence CONFIG_RCU_EXPERT
creates this splat when an MPTCP socket is created:

  =============================
  WARNING: suspicious RCU usage
  6.12.0-rc2+ #11 Not tainted
  -----------------------------
  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
  no locks held by mptcp_connect/176.

  stack backtrace:
  CPU: 0 UID: 0 PID: 176 Comm: mptcp_connect Not tainted 6.12.0-rc2+ #11
  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))
   mptcp_init_sock (net/mptcp/protocol.c:2867 (discriminator 1))
   ? sock_init_data_uid (arch/x86/include/asm/atomic.h:28)
   inet_create.part.0.constprop.0 (net/ipv4/af_inet.c:386)
   ? __sock_create (include/linux/rcupdate.h:347 (discriminator 1))
   __sock_create (net/socket.c:1576)
   __sys_socket (net/socket.c:1671)
   ? __pfx___sys_socket (net/socket.c:1712)
   ? do_user_addr_fault (arch/x86/mm/fault.c:1419 (discriminator 1))
   __x64_sys_socket (net/socket.c:1728)
   do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1))
   entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)

That's because when the socket is initialised, rcu_read_lock() is not
used despite the explicit comment written above the declaration of
mptcp_sched_find() in sched.c. Adding the missing lock/unlock avoids the
warning.

Fixes: 1730b2b2c5a5 ("mptcp: add sched in mptcp_sock")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/523
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index e420ce9bbfb6e0527ed3ce8cbe2a0990c6366d12..21bc3586c33e16471056fedf49ee044ba27731d9 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2864,8 +2864,10 @@ static int mptcp_init_sock(struct sock *sk)
 	if (unlikely(!net->mib.mptcp_statistics) && !mptcp_mib_alloc(net))
 		return -ENOMEM;
 
+	rcu_read_lock();
 	ret = mptcp_init_sched(mptcp_sk(sk),
 			       mptcp_sched_find(mptcp_get_scheduler(net)));
+	rcu_read_unlock();
 	if (ret)
 		return ret;
 

-- 
2.45.2

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

[Matthieu Baerts (NGI0)]

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

-- 
2.45.2

Re: [PATCH mptcp-net 0/2] mptcp: "fix suspicious RCU usage" warnings

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11372377607

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/443b0dde1d65
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=899900


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-net 1/2] mptcp: init: protect sched with rcu_read_lock

[Geliang Tang]

Hi Matt,

Thanks for this fix.

On Wed, 2024-10-16 at 21:05 +0200, Matthieu Baerts (NGI0) wrote:
> Enabling CONFIG_PROVE_RCU_LIST with its dependence CONFIG_RCU_EXPERT
> creates this splat when an MPTCP socket is created:
> 
>   =============================
>   WARNING: suspicious RCU usage
>   6.12.0-rc2+ #11 Not tainted
>   -----------------------------
>   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
>   no locks held by mptcp_connect/176.
> 
>   stack backtrace:
>   CPU: 0 UID: 0 PID: 176 Comm: mptcp_connect Not tainted 6.12.0-rc2+
> #11
>   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))
>    mptcp_init_sock (net/mptcp/protocol.c:2867 (discriminator 1))
>    ? sock_init_data_uid (arch/x86/include/asm/atomic.h:28)
>    inet_create.part.0.constprop.0 (net/ipv4/af_inet.c:386)
>    ? __sock_create (include/linux/rcupdate.h:347 (discriminator 1))
>    __sock_create (net/socket.c:1576)
>    __sys_socket (net/socket.c:1671)
>    ? __pfx___sys_socket (net/socket.c:1712)
>    ? do_user_addr_fault (arch/x86/mm/fault.c:1419 (discriminator 1))
>    __x64_sys_socket (net/socket.c:1728)
>    do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1))
>    entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
> 
> That's because when the socket is initialised, rcu_read_lock() is not
> used despite the explicit comment written above the declaration of
> mptcp_sched_find() in sched.c. Adding the missing lock/unlock avoids
> the
> warning.
> 
> Fixes: 1730b2b2c5a5 ("mptcp: add sched in mptcp_sock")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/523
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Reviewed-by: Geliang Tang <geliang@kernel.org>

Good catch!

Some code in tcp_ca_dst_init() uses rcu_read_lock() too:

        rcu_read_lock();
        ca = tcp_ca_find_key(ca_key);
        if (likely(ca && bpf_try_module_get(ca, ca->owner))) {
                bpf_module_put(...);
                icsk->icsk_ca_dst_locked = tcp_ca_dst_locked(dst);
                icsk->icsk_ca_ops = ca;
        }
        rcu_read_unlock();

I will also sync this part of the changes to BPF path manager code
which is under review.

-Geliang

> ---
>  net/mptcp/protocol.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index
> e420ce9bbfb6e0527ed3ce8cbe2a0990c6366d12..21bc3586c33e16471056fedf49e
> e044ba27731d9 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2864,8 +2864,10 @@ static int mptcp_init_sock(struct sock *sk)
>   if (unlikely(!net->mib.mptcp_statistics) && !mptcp_mib_alloc(net))
>   return -ENOMEM;
>  
> + rcu_read_lock();
>   ret = mptcp_init_sched(mptcp_sk(sk),
>          mptcp_sched_find(mptcp_get_scheduler(net)));
> + rcu_read_unlock();
>   if (ret)
>   return ret;
>  
> 

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

[Geliang Tang]

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

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

[Matthieu Baerts]

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.

Re: [PATCH mptcp-net 0/2] mptcp: "fix suspicious RCU usage" warnings

[Matthieu Baerts]

Hi Geliang,

On 16/10/2024 21:05, Matthieu Baerts (NGI0) wrote:
> Enabling PROVE_RCU_LIST (and RCU_EXPERT) shows list_for_each_entry_rcu()
> from mptcp_sched_find() not being used with RCU read lock held.
> 
> The first patch is a fix for -net. The other one is for a commit that is
> only in our tree.
> 
> Link: https://lore.kernel.org/20241016011144.3058445-1-kuba@kernel.org
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Matthieu Baerts (NGI0) (2):
>       mptcp: init: protect sched with rcu_read_lock
>       Squash to "bpf: Add bpf_mptcp_sched_ops"

Thank you for the review! I just applied these two patches in our tree:

New patches for t/upstream-net and t/upstream:
- e80dfc53fa3d: mptcp: init: protect sched with rcu_read_lock
- Results: 32fda8b8e0f1..4367da1fbf63 (export-net)
- 8e293a58ad78: "squashed" patch 2/2 in "bpf: Add bpf_mptcp_sched_ops"
- Results: baccd7675477..0004ac084daf (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/6b56229c81f9ee84435f808186d11e9bcb7ad6c3/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/e8bb62da034e057e6d778b8d7c8c0c9c5cd5d5ab/checks

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net 0/2] mptcp: "fix suspicious RCU usage" warnings

[Paolo Abeni]

Hi,

On 10/16/24 21:05, Matthieu Baerts (NGI0) wrote:
> Enabling PROVE_RCU_LIST (and RCU_EXPERT) shows list_for_each_entry_rcu()
> from mptcp_sched_find() not being used with RCU read lock held.
> 
> The first patch is a fix for -net. The other one is for a commit that is
> only in our tree.
> 
> Link: https://lore.kernel.org/20241016011144.3058445-1-kuba@kernel.org
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Matthieu Baerts (NGI0) (2):
>        mptcp: init: protect sched with rcu_read_lock
>        Squash to "bpf: Add bpf_mptcp_sched_ops"
> 
>   net/mptcp/bpf.c      | 10 +++++++---
>   net/mptcp/protocol.c |  2 ++
>   2 files changed, 9 insertions(+), 3 deletions(-)
> ---
> base-commit: baccd7675477b1db387aa71b48c3312b5fb67a5d
> change-id: 20241016-mptcp-sched-find-rcu-649ce3399334
> 
> Best regards,

Only slightly related, but mptcp_get_available_schedulers() currently 
acquires both the rcu and the sched_list lock. I think the latter is not 
needed and should be dropped.

Cheers,

Paolo

Re: [PATCH mptcp-net 0/2] mptcp: "fix suspicious RCU usage" warnings

[Matthieu Baerts]

Hi Paolo,

On 17/10/2024 11:38, Paolo Abeni wrote:
> On 10/16/24 21:05, Matthieu Baerts (NGI0) wrote:
>> Enabling PROVE_RCU_LIST (and RCU_EXPERT) shows list_for_each_entry_rcu()
>> from mptcp_sched_find() not being used with RCU read lock held.
>>
>> The first patch is a fix for -net. The other one is for a commit that is
>> only in our tree.
>>
>> Link: https://lore.kernel.org/20241016011144.3058445-1-kuba@kernel.org
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Matthieu Baerts (NGI0) (2):
>>        mptcp: init: protect sched with rcu_read_lock
>>        Squash to "bpf: Add bpf_mptcp_sched_ops"
>>
>>   net/mptcp/bpf.c      | 10 +++++++---
>>   net/mptcp/protocol.c |  2 ++
>>   2 files changed, 9 insertions(+), 3 deletions(-)
>> ---
>> base-commit: baccd7675477b1db387aa71b48c3312b5fb67a5d
>> change-id: 20241016-mptcp-sched-find-rcu-649ce3399334
>>
>> Best regards,
> 
> Only slightly related, but mptcp_get_available_schedulers() currently
> acquires both the rcu and the sched_list lock. I think the latter is not
> needed and should be dropped.

Good point, the list is not modified.
I can send a patch for that.

(sorry, I should not have applied these patches that quickly, I was busy
adding 'debug' support for the BPF tests in our CI, and I didn't think
it was too quick)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.