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

[Geliang Tang <geliang@kernel.org>]

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