* [PATCH mptcp-net 0/3] Locking fixes for subflow flag changes @ 2022-06-28 19:03 Mat Martineau 2022-06-28 19:03 ` [PATCH mptcp-net 1/3] mptcp: Avoid acquiring PM lock for subflow priority changes Mat Martineau ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Mat Martineau @ 2022-06-28 19:03 UTC (permalink / raw) To: mptcp; +Cc: Mat Martineau 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 3 addresses this, and is not a drastic change since the subflow lock was being acquired anyway in mptcp_subflow_send_ack(). Patch 2 is a prerequisite patch adding a __mptcp_subflow_send_ack() helper to use in patch 3 when the subflow is already locked. Mat Martineau (3): mptcp: Avoid acquiring PM lock for subflow priority changes mptcp: Add a variant of mptcp_subflow_send_ack() for locked subflows mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags net/mptcp/pm_netlink.c | 11 ++++++----- net/mptcp/protocol.c | 9 +++++++-- net/mptcp/protocol.h | 1 + 3 files changed, 14 insertions(+), 7 deletions(-) -- 2.37.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH mptcp-net 1/3] mptcp: Avoid acquiring PM lock for subflow priority changes 2022-06-28 19:03 [PATCH mptcp-net 0/3] Locking fixes for subflow flag changes Mat Martineau @ 2022-06-28 19:03 ` Mat Martineau 2022-06-29 19:57 ` Paolo Abeni 2022-06-28 19:03 ` [PATCH mptcp-net 2/3] mptcp: Add a variant of mptcp_subflow_send_ack() for locked subflows Mat Martineau 2022-06-28 19:03 ` [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Mat Martineau 2 siblings, 1 reply; 8+ messages in thread From: Mat Martineau @ 2022-06-28 19:03 UTC (permalink / raw) To: mptcp; +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. 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. Fixes: 0f9f696a502e ("mptcp: add set_flags command in PM netlink") Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> --- net/mptcp/pm_netlink.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index e099f2a12504..d04b34fc9a8e 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -741,10 +741,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk, 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 +1814,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 +1835,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] 8+ messages in thread
* Re: [PATCH mptcp-net 1/3] mptcp: Avoid acquiring PM lock for subflow priority changes 2022-06-28 19:03 ` [PATCH mptcp-net 1/3] mptcp: Avoid acquiring PM lock for subflow priority changes Mat Martineau @ 2022-06-29 19:57 ` Paolo Abeni 0 siblings, 0 replies; 8+ messages in thread From: Paolo Abeni @ 2022-06-29 19:57 UTC (permalink / raw) To: Mat Martineau, mptcp On Tue, 2022-06-28 at 12:03 -0700, Mat Martineau wrote: > 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. > > 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. > > Fixes: 0f9f696a502e ("mptcp: add set_flags command in PM netlink") > Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > --- > net/mptcp/pm_netlink.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index e099f2a12504..d04b34fc9a8e 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -741,10 +741,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk, > 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 +1814,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 +1835,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); This causes a BUG: using __this_cpu_add() in preemptible code as __MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIOTX); in mptcp_pm_nl_mp_prio_send_ack is no longer in atomic context. Either use MPTCP_INC_STATS() and/or move the increment in write_options (I think the latter is better) > if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH) > mptcp_pm_nl_fullmesh(msk, addr); > - spin_unlock_bh(&msk->pm.lock); > release_sock(sk); > > next: ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH mptcp-net 2/3] mptcp: Add a variant of mptcp_subflow_send_ack() for locked subflows 2022-06-28 19:03 [PATCH mptcp-net 0/3] Locking fixes for subflow flag changes Mat Martineau 2022-06-28 19:03 ` [PATCH mptcp-net 1/3] mptcp: Avoid acquiring PM lock for subflow priority changes Mat Martineau @ 2022-06-28 19:03 ` Mat Martineau 2022-06-28 19:03 ` [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Mat Martineau 2 siblings, 0 replies; 8+ messages in thread From: Mat Martineau @ 2022-06-28 19:03 UTC (permalink / raw) To: mptcp; +Cc: Mat Martineau The existing mptcp_subflow_send_ack() function handles subflow locking, but it's also sometimes necessary to force an ack from a caller that already holds the subflow socket lock. This is required for the following commit. Fixes: 340fa6667a69 ("mptcp: Only send extra TCP acks in eligible socket states") Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> --- net/mptcp/protocol.c | 9 +++++++-- net/mptcp/protocol.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) 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] 8+ messages in thread
* [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags 2022-06-28 19:03 [PATCH mptcp-net 0/3] Locking fixes for subflow flag changes Mat Martineau 2022-06-28 19:03 ` [PATCH mptcp-net 1/3] mptcp: Avoid acquiring PM lock for subflow priority changes Mat Martineau 2022-06-28 19:03 ` [PATCH mptcp-net 2/3] mptcp: Add a variant of mptcp_subflow_send_ack() for locked subflows Mat Martineau @ 2022-06-28 19:03 ` Mat Martineau 2022-06-28 20:50 ` mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags: Tests Results MPTCP CI 2022-06-29 19:59 ` [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Paolo Abeni 2 siblings, 2 replies; 8+ messages in thread From: Mat Martineau @ 2022-06-28 19:03 UTC (permalink / raw) To: mptcp; +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. 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 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index d04b34fc9a8e..05c6a95e9c28 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -729,11 +729,13 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk, struct sock *ssk = mptcp_subflow_tcp_sock(subflow); struct sock *sk = (struct sock *)msk; 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; @@ -742,7 +744,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk, __MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIOTX); pr_debug("send ack for mp_prio"); - mptcp_subflow_send_ack(ssk); + __mptcp_subflow_send_ack(ssk); + unlock_sock_fast(ssk, slow); return 0; } -- 2.37.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags: Tests Results 2022-06-28 19:03 ` [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Mat Martineau @ 2022-06-28 20:50 ` MPTCP CI 2022-06-29 19:59 ` [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Paolo Abeni 1 sibling, 0 replies; 8+ messages in thread From: MPTCP CI @ 2022-06-28 20:50 UTC (permalink / raw) To: Mat Martineau; +Cc: mptcp Hi Mat, Thank you for your modifications, that's great! Our CI did some validations and here is its report: - KVM Validation: normal: - Unstable: 1 failed test(s): selftest_simult_flows 🔴: - Task: https://cirrus-ci.com/task/5828656101588992 - Summary: https://api.cirrus-ci.com/v1/artifact/task/5828656101588992/summary/summary.txt - KVM Validation: debug: - Unstable: 2 failed test(s): packetdrill_add_addr selftest_mptcp_join 🔴: - Task: https://cirrus-ci.com/task/5265706148167680 - Summary: https://api.cirrus-ci.com/v1/artifact/task/5265706148167680/summary/summary.txt Initiator: Patchew Applier Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/aebc91b918e8 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-debug 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 (Tessares) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags 2022-06-28 19:03 ` [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Mat Martineau 2022-06-28 20:50 ` mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags: Tests Results MPTCP CI @ 2022-06-29 19:59 ` Paolo Abeni 2022-06-29 20:51 ` Mat Martineau 1 sibling, 1 reply; 8+ messages in thread From: Paolo Abeni @ 2022-06-29 19:59 UTC (permalink / raw) To: Mat Martineau, mptcp On Tue, 2022-06-28 at 12:03 -0700, Mat Martineau wrote: > 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. > > 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 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index d04b34fc9a8e..05c6a95e9c28 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -729,11 +729,13 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk, > struct sock *ssk = mptcp_subflow_tcp_sock(subflow); > struct sock *sk = (struct sock *)msk; > 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; > @@ -742,7 +744,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk, > __MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIOTX); > > pr_debug("send ack for mp_prio"); > - mptcp_subflow_send_ack(ssk); > + __mptcp_subflow_send_ack(ssk); > + unlock_sock_fast(ssk, slow); After this chunk, we can remove mptcp_subflow_send_ack() from protocol.h and make it static. I think it's easier squashing this patch into the previous one. Thanks! Paolo > > return 0; > } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags 2022-06-29 19:59 ` [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Paolo Abeni @ 2022-06-29 20:51 ` Mat Martineau 0 siblings, 0 replies; 8+ messages in thread From: Mat Martineau @ 2022-06-29 20:51 UTC (permalink / raw) To: Paolo Abeni; +Cc: mptcp On Wed, 29 Jun 2022, Paolo Abeni wrote: > On Tue, 2022-06-28 at 12:03 -0700, Mat Martineau wrote: >> 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. >> >> 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 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c >> index d04b34fc9a8e..05c6a95e9c28 100644 >> --- a/net/mptcp/pm_netlink.c >> +++ b/net/mptcp/pm_netlink.c >> @@ -729,11 +729,13 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk, >> struct sock *ssk = mptcp_subflow_tcp_sock(subflow); >> struct sock *sk = (struct sock *)msk; >> 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; >> @@ -742,7 +744,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk, >> __MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIOTX); >> >> pr_debug("send ack for mp_prio"); >> - mptcp_subflow_send_ack(ssk); >> + __mptcp_subflow_send_ack(ssk); >> + unlock_sock_fast(ssk, slow); > > After this chunk, we can remove mptcp_subflow_send_ack() from > protocol.h and make it static. I think it's easier squashing this patch > into the previous one. > There's still a call to mptcp_subflow_send_ack() in pm_netlink.c, so I'll leave both in protocol.h for now. Will squash patches 2 & 3 though. -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-06-29 20:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-28 19:03 [PATCH mptcp-net 0/3] Locking fixes for subflow flag changes Mat Martineau 2022-06-28 19:03 ` [PATCH mptcp-net 1/3] mptcp: Avoid acquiring PM lock for subflow priority changes Mat Martineau 2022-06-29 19:57 ` Paolo Abeni 2022-06-28 19:03 ` [PATCH mptcp-net 2/3] mptcp: Add a variant of mptcp_subflow_send_ack() for locked subflows Mat Martineau 2022-06-28 19:03 ` [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Mat Martineau 2022-06-28 20:50 ` mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags: Tests Results MPTCP CI 2022-06-29 19:59 ` [PATCH mptcp-net 3/3] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags Paolo Abeni 2022-06-29 20:51 ` Mat Martineau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).