[PATCH mptcp-net v2 0/2] Locking fixes for subflow flag changes

[PATCH mptcp-net v2 0/2] Locking fixes for subflow flag changes

[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 2 addresses
this, and is not a drastic change since the subflow lock was being
acquired anyway in mptcp_subflow_send_ack().

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    |  2 ++
 net/mptcp/pm_netlink.c | 12 ++++++------
 net/mptcp/protocol.c   |  9 +++++++--
 net/mptcp/protocol.h   |  1 +
 4 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.36.1

[PATCH mptcp-net v2 1/2] mptcp: Avoid acquiring PM lock for subflow priority changes

[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.

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    | 2 ++
 net/mptcp/pm_netlink.c | 7 ++-----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index aead331866a0..d5e58d2ce153 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1584,6 +1584,8 @@ 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(sk), MPTCP_MIB_MPPRIOTX);
 	}
 
 mp_capable_done:
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e099f2a12504..936d72359c46 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -739,12 +739,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 +1813,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 +1834,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.36.1

[PATCH mptcp-net v2 2/2] mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags

[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 936d72359c46..04bae2d38b5b 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;
@@ -741,7 +743,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.36.1

Re: mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags: Build Failure

[MPTCP CI]

Hi Mat,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/20220629223550.655928-3-mathew.j.martineau@linux.intel.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/2586322258

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/2c27d7a3c28d

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-net v2 1/2] mptcp: Avoid acquiring PM lock for subflow priority changes

[Mat Martineau]

On Wed, 29 Jun 2022, 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. 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.
>
> 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    | 2 ++
> net/mptcp/pm_netlink.c | 7 ++-----
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index aead331866a0..d5e58d2ce153 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1584,6 +1584,8 @@ 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(sk), MPTCP_MIB_MPPRIOTX);

Forgot to check in the build fix for this line before generating the 
.patch files... v3 coming right up.

- Mat

> 	}
>
> mp_capable_done:
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index e099f2a12504..936d72359c46 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -739,12 +739,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 +1813,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 +1834,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.36.1
>
>

--
Mat Martineau
Intel

Re: mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags: Tests Results

[MPTCP CI]

Hi Mat,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/4719806531239936
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4719806531239936/summary/summary.txt

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/5845706438082560
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5845706438082560/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/2c27d7a3c28d


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)

Re: mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags: Tests Results

[MPTCP CI]

Hi Mat,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4757612108447744
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4757612108447744/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/5883512015290368
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5883512015290368/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8a2a284523d9


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)

Re: mptcp: Acquire the subflow socket lock before modifying MP_PRIO flags: Tests Results

[MPTCP CI]

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)