Hi Greg, Sasha,
Here are two MPTCP patches backports (patches 2-3/4), and one prerequisite (patch 1/4), that recently failed to apply to the 6.1 stable tree. They prevent some locking issues with MPTCP.
After having cherry-picked patch 1/4 -- a simple refactoring to make a function more generic -- patch 2/4 applied without any issue.
For patch 3/4, I had to resolve two simple function because two if-statements around the modified code have curly braces in v6.1, not later, see commit 976d302fb616 ("mptcp: deduplicate error paths on endpoint creation").
On top of that, patch 4/4 fixes MPTCP userspace PM selftest that has been recently broken due to a backport done in v6.1.8.
Signed-off-by: Matthieu Baerts matthieu.baerts@tessares.net --- Matthieu Baerts (2): mptcp: sockopt: make 'tcp_fastopen_connect' generic selftests: mptcp: userspace: fix v4-v6 test in v6.1
Paolo Abeni (2): mptcp: fix locking for setsockopt corner-case mptcp: fix locking for in-kernel listener creation
net/mptcp/pm_netlink.c | 10 ++++++---- net/mptcp/sockopt.c | 20 ++++++++++++++------ net/mptcp/subflow.c | 2 +- tools/testing/selftests/net/mptcp/userspace_pm.sh | 11 +++++++++++ 4 files changed, 32 insertions(+), 11 deletions(-) --- base-commit: 9012d1ebd3236e1d741ab4264f1d14e276c2e29f change-id: 20230214-upstream-stable-20230214-linux-6-1-12-rc1-mptcp-fixes-df24a5f41151
Best regards,
[ Upstream commit d3d429047cc66ff49780c93e4fccd9527723d385 ]
There are other socket options that need to act only on the first subflow, e.g. all TCP_FASTOPEN* socket options.
This is similar to the getsockopt version.
In the next commit, this new mptcp_setsockopt_first_sf_only() helper is used by other another option.
Reviewed-by: Mat Martineau mathew.j.martineau@linux.intel.com Signed-off-by: Matthieu Baerts matthieu.baerts@tessares.net Signed-off-by: Mat Martineau mathew.j.martineau@linux.intel.com Signed-off-by: Paolo Abeni pabeni@redhat.com Stable-dep-of: 21e43569685d ("mptcp: fix locking for setsockopt corner-case") Signed-off-by: Matthieu Baerts matthieu.baerts@tessares.net --- net/mptcp/sockopt.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index c7cb68c725b2..8d3b09d75c3a 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -769,17 +769,17 @@ static int mptcp_setsockopt_sol_tcp_defer(struct mptcp_sock *msk, sockptr_t optv return tcp_setsockopt(listener->sk, SOL_TCP, TCP_DEFER_ACCEPT, optval, optlen); }
-static int mptcp_setsockopt_sol_tcp_fastopen_connect(struct mptcp_sock *msk, sockptr_t optval, - unsigned int optlen) +static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int optname, + sockptr_t optval, unsigned int optlen) { struct socket *sock;
- /* Limit to first subflow */ + /* Limit to first subflow, before the connection establishment */ sock = __mptcp_nmpc_socket(msk); if (!sock) return -EINVAL;
- return tcp_setsockopt(sock->sk, SOL_TCP, TCP_FASTOPEN_CONNECT, optval, optlen); + return tcp_setsockopt(sock->sk, level, optname, optval, optlen); }
static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, @@ -811,7 +811,8 @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname, case TCP_DEFER_ACCEPT: return mptcp_setsockopt_sol_tcp_defer(msk, optval, optlen); case TCP_FASTOPEN_CONNECT: - return mptcp_setsockopt_sol_tcp_fastopen_connect(msk, optval, optlen); + return mptcp_setsockopt_first_sf_only(msk, SOL_TCP, optname, + optval, optlen); }
return -EOPNOTSUPP;
From: Paolo Abeni pabeni@redhat.com
[ Upstream commit 21e43569685de4ad773fb060c11a15f3fd5e7ac4 ]
We need to call the __mptcp_nmpc_socket(), and later subflow socket access under the msk socket lock, or e.g. a racing connect() could change the socket status under the hood, with unexpected results.
Fixes: 54635bd04701 ("mptcp: add TCP_FASTOPEN_CONNECT socket option") Cc: stable@vger.kernel.org Signed-off-by: Paolo Abeni pabeni@redhat.com Reviewed-by: Matthieu Baerts matthieu.baerts@tessares.net Signed-off-by: Matthieu Baerts matthieu.baerts@tessares.net Signed-off-by: David S. Miller davem@davemloft.net Signed-off-by: Matthieu Baerts matthieu.baerts@tessares.net --- net/mptcp/sockopt.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index 8d3b09d75c3a..696ba398d699 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -772,14 +772,21 @@ static int mptcp_setsockopt_sol_tcp_defer(struct mptcp_sock *msk, sockptr_t optv static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int optname, sockptr_t optval, unsigned int optlen) { + struct sock *sk = (struct sock *)msk; struct socket *sock; + int ret = -EINVAL;
/* Limit to first subflow, before the connection establishment */ + lock_sock(sk); sock = __mptcp_nmpc_socket(msk); if (!sock) - return -EINVAL; + goto unlock;
- return tcp_setsockopt(sock->sk, level, optname, optval, optlen); + ret = tcp_setsockopt(sock->sk, level, optname, optval, optlen); + +unlock: + release_sock(sk); + return ret; }
static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
From: Paolo Abeni pabeni@redhat.com
[ Upstream commit ad2171009d968104ccda9dc517f5a3ba891515db ]
For consistency, in mptcp_pm_nl_create_listen_socket(), we need to call the __mptcp_nmpc_socket() under the msk socket lock.
Note that as a side effect, mptcp_subflow_create_socket() needs a 'nested' lockdep annotation, as it will acquire the subflow (kernel) socket lock under the in-kernel listener msk socket lock.
The current lack of locking is almost harmless, because the relevant socket is not exposed to the user space, but in future we will add more complexity to the mentioned helper, let's play safe.
Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port") Cc: stable@vger.kernel.org Signed-off-by: Paolo Abeni pabeni@redhat.com Reviewed-by: Matthieu Baerts matthieu.baerts@tessares.net Signed-off-by: Matthieu Baerts matthieu.baerts@tessares.net Signed-off-by: David S. Miller davem@davemloft.net Signed-off-by: Matthieu Baerts matthieu.baerts@tessares.net --- net/mptcp/pm_netlink.c | 10 ++++++---- net/mptcp/subflow.c | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index 9813ed0fde9b..15253afc3fff 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -992,8 +992,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk, { int addrlen = sizeof(struct sockaddr_in); struct sockaddr_storage addr; - struct mptcp_sock *msk; struct socket *ssock; + struct sock *newsk; int backlog = 1024; int err;
@@ -1002,13 +1002,15 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk, if (err) return err;
- msk = mptcp_sk(entry->lsk->sk); - if (!msk) { + newsk = entry->lsk->sk; + if (!newsk) { err = -EINVAL; goto out; }
- ssock = __mptcp_nmpc_socket(msk); + lock_sock(newsk); + ssock = __mptcp_nmpc_socket(mptcp_sk(newsk)); + release_sock(newsk); if (!ssock) { err = -EINVAL; goto out; diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 929b0ee8b3d5..c4971bc42f60 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -1631,7 +1631,7 @@ int mptcp_subflow_create_socket(struct sock *sk, unsigned short family, if (err) return err;
- lock_sock(sf->sk); + lock_sock_nested(sf->sk, SINGLE_DEPTH_NESTING);
/* the newly created socket has to be in the same cgroup as its parent */ mptcp_attach_cgroup(sk, sf->sk);
The commit 4656d72c1efa ("selftests: mptcp: userspace: validate v4-v6 subflows mix") has been backported to v6.1.8 without any conflicts. But it looks like it was depending on a previous one:
commit 1cc94ac1af4b ("selftests: mptcp: make evts global in userspace_pm")
Without it, the test fails with:
./userspace_pm.sh: line 788: : No such file or directory # ADD_ADDR4 id:14 10.0.2.1 (ns1) => ns2, reuse port [FAIL] sed: can't read : No such file or directory
This dependence refactors the way the monitoring files are being created: only once for all the different sub-tests instead of per sub-test.
It is probably better to avoid backporting the refactoring. That is why the new sub-test has been adapted to work using the previous way that is still in place here in v6.1: the monitoring is started at the beginning of each sub-test and the created file is removed at the end.
Fixes: f59549814a64 ("selftests: mptcp: userspace: validate v4-v6 subflows mix") Signed-off-by: Matthieu Baerts matthieu.baerts@tessares.net --- tools/testing/selftests/net/mptcp/userspace_pm.sh | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh index 0040e3bc7b16..ad6547c79b83 100755 --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh @@ -778,6 +778,14 @@ test_subflows()
test_subflows_v4_v6_mix() { + local client_evts + client_evts=$(mktemp) + # Capture events on the network namespace running the client + :>"$client_evts" + ip netns exec "$ns2" ./pm_nl_ctl events >> "$client_evts" 2>&1 & + evts_pid=$! + sleep 0.5 + # Attempt to add a listener at 10.0.2.1:<subflow-port> ip netns exec "$ns1" ./pm_nl_ctl listen 10.0.2.1\ $app6_port > /dev/null 2>&1 & @@ -820,6 +828,9 @@ test_subflows_v4_v6_mix() ip netns exec "$ns1" ./pm_nl_ctl rem id $server_addr_id token\ "$server6_token" > /dev/null 2>&1 sleep 0.5 + + kill_wait $evts_pid + rm -f "$client_evts" }
test_prio()
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.' Subject: [PATCH 6.1 4/4] selftests: mptcp: userspace: fix v4-v6 test in v6.1 Link: https://lore.kernel.org/stable/20230214-upstream-stable-20230214-linux-6-1-1...
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
Hey Matthieu,
I've ended up doing something slightly different:
On Tue, Feb 14, 2023 at 05:05:06PM +0100, Matthieu Baerts wrote:
Hi Greg, Sasha,
Here are two MPTCP patches backports (patches 2-3/4), and one prerequisite (patch 1/4), that recently failed to apply to the 6.1 stable tree. They prevent some locking issues with MPTCP.
After having cherry-picked patch 1/4 -- a simple refactoring to make a function more generic -- patch 2/4 applied without any issue.
I did the same for 1+2.
For patch 3/4, I had to resolve two simple function because two if-statements around the modified code have curly braces in v6.1, not later, see commit 976d302fb616 ("mptcp: deduplicate error paths on endpoint creation").
Instead of resolving the conflict, I took 976d302fb616 ("mptcp: deduplicate error paths on endpoint creation".
On top of that, patch 4/4 fixes MPTCP userspace PM selftest that has been recently broken due to a backport done in v6.1.8.
I didn't have this one, I'll go and queue it up. Thanks!
linux-stable-mirror@lists.linaro.org
-
kernel test robot -
Matthieu Baerts -
Sasha Levin