* [PATCH mptcp-net v3 1/2] mptcp: Avoid acquiring PM lock for subflow priority changes
2022-06-29 22:53 [PATCH mptcp-net v3 0/2] Locking fixes for subflow flag changes Mat Martineau
@ 2022-06-29 22:53 ` Mat Martineau
2022-06-29 22:53 ` [PATCH mptcp-net v3 2/2] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Mat Martineau
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Mat Martineau @ 2022-06-29 22:53 UTC (permalink / raw)
To: mptcp, pabeni; +Cc: Mat Martineau
The in-kernel path manager code for changing subflow flags acquired both
the msk socket lock and the PM lock when possibly changing the "backup"
and "fullmesh" flags. mptcp_pm_nl_mp_prio_send_ack() does not access
anything protected by the PM lock, and it must release and reacquire
the PM lock.
By pushing the PM lock to where it is needed in mptcp_pm_nl_fullmesh(),
the lock is only acquired when the fullmesh flag is changed and the
backup flag code no longer has to release and reacquire the PM lock. The
change in locking context requires the MIB update to be modified - move
that to a better location instead.
This change also makes it possible to call
mptcp_pm_nl_mp_prio_send_ack() for the userspace PM commands without
manipulating the in-kernel PM lock.
v2: Move MIB call to handle change in atomic context.
v3: Build fixes
Fixes: 0f9f696a502e ("mptcp: add set_flags command in PM netlink")
Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
net/mptcp/options.c | 3 +++
net/mptcp/pm_netlink.c | 8 ++------
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index aead331866a0..bd8f0f425be4 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1584,6 +1584,9 @@ void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
*ptr++ = mptcp_option(MPTCPOPT_MP_PRIO,
TCPOLEN_MPTCP_PRIO,
opts->backup, TCPOPT_NOP);
+
+ MPTCP_INC_STATS(sock_net((const struct sock *)tp),
+ MPTCP_MIB_MPPRIOTX);
}
mp_capable_done:
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e099f2a12504..5ff93b73f33d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -727,7 +727,6 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
mptcp_for_each_subflow(msk, subflow) {
struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
- struct sock *sk = (struct sock *)msk;
struct mptcp_addr_info local;
local_address((struct sock_common *)ssk, &local);
@@ -739,12 +738,9 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
subflow->backup = bkup;
subflow->send_mp_prio = 1;
subflow->request_bkup = bkup;
- __MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIOTX);
- spin_unlock_bh(&msk->pm.lock);
pr_debug("send ack for mp_prio");
mptcp_subflow_send_ack(ssk);
- spin_lock_bh(&msk->pm.lock);
return 0;
}
@@ -1816,8 +1812,10 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
list.ids[list.nr++] = addr->id;
+ spin_lock_bh(&msk->pm.lock);
mptcp_pm_nl_rm_subflow_received(msk, &list);
mptcp_pm_create_subflow_or_signal_addr(msk);
+ spin_unlock_bh(&msk->pm.lock);
}
static int mptcp_nl_set_flags(struct net *net,
@@ -1835,12 +1833,10 @@ static int mptcp_nl_set_flags(struct net *net,
goto next;
lock_sock(sk);
- spin_lock_bh(&msk->pm.lock);
if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
ret = mptcp_pm_nl_mp_prio_send_ack(msk, addr, bkup);
if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
mptcp_pm_nl_fullmesh(msk, addr);
- spin_unlock_bh(&msk->pm.lock);
release_sock(sk);
next:
--
2.37.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH mptcp-net v3 2/2] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags
2022-06-29 22:53 [PATCH mptcp-net v3 0/2] Locking fixes for subflow flag changes Mat Martineau
2022-06-29 22:53 ` [PATCH mptcp-net v3 1/2] mptcp: Avoid acquiring PM lock for subflow priority changes Mat Martineau
@ 2022-06-29 22:53 ` Mat Martineau
2022-06-30 0:44 ` mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags: Tests Results MPTCP CI
2022-06-30 16:18 ` [PATCH mptcp-net v3 0/2] Locking fixes for subflow flag changes Paolo Abeni
2022-06-30 17:39 ` Matthieu Baerts
3 siblings, 1 reply; 6+ messages in thread
From: Mat Martineau @ 2022-06-29 22:53 UTC (permalink / raw)
To: mptcp, pabeni; +Cc: Mat Martineau
When setting up a subflow's flags for sending MP_PRIO MPTCP options, the
subflow socket lock was not held while reading and modifying several
struct members that are also read and modified in mptcp_write_options().
Acquire the subflow socket lock earlier and send the MP_PRIO ACK with
that lock already acquired. Add a new variant of the
mptcp_subflow_send_ack() helper to use with the subflow lock held.
v2: Squashed helper function commit
Fixes: 067065422fcd ("mptcp: add the outgoing MP_PRIO support")
Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
net/mptcp/pm_netlink.c | 5 ++++-
net/mptcp/protocol.c | 9 +++++++--
net/mptcp/protocol.h | 1 +
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 5ff93b73f33d..ca86c88f89e0 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -728,11 +728,13 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
mptcp_for_each_subflow(msk, subflow) {
struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
struct mptcp_addr_info local;
+ bool slow;
local_address((struct sock_common *)ssk, &local);
if (!mptcp_addresses_equal(&local, addr, addr->port))
continue;
+ slow = lock_sock_fast(ssk);
if (subflow->backup != bkup)
msk->last_snd = NULL;
subflow->backup = bkup;
@@ -740,7 +742,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
subflow->request_bkup = bkup;
pr_debug("send ack for mp_prio");
- mptcp_subflow_send_ack(ssk);
+ __mptcp_subflow_send_ack(ssk);
+ unlock_sock_fast(ssk, slow);
return 0;
}
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3efbae948707..b96a80c8e22a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -506,13 +506,18 @@ static inline bool tcp_can_send_ack(const struct sock *ssk)
(TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
}
+void __mptcp_subflow_send_ack(struct sock *ssk)
+{
+ if (tcp_can_send_ack(ssk))
+ tcp_send_ack(ssk);
+}
+
void mptcp_subflow_send_ack(struct sock *ssk)
{
bool slow;
slow = lock_sock_fast(ssk);
- if (tcp_can_send_ack(ssk))
- tcp_send_ack(ssk);
+ __mptcp_subflow_send_ack(ssk);
unlock_sock_fast(ssk, slow);
}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index c14d70c036d0..033c995772dc 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -607,6 +607,7 @@ void __init mptcp_subflow_init(void);
void mptcp_subflow_shutdown(struct sock *sk, struct sock *ssk, int how);
void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
struct mptcp_subflow_context *subflow);
+void __mptcp_subflow_send_ack(struct sock *ssk);
void mptcp_subflow_send_ack(struct sock *ssk);
void mptcp_subflow_reset(struct sock *ssk);
void mptcp_subflow_queue_clean(struct sock *ssk);
--
2.37.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH mptcp-net v3 0/2] Locking fixes for subflow flag changes
2022-06-29 22:53 [PATCH mptcp-net v3 0/2] Locking fixes for subflow flag changes Mat Martineau
2022-06-29 22:53 ` [PATCH mptcp-net v3 1/2] mptcp: Avoid acquiring PM lock for subflow priority changes Mat Martineau
2022-06-29 22:53 ` [PATCH mptcp-net v3 2/2] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Mat Martineau
@ 2022-06-30 16:18 ` Paolo Abeni
2022-06-30 17:39 ` Matthieu Baerts
3 siblings, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2022-06-30 16:18 UTC (permalink / raw)
To: Mat Martineau, mptcp
On Wed, 2022-06-29 at 15:53 -0700, Mat Martineau wrote:
> When working with Kishen on the userspace PM, the locking code in
> mptcp_pm_nl_mp_prio_send_ack() caught my attention. The
> release-and-reacquire of the PM lock in that function was requiring the
> userspace PM code to grab that lock, even though nothing protected by
> that lock was being accessed. Patch 1 addresses this.
>
> While investigating that issue, I realized that
> mptcp_pm_nl_mp_prio_send_ack() was not holding the subflow socket lock
> while modifying several parts of the subflow context. Patch 2 addresses
> this, and is not a drastic change since the subflow lock was being
> acquired anyway in mptcp_subflow_send_ack().
>
> v2->v3: Fix build error
>
> v1->v2: Based on Paolo's feedback, fix MIB issue with atomic context in
> patch 1 (tested with CONFIG_PREEMPT), and squash patches 2 & 3.
>
>
> Mat Martineau (2):
> mptcp: Avoid acquiring PM lock for subflow priority changes
> mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags
>
> net/mptcp/options.c | 3 +++
> net/mptcp/pm_netlink.c | 13 ++++++-------
> net/mptcp/protocol.c | 9 +++++++--
> net/mptcp/protocol.h | 1 +
> 4 files changed, 17 insertions(+), 9 deletions(-)
LGTM! for the series:
Acked-by: Paolo Abeni <pabeni@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-net v3 0/2] Locking fixes for subflow flag changes
2022-06-29 22:53 [PATCH mptcp-net v3 0/2] Locking fixes for subflow flag changes Mat Martineau
` (2 preceding siblings ...)
2022-06-30 16:18 ` [PATCH mptcp-net v3 0/2] Locking fixes for subflow flag changes Paolo Abeni
@ 2022-06-30 17:39 ` Matthieu Baerts
3 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts @ 2022-06-30 17:39 UTC (permalink / raw)
To: Mat Martineau, mptcp, pabeni
Hi Mat, Paolo,
On 30/06/2022 00:53, Mat Martineau wrote:
> When working with Kishen on the userspace PM, the locking code in
> mptcp_pm_nl_mp_prio_send_ack() caught my attention. The
> release-and-reacquire of the PM lock in that function was requiring the
> userspace PM code to grab that lock, even though nothing protected by
> that lock was being accessed. Patch 1 addresses this.
>
> While investigating that issue, I realized that
> mptcp_pm_nl_mp_prio_send_ack() was not holding the subflow socket lock
> while modifying several parts of the subflow context. Patch 2 addresses
> this, and is not a drastic change since the subflow lock was being
> acquired anyway in mptcp_subflow_send_ack().
>
> v2->v3: Fix build error
>
> v1->v2: Based on Paolo's feedback, fix MIB issue with atomic context in
> patch 1 (tested with CONFIG_PREEMPT), and squash patches 2 & 3.
>
>
> Mat Martineau (2):
> mptcp: Avoid acquiring PM lock for subflow priority changes
> mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags
Thank you for the patches and the reviews!
Now in our tree (fixes for -net) with Paolo's ACK and without the
internal changelog.
New patches for t/upstream-net:
- 2ac39a576e95: mptcp: Avoid acquiring PM lock for subflow priority changes
- 1554cc2cec00: mptcp: Acquire the subflow socket lock before modifying
MP_PRIO flags
- Results: 1fc747ab56ea..3c88708b1aca (export-net)
New patches for t/upstream:
- 2ac39a576e95: mptcp: Avoid acquiring PM lock for subflow priority changes
- 1554cc2cec00: mptcp: Acquire the subflow socket lock before modifying
MP_PRIO flags
- Results: 223590507ff9..ce6114ac321d (export)
Builds and tests are now in progress:
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20220630T173736
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export-net
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220630T173736
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 6+ messages in thread