[PATCH net 0/3] mptcp: sched: fix some lock issues

[PATCH net 0/3] mptcp: sched: fix some lock issues

[Matthieu Baerts (NGI0)]

Two small fixes related to the MPTCP packets scheduler:

- Patch 1: add missing rcu_read_(un)lock(). A fix for >= 6.6.

- Patch 2: remove unneeded lock when listing packets schedulers. A fix
  for >= 6.10.

And some modifications in the MPTCP selftests:

- Patch 3: a small addition to the MPTCP selftests to cover more code.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (3):
      mptcp: init: protect sched with rcu_read_lock
      mptcp: remove unneeded lock when listing scheds
      selftests: mptcp: list sysctl data

 net/mptcp/protocol.c                               | 2 ++
 net/mptcp/sched.c                                  | 2 --
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 9 +++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)
---
base-commit: 3b05b9c36ddd01338e1352588f2ec1ea23f97d43
change-id: 20241021-net-mptcp-sched-lock-10dfc75d1e00

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

[PATCH net 1/3] 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")
Cc: stable@vger.kernel.org
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/523
Reviewed-by: Geliang Tang <geliang@kernel.org>
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 6d0e201c3eb26aa6ca0ff27e5a65cb6f911012f2..d263091659e076587bc3406dfdcb4409adb3247e 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 net 2/3] mptcp: remove unneeded lock when listing scheds

[Matthieu Baerts (NGI0)]

mptcp_get_available_schedulers() needs to iterate over the schedulers'
list only to read the names: it doesn't modify anything there.

In this case, it is enough to hold the RCU read lock, no need to combine
this with the associated spin lock.

Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers")
Cc: stable@vger.kernel.org
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/sched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 78ed508ebc1b8dd9f0e020cca1bdd86f24f0afeb..df7dbcfa3b71370cc4d7e4e4f16cc1e41a50dddf 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -60,7 +60,6 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen)
 	size_t offs = 0;
 
 	rcu_read_lock();
-	spin_lock(&mptcp_sched_list_lock);
 	list_for_each_entry_rcu(sched, &mptcp_sched_list, list) {
 		offs += snprintf(buf + offs, maxlen - offs,
 				 "%s%s",
@@ -69,7 +68,6 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen)
 		if (WARN_ON_ONCE(offs >= maxlen))
 			break;
 	}
-	spin_unlock(&mptcp_sched_list_lock);
 	rcu_read_unlock();
 }
 

-- 
2.45.2

[PATCH net 3/3] selftests: mptcp: list sysctl data

[Matthieu Baerts (NGI0)]

Listing all the values linked to the MPTCP sysctl knobs was not
exercised in MPTCP test suite.

Let's do that to avoid any regressions, but also to have a kernel with a
debug kconfig verifying more assumptions. For the moment, we are not
interested by the output, only to avoid crashes and warnings.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 57325d57e4c6e3653019db2de09620d692143683..b48b4e56826a9cfdb3501242b707ae2ebe29b220 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -259,6 +259,15 @@ check_mptcp_disabled()
 	mptcp_lib_ns_init disabled_ns
 
 	print_larger_title "New MPTCP socket can be blocked via sysctl"
+
+	# mainly to cover more code
+	if ! ip netns exec ${disabled_ns} sysctl net.mptcp >/dev/null; then
+		mptcp_lib_pr_fail "not able to list net.mptcp sysctl knobs"
+		mptcp_lib_result_fail "not able to list net.mptcp sysctl knobs"
+		ret=${KSFT_FAIL}
+		return 1
+	fi
+
 	# net.mptcp.enabled should be enabled by default
 	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
 		mptcp_lib_pr_fail "net.mptcp.enabled sysctl is not 1 by default"

-- 
2.45.2

Re: [PATCH net 3/3] selftests: mptcp: list sysctl data

[Simon Horman]

On Mon, Oct 21, 2024 at 12:25:28PM +0200, Matthieu Baerts (NGI0) wrote:
> Listing all the values linked to the MPTCP sysctl knobs was not
> exercised in MPTCP test suite.
> 
> Let's do that to avoid any regressions, but also to have a kernel with a
> debug kconfig verifying more assumptions. For the moment, we are not
> interested by the output, only to avoid crashes and warnings.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

I am assuming that we are ok with expanding test coverage via net,
which FWIIW, does seem reasonable to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

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

[Simon Horman]

On Mon, Oct 21, 2024 at 12:25:26PM +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")
> Cc: stable@vger.kernel.org
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/523
> Reviewed-by: Geliang Tang <geliang@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 2/3] mptcp: remove unneeded lock when listing scheds

[Simon Horman]

On Mon, Oct 21, 2024 at 12:25:27PM +0200, Matthieu Baerts (NGI0) wrote:
> mptcp_get_available_schedulers() needs to iterate over the schedulers'
> list only to read the names: it doesn't modify anything there.
> 
> In this case, it is enough to hold the RCU read lock, no need to combine
> this with the associated spin lock.
> 
> Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers")
> Cc: stable@vger.kernel.org
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Reviewed-by: Geliang Tang <geliang@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

I do wonder if it would be more appropriate to route this via net-next
(without a fixes tag) rather than via net. But either way this looks good
to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH net 2/3] mptcp: remove unneeded lock when listing scheds

[Matthieu Baerts]

Hi Simon,

Thank you for the reviews!

On 23/10/2024 14:21, Simon Horman wrote:
> On Mon, Oct 21, 2024 at 12:25:27PM +0200, Matthieu Baerts (NGI0) wrote:
>> mptcp_get_available_schedulers() needs to iterate over the schedulers'
>> list only to read the names: it doesn't modify anything there.
>>
>> In this case, it is enough to hold the RCU read lock, no need to combine
>> this with the associated spin lock.
>>
>> Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers")
>> Cc: stable@vger.kernel.org
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>> Reviewed-by: Geliang Tang <geliang@kernel.org>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> I do wonder if it would be more appropriate to route this via net-next
> (without a fixes tag) rather than via net. But either way this looks good
> to me.
Good point. On one hand, I marked it as a fix, because when working on
the patch 1/3, we noticed these spin_(un)lock() were not supposed to be
there in the first place. On the other hand, even it's fixing a small
performance issue, it is not fixing a regression.

I think it is easier to route this via -net, but I'm fine if it is
applied in net-next.

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

Re: [PATCH net 2/3] mptcp: remove unneeded lock when listing scheds

[Simon Horman]

On Wed, Oct 23, 2024 at 04:13:36PM +0200, Matthieu Baerts wrote:
> Hi Simon,
> 
> Thank you for the reviews!
> 
> On 23/10/2024 14:21, Simon Horman wrote:
> > On Mon, Oct 21, 2024 at 12:25:27PM +0200, Matthieu Baerts (NGI0) wrote:
> >> mptcp_get_available_schedulers() needs to iterate over the schedulers'
> >> list only to read the names: it doesn't modify anything there.
> >>
> >> In this case, it is enough to hold the RCU read lock, no need to combine
> >> this with the associated spin lock.
> >>
> >> Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers")
> >> Cc: stable@vger.kernel.org
> >> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> >> Reviewed-by: Geliang Tang <geliang@kernel.org>
> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > 
> > I do wonder if it would be more appropriate to route this via net-next
> > (without a fixes tag) rather than via net. But either way this looks good
> > to me.
> Good point. On one hand, I marked it as a fix, because when working on
> the patch 1/3, we noticed these spin_(un)lock() were not supposed to be
> there in the first place. On the other hand, even it's fixing a small
> performance issue, it is not fixing a regression.
> 
> I think it is easier to route this via -net, but I'm fine if it is
> applied in net-next.

Understood. FTR, I don't feel strongly about this either way.

Re: [PATCH net 2/3] mptcp: remove unneeded lock when listing scheds

[Jakub Kicinski]

On Wed, 23 Oct 2024 16:13:36 +0200 Matthieu Baerts wrote:
> On 23/10/2024 14:21, Simon Horman wrote:
> > On Mon, Oct 21, 2024 at 12:25:27PM +0200, Matthieu Baerts (NGI0) wrote:  
> >> mptcp_get_available_schedulers() needs to iterate over the schedulers'
> >> list only to read the names: it doesn't modify anything there.
> >>
> >> In this case, it is enough to hold the RCU read lock, no need to combine
> >> this with the associated spin lock.
> >>
> >> Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers")
> >> Cc: stable@vger.kernel.org
> >> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> >> Reviewed-by: Geliang Tang <geliang@kernel.org>
> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>  
> > 
> > I do wonder if it would be more appropriate to route this via net-next
> > (without a fixes tag) rather than via net. But either way this looks good
> > to me.  
> Good point. On one hand, I marked it as a fix, because when working on
> the patch 1/3, we noticed these spin_(un)lock() were not supposed to be
> there in the first place. On the other hand, even it's fixing a small
> performance issue, it is not fixing a regression.
> 
> I think it is easier to route this via -net, but I'm fine if it is
> applied in net-next.

I agree with Simon's initial response. Let's not blur the lines.
Please re-queue for net-next, I'll apply the rest.

BTW thanks a lot for proactively fixing the CONFIG_PROVE_RCU_LIST
splats!

Re: [PATCH net 0/3] mptcp: sched: fix some lock issues

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 21 Oct 2024 12:25:25 +0200 you wrote:
> Two small fixes related to the MPTCP packets scheduler:
> 
> - Patch 1: add missing rcu_read_(un)lock(). A fix for >= 6.6.
> 
> - Patch 2: remove unneeded lock when listing packets schedulers. A fix
>   for >= 6.10.
> 
> [...]

Here is the summary with links:
  - [net,1/3] mptcp: init: protect sched with rcu_read_lock
    https://git.kernel.org/netdev/net/c/3deb12c788c3
  - [net,2/3] mptcp: remove unneeded lock when listing scheds
    (no matching commit)
  - [net,3/3] selftests: mptcp: list sysctl data
    https://git.kernel.org/netdev/net/c/5513dc1d8fec

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html