[PATCH RFC 0/4] mptcp: let the in kernel PM control the MPC

[PATCH RFC 0/4] mptcp: let the in kernel PM control the MPC

[Paolo Abeni]

this is follow-up to the last public mtg and:

https://github.com/multipath-tcp/mptcp_net-next/issues/285

implementing the idea described in the last comment from the above isse
It's just a proof of concept, only compile tested, to better describe
the idea.

While implementing the above I found a fews bugs fixed in patch
1/4, 2/4 and 4/4.

If there is agreement on this kind of changes, next week I plan to cook
a formal submission, including a few additional self-tests.

Paolo Abeni (4):
  mptcp: fix local endpoint acconting.
  mptcp: introduce and use mptcp_pm_send_ack()
  mptcp: allow the in kernel PM to set MPC subflow priority
  mptcp: more accurate MPC endpoint tracking

 net/mptcp/pm_netlink.c | 103 ++++++++++++++++++++++-------------------
 net/mptcp/protocol.c   |  11 +++--
 net/mptcp/protocol.h   |   3 +-
 3 files changed, 65 insertions(+), 52 deletions(-)

-- 
2.35.3

[PATCH RFC 1/4] mptcp: fix local endpoint acconting.

[Paolo Abeni]

In mptcp_pm_nl_rm_addr_or_subflow() we always mark as availble
the id corresponding to the just removed address.

The used bitmap actually tracks only the local IDs: we must
restrict the operation when a (local) subflow is removed.

Fixes: a88c9e496937 ("mptcp: do not block subflows creation on errors")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e099f2a12504..2b2bb3599781 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -801,7 +801,8 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 			removed = true;
 			__MPTCP_INC_STATS(sock_net(sk), rm_type);
 		}
-		__set_bit(rm_list->ids[i], msk->pm.id_avail_bitmap);
+		if (rm_type == MPTCP_MIB_RMSUBFLOW)
+			__set_bit(rm_list->ids[i], msk->pm.id_avail_bitmap);
 		if (!removed)
 			continue;
 
-- 
2.35.3

[PATCH RFC 2/4] mptcp: introduce and use mptcp_pm_send_ack()

[Paolo Abeni]

The in-kernel PM has a bit of duplicate code related to ack
generation. Create a new helper factoring out the PM-specific
needs and use it in a couple of places.

Note that this additionally moves a few unsafe subflow socket
access under the relevant socket lock.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c | 45 ++++++++++++++++++++++++------------------
 net/mptcp/protocol.c   | 11 ++++++++---
 net/mptcp/protocol.h   |  2 +-
 3 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 2b2bb3599781..91f6ed2a076a 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -463,6 +463,29 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
 	return i;
 }
 
+static void mptcp_pm_send_ack(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
+			      bool prio, bool backup)
+{
+	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+	bool slow;
+
+	spin_unlock_bh(&msk->pm.lock);
+	pr_debug("send ack for %s",
+		 prio ? "mp_prio" : (mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr"));
+
+	slow = lock_sock_fast(ssk);
+	if (prio) {
+		subflow->send_mp_prio = 1;
+		subflow->backup = backup;
+		subflow->request_bkup = backup;
+	}
+
+	__mptcp_subflow_send_ack(ssk);
+	unlock_sock_fast(ssk, slow);
+
+	spin_lock_bh(&msk->pm.lock);
+}
+
 static struct mptcp_pm_addr_entry *
 __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
 {
@@ -705,16 +728,8 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
 		return;
 
 	subflow = list_first_entry_or_null(&msk->conn_list, typeof(*subflow), node);
-	if (subflow) {
-		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
-
-		spin_unlock_bh(&msk->pm.lock);
-		pr_debug("send ack for %s",
-			 mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
-
-		mptcp_subflow_send_ack(ssk);
-		spin_lock_bh(&msk->pm.lock);
-	}
+	if (subflow)
+		mptcp_pm_send_ack(msk, subflow, false, false);
 }
 
 static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
@@ -736,16 +751,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
 
 		if (subflow->backup != bkup)
 			msk->last_snd = NULL;
-		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);
-
+		mptcp_pm_send_ack(msk, subflow, true, bkup);
 		return 0;
 	}
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b1fae2f747c9..874344f7e0fa 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -502,13 +502,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)
+void __mptcp_subflow_send_ack(struct sock *ssk)
+{
+	if (tcp_can_send_ack(ssk))
+		tcp_send_ack(ssk);
+}
+
+static 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 95c9ace1437b..a92b6276a03c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -609,7 +609,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);
 void mptcp_sock_graft(struct sock *sk, struct socket *parent);
-- 
2.35.3

[PATCH RFC 3/4] mptcp: allow the in kernel PM to set MPC subflow priority

[Paolo Abeni]

Any local endpoints configured on the address matching the
MPC subflow are currently ignored.

Specifically, setting a backup flag on them has no effect
on the first subflow, as the MPC handshake can't carry such
info.

This change refactors the MPC endpoint id accounting to
additionally fetch the priority info from the relevant endpoint
and eventually trigger the MP_PRIO handshake as needed.

As a result, the MPC subflow now switches to backup priority
after that the MPTCP socket is fully established, according
to the local endpoint configuration.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 91f6ed2a076a..a6eb501e5031 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -505,30 +505,14 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info,
 	struct mptcp_pm_addr_entry *entry;
 
 	list_for_each_entry(entry, &pernet->local_addr_list, list) {
-		if ((!lookup_by_id && mptcp_addresses_equal(&entry->addr, info, true)) ||
+		if ((!lookup_by_id &&
+		     mptcp_addresses_equal(&entry->addr, info, entry->addr.port)) ||
 		    (lookup_by_id && entry->addr.id == info->id))
 			return entry;
 	}
 	return NULL;
 }
 
-static int
-lookup_id_by_addr(const struct pm_nl_pernet *pernet, const struct mptcp_addr_info *addr)
-{
-	const struct mptcp_pm_addr_entry *entry;
-	int ret = -1;
-
-	rcu_read_lock();
-	list_for_each_entry(entry, &pernet->local_addr_list, list) {
-		if (mptcp_addresses_equal(&entry->addr, addr, entry->addr.port)) {
-			ret = entry->addr.id;
-			break;
-		}
-	}
-	rcu_read_unlock();
-	return ret;
-}
-
 static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 {
 	struct sock *sk = (struct sock *)msk;
@@ -546,13 +530,22 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 
 	/* do lazy endpoint usage accounting for the MPC subflows */
 	if (unlikely(!(msk->pm.status & BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED))) && msk->first) {
+		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);
+		struct mptcp_pm_addr_entry *entry;
 		struct mptcp_addr_info mpc_addr;
-		int mpc_id;
+		bool backup = false;
 
 		local_address((struct sock_common *)msk->first, &mpc_addr);
-		mpc_id = lookup_id_by_addr(pernet, &mpc_addr);
-		if (mpc_id >= 0)
-			__clear_bit(mpc_id, msk->pm.id_avail_bitmap);
+		rcu_read_lock();
+		entry = __lookup_addr(pernet, &mpc_addr, false);
+		if (entry) {
+			__clear_bit(entry->addr.id, msk->pm.id_avail_bitmap);
+			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+		}
+		rcu_read_unlock();
+
+		if (backup)
+			mptcp_pm_send_ack(msk, subflow, true, backup);
 
 		msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
 	}
-- 
2.35.3

[PATCH RFC 4/4] mptcp: more accurate MPC endpoint tracking

[Paolo Abeni]

Currently the id accounting for the ID 0 subflow is not correct:
at creation time we mark (correctly) as unavailable the endpoint
id corresponding the MPC subflow source address, while at subflow
removal time set as available the id 0.

With this change we track explicitly the endpoint id corresponding
to the MPC subflow so that we can mark it as available at removal time.
Additionally this allow deleting the initial subflow via the NL PM
specifying the corresponding endpoint id.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c | 20 +++++++++++++-------
 net/mptcp/protocol.h   |  1 +
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index a6eb501e5031..cd0093ff1103 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -540,6 +540,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 		entry = __lookup_addr(pernet, &mpc_addr, false);
 		if (entry) {
 			__clear_bit(entry->addr.id, msk->pm.id_avail_bitmap);
+			msk->mpc_endpoint_id = entry->addr.id;
 			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 		}
 		rcu_read_unlock();
@@ -752,6 +753,11 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
 	return -EINVAL;
 }
 
+static bool mptcp_local_id_match(const struct mptcp_sock *msk, u8 local_id, u8 id)
+{
+	return local_id == id || (!local_id && msk->mpc_endpoint_id == id);
+}
+
 static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 					   const struct mptcp_rm_list *rm_list,
 					   enum linux_mptcp_mib_field rm_type)
@@ -775,6 +781,7 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 		return;
 
 	for (i = 0; i < rm_list->nr; i++) {
+		u8 rm_id = rm_list->ids[i];
 		bool removed = false;
 
 		list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node) {
@@ -782,15 +789,14 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 			int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
 			u8 id = subflow->local_id;
 
-			if (rm_type == MPTCP_MIB_RMADDR)
-				id = subflow->remote_id;
-
-			if (rm_list->ids[i] != id)
+			if (rm_type == MPTCP_MIB_RMADDR && subflow->remote_id != rm_id)
+				continue;
+			if (rm_type == MPTCP_MIB_RMSUBFLOW && !mptcp_local_id_match(msk, id, rm_id))
 				continue;
 
-			pr_debug(" -> %s rm_list_ids[%d]=%u local_id=%u remote_id=%u",
+			pr_debug(" -> %s rm_list_ids[%d]=%u local_id=%u remote_id=%u mpc_id=%u",
 				 rm_type == MPTCP_MIB_RMADDR ? "address" : "subflow",
-				 i, rm_list->ids[i], subflow->local_id, subflow->remote_id);
+				 i, rm_id, subflow->local_id, subflow->remote_id, msk->mpc_endpoint_id);
 			spin_unlock_bh(&msk->pm.lock);
 			mptcp_subflow_shutdown(sk, ssk, how);
 
@@ -802,7 +808,7 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 			__MPTCP_INC_STATS(sock_net(sk), rm_type);
 		}
 		if (rm_type == MPTCP_MIB_RMSUBFLOW)
-			__set_bit(rm_list->ids[i], msk->pm.id_avail_bitmap);
+			__set_bit(rm_id ? rm_id : msk->mpc_endpoint_id, msk->pm.id_avail_bitmap);
 		if (!removed)
 			continue;
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index a92b6276a03c..ec91776f9b18 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -282,6 +282,7 @@ struct mptcp_sock {
 	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
 	bool		csum_enabled;
 	bool		allow_infinite_fallback;
+	u8		mpc_endpoint_id;
 	u8		recvmsg_inq:1,
 			cork:1,
 			nodelay:1;
-- 
2.35.3

Re: mptcp: more accurate MPC endpoint tracking: Build Failure

[MPTCP CI]

Hi Paolo,

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/25ad186319f6b464aee3286c0386f2851f9a1f8f.1656088406.git.pabeni@redhat.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/2556986908

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

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: mptcp: more accurate MPC endpoint tracking: Tests Results

[MPTCP CI]

Hi Paolo,

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_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6620488473509888
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6620488473509888/summary/summary.txt

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

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


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: [PATCH RFC 2/4] mptcp: introduce and use mptcp_pm_send_ack()

[Mat Martineau]

On Fri, 24 Jun 2022, Paolo Abeni wrote:

> The in-kernel PM has a bit of duplicate code related to ack
> generation. Create a new helper factoring out the PM-specific
> needs and use it in a couple of places.
>
> Note that this additionally moves a few unsafe subflow socket
> access under the relevant socket lock.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/pm_netlink.c | 45 ++++++++++++++++++++++++------------------
> net/mptcp/protocol.c   | 11 ++++++++---
> net/mptcp/protocol.h   |  2 +-
> 3 files changed, 35 insertions(+), 23 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 2b2bb3599781..91f6ed2a076a 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -463,6 +463,29 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
> 	return i;
> }
>
> +static void mptcp_pm_send_ack(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
> +			      bool prio, bool backup)
> +{
> +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +	bool slow;
> +
> +	spin_unlock_bh(&msk->pm.lock);
> +	pr_debug("send ack for %s",
> +		 prio ? "mp_prio" : (mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr"));
> +
> +	slow = lock_sock_fast(ssk);
> +	if (prio) {
> +		subflow->send_mp_prio = 1;
> +		subflow->backup = backup;
> +		subflow->request_bkup = backup;
> +	}
> +
> +	__mptcp_subflow_send_ack(ssk);
> +	unlock_sock_fast(ssk, slow);
> +
> +	spin_lock_bh(&msk->pm.lock);
> +}

I was short on time yesterday so I delayed reviewing this RFC series... 
Instead I spent time working on the "Locking fixes for subflow flag 
changes" series which has very, very similar locking changes. MPTCP minds 
think alike?

I guess the lesson is that I should always look at Paolo's pending patches 
before trying to solve a "new" problem :)

In that other series, I remove the pm locking when sending this ack for 
MP_PRIO - but I think this refactoring could still be helpful. The pm lock 
could be released and reacquired by the caller instead of inside 
mptcp_pm_send_ack().

- Mat

> +
> static struct mptcp_pm_addr_entry *
> __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
> {
> @@ -705,16 +728,8 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
> 		return;
>
> 	subflow = list_first_entry_or_null(&msk->conn_list, typeof(*subflow), node);
> -	if (subflow) {
> -		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> -
> -		spin_unlock_bh(&msk->pm.lock);
> -		pr_debug("send ack for %s",
> -			 mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
> -
> -		mptcp_subflow_send_ack(ssk);
> -		spin_lock_bh(&msk->pm.lock);
> -	}
> +	if (subflow)
> +		mptcp_pm_send_ack(msk, subflow, false, false);
> }
>
> static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
> @@ -736,16 +751,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
>
> 		if (subflow->backup != bkup)
> 			msk->last_snd = NULL;
> -		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);
> -
> +		mptcp_pm_send_ack(msk, subflow, true, bkup);
> 		return 0;
> 	}
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index b1fae2f747c9..874344f7e0fa 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -502,13 +502,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)
> +void __mptcp_subflow_send_ack(struct sock *ssk)
> +{
> +	if (tcp_can_send_ack(ssk))
> +		tcp_send_ack(ssk);
> +}
> +
> +static 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 95c9ace1437b..a92b6276a03c 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -609,7 +609,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);
> void mptcp_sock_graft(struct sock *sk, struct socket *parent);
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH RFC 3/4] mptcp: allow the in kernel PM to set MPC subflow priority

[Mat Martineau]

On Fri, 24 Jun 2022, Paolo Abeni wrote:

> Any local endpoints configured on the address matching the
> MPC subflow are currently ignored.
>
> Specifically, setting a backup flag on them has no effect
> on the first subflow, as the MPC handshake can't carry such
> info.
>
> This change refactors the MPC endpoint id accounting to
> additionally fetch the priority info from the relevant endpoint
> and eventually trigger the MP_PRIO handshake as needed.
>
> As a result, the MPC subflow now switches to backup priority
> after that the MPTCP socket is fully established, according
> to the local endpoint configuration.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/pm_netlink.c | 37 +++++++++++++++----------------------
> 1 file changed, 15 insertions(+), 22 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 91f6ed2a076a..a6eb501e5031 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -505,30 +505,14 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info,
> 	struct mptcp_pm_addr_entry *entry;
>
> 	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> -		if ((!lookup_by_id && mptcp_addresses_equal(&entry->addr, info, true)) ||
> +		if ((!lookup_by_id &&
> +		     mptcp_addresses_equal(&entry->addr, info, entry->addr.port)) ||

It seems like we could have multiple entries in the local_addr_list with 
the same address, but with different entries having port==0 or nonzero 
ports. If mptcp_nl_cmd_set_flags() is called with a nonzero port, but this 
lookup function encounters the port==0 entry first, this will match an 
unexpected entry in local_addr_list.

Is there some other constraint preventing this?

> 		    (lookup_by_id && entry->addr.id == info->id))
> 			return entry;
> 	}
> 	return NULL;
> }
>
> -static int
> -lookup_id_by_addr(const struct pm_nl_pernet *pernet, const struct mptcp_addr_info *addr)
> -{
> -	const struct mptcp_pm_addr_entry *entry;
> -	int ret = -1;
> -
> -	rcu_read_lock();
> -	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> -		if (mptcp_addresses_equal(&entry->addr, addr, entry->addr.port)) {
> -			ret = entry->addr.id;
> -			break;
> -		}
> -	}
> -	rcu_read_unlock();
> -	return ret;
> -}
> -
> static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> {
> 	struct sock *sk = (struct sock *)msk;
> @@ -546,13 +530,22 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>
> 	/* do lazy endpoint usage accounting for the MPC subflows */
> 	if (unlikely(!(msk->pm.status & BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED))) && msk->first) {
> +		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);
> +		struct mptcp_pm_addr_entry *entry;
> 		struct mptcp_addr_info mpc_addr;
> -		int mpc_id;
> +		bool backup = false;
>
> 		local_address((struct sock_common *)msk->first, &mpc_addr);
> -		mpc_id = lookup_id_by_addr(pernet, &mpc_addr);
> -		if (mpc_id >= 0)
> -			__clear_bit(mpc_id, msk->pm.id_avail_bitmap);
> +		rcu_read_lock();
> +		entry = __lookup_addr(pernet, &mpc_addr, false);
> +		if (entry) {
> +			__clear_bit(entry->addr.id, msk->pm.id_avail_bitmap);
> +			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> +		}
> +		rcu_read_unlock();
> +
> +		if (backup)
> +			mptcp_pm_send_ack(msk, subflow, true, backup);

This looks ok to me. I think I follow your example in #285 for how to 
create endpoints that would allow the priority to be changed later, but I 
think the selftests might make that clearer.

Thanks!

>
> 		msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
> 	}
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH RFC 2/4] mptcp: introduce and use mptcp_pm_send_ack()

[Paolo Abeni]

On Tue, 2022-06-28 at 16:28 -0700, Mat Martineau wrote:
> On Fri, 24 Jun 2022, Paolo Abeni wrote:
> 
> > The in-kernel PM has a bit of duplicate code related to ack
> > generation. Create a new helper factoring out the PM-specific
> > needs and use it in a couple of places.
> > 
> > Note that this additionally moves a few unsafe subflow socket
> > access under the relevant socket lock.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/pm_netlink.c | 45 ++++++++++++++++++++++++------------------
> > net/mptcp/protocol.c   | 11 ++++++++---
> > net/mptcp/protocol.h   |  2 +-
> > 3 files changed, 35 insertions(+), 23 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 2b2bb3599781..91f6ed2a076a 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -463,6 +463,29 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
> > 	return i;
> > }
> > 
> > +static void mptcp_pm_send_ack(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
> > +			      bool prio, bool backup)
> > +{
> > +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > +	bool slow;
> > +
> > +	spin_unlock_bh(&msk->pm.lock);
> > +	pr_debug("send ack for %s",
> > +		 prio ? "mp_prio" : (mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr"));
> > +
> > +	slow = lock_sock_fast(ssk);
> > +	if (prio) {
> > +		subflow->send_mp_prio = 1;
> > +		subflow->backup = backup;
> > +		subflow->request_bkup = backup;
> > +	}
> > +
> > +	__mptcp_subflow_send_ack(ssk);
> > +	unlock_sock_fast(ssk, slow);
> > +
> > +	spin_lock_bh(&msk->pm.lock);
> > +}
> 
> I was short on time yesterday so I delayed reviewing this RFC series... 
> Instead I spent time working on the "Locking fixes for subflow flag 
> changes" series which has very, very similar locking changes. MPTCP minds 
> think alike?

:)


> I guess the lesson is that I should always look at Paolo's pending patches 
> before trying to solve a "new" problem :)
> 
> In that other series, I remove the pm locking when sending this ack for 
> MP_PRIO - but I think this refactoring could still be helpful. The pm lock 
> could be released and reacquired by the caller instead of inside 
> mptcp_pm_send_ack().

LGTM! If you agree, I could rebase this series on top of "mptcp: Avoid
acquiring PM lock for subflow priority changes".


Thank!

Paolo

Re: [PATCH RFC 3/4] mptcp: allow the in kernel PM to set MPC subflow priority

[Paolo Abeni]

On Tue, 2022-06-28 at 17:11 -0700, Mat Martineau wrote:
> On Fri, 24 Jun 2022, Paolo Abeni wrote:
> 
> > Any local endpoints configured on the address matching the
> > MPC subflow are currently ignored.
> > 
> > Specifically, setting a backup flag on them has no effect
> > on the first subflow, as the MPC handshake can't carry such
> > info.
> > 
> > This change refactors the MPC endpoint id accounting to
> > additionally fetch the priority info from the relevant endpoint
> > and eventually trigger the MP_PRIO handshake as needed.
> > 
> > As a result, the MPC subflow now switches to backup priority
> > after that the MPTCP socket is fully established, according
> > to the local endpoint configuration.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/pm_netlink.c | 37 +++++++++++++++----------------------
> > 1 file changed, 15 insertions(+), 22 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 91f6ed2a076a..a6eb501e5031 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -505,30 +505,14 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info,
> > 	struct mptcp_pm_addr_entry *entry;
> > 
> > 	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> > -		if ((!lookup_by_id && mptcp_addresses_equal(&entry->addr, info, true)) ||
> > +		if ((!lookup_by_id &&
> > +		     mptcp_addresses_equal(&entry->addr, info, entry->addr.port)) ||
> 
> It seems like we could have multiple entries in the local_addr_list with 
> the same address, but with different entries having port==0 or nonzero 
> ports. If mptcp_nl_cmd_set_flags() is called with a nonzero port, but this 
> lookup function encounters the port==0 entry first, this will match an 
> unexpected entry in local_addr_list.
> 
> Is there some other constraint preventing this?

Uhmmm.. I updated the mptcp_addresses_equal() statement to match the
existing one in lookup_id_by_addr() below.

You are right, this check is uncorrect, and generally speaking the
mptcp_addresses_equal() invocations are a bit fuzzy. I'll try to
clarify that with an additional patches:

- mptcp_addresses_equal() on subflow list use port matching if the
looked-up address has  non 0 port (the current code looks correct)

- mptcp_addresses_equal on local_addr_list should use port matching if
address_use_port(entry), and entry with port matching should be
preferred over entries without port-matching. The latter can be
enforces adding entries with address_use_port(entry) at the list head
and entries with !address_use_port(entry) at tail

WDYT?

Thanks!

Paolo

Re: [PATCH RFC 2/4] mptcp: introduce and use mptcp_pm_send_ack()

[Mat Martineau]

On Wed, 29 Jun 2022, Paolo Abeni wrote:

> On Tue, 2022-06-28 at 16:28 -0700, Mat Martineau wrote:
>> On Fri, 24 Jun 2022, Paolo Abeni wrote:
>>
>>> The in-kernel PM has a bit of duplicate code related to ack
>>> generation. Create a new helper factoring out the PM-specific
>>> needs and use it in a couple of places.
>>>
>>> Note that this additionally moves a few unsafe subflow socket
>>> access under the relevant socket lock.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> net/mptcp/pm_netlink.c | 45 ++++++++++++++++++++++++------------------
>>> net/mptcp/protocol.c   | 11 ++++++++---
>>> net/mptcp/protocol.h   |  2 +-
>>> 3 files changed, 35 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index 2b2bb3599781..91f6ed2a076a 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -463,6 +463,29 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
>>> 	return i;
>>> }
>>>
>>> +static void mptcp_pm_send_ack(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
>>> +			      bool prio, bool backup)
>>> +{
>>> +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>>> +	bool slow;
>>> +
>>> +	spin_unlock_bh(&msk->pm.lock);
>>> +	pr_debug("send ack for %s",
>>> +		 prio ? "mp_prio" : (mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr"));
>>> +
>>> +	slow = lock_sock_fast(ssk);
>>> +	if (prio) {
>>> +		subflow->send_mp_prio = 1;
>>> +		subflow->backup = backup;
>>> +		subflow->request_bkup = backup;
>>> +	}
>>> +
>>> +	__mptcp_subflow_send_ack(ssk);
>>> +	unlock_sock_fast(ssk, slow);
>>> +
>>> +	spin_lock_bh(&msk->pm.lock);
>>> +}
>>
>> I was short on time yesterday so I delayed reviewing this RFC series...
>> Instead I spent time working on the "Locking fixes for subflow flag
>> changes" series which has very, very similar locking changes. MPTCP minds
>> think alike?
>
> :)
>
>
>> I guess the lesson is that I should always look at Paolo's pending patches
>> before trying to solve a "new" problem :)
>>
>> In that other series, I remove the pm locking when sending this ack for
>> MP_PRIO - but I think this refactoring could still be helpful. The pm lock
>> could be released and reacquired by the caller instead of inside
>> mptcp_pm_send_ack().
>
> LGTM! If you agree, I could rebase this series on top of "mptcp: Avoid
> acquiring PM lock for subflow priority changes".
>

Yes, would be good to rebase on that series. Thanks!

--
Mat Martineau
Intel

Re: [PATCH RFC 3/4] mptcp: allow the in kernel PM to set MPC subflow priority

[Mat Martineau]

On Wed, 29 Jun 2022, Paolo Abeni wrote:

> On Tue, 2022-06-28 at 17:11 -0700, Mat Martineau wrote:
>> On Fri, 24 Jun 2022, Paolo Abeni wrote:
>>
>>> Any local endpoints configured on the address matching the
>>> MPC subflow are currently ignored.
>>>
>>> Specifically, setting a backup flag on them has no effect
>>> on the first subflow, as the MPC handshake can't carry such
>>> info.
>>>
>>> This change refactors the MPC endpoint id accounting to
>>> additionally fetch the priority info from the relevant endpoint
>>> and eventually trigger the MP_PRIO handshake as needed.
>>>
>>> As a result, the MPC subflow now switches to backup priority
>>> after that the MPTCP socket is fully established, according
>>> to the local endpoint configuration.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> net/mptcp/pm_netlink.c | 37 +++++++++++++++----------------------
>>> 1 file changed, 15 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index 91f6ed2a076a..a6eb501e5031 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -505,30 +505,14 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info,
>>> 	struct mptcp_pm_addr_entry *entry;
>>>
>>> 	list_for_each_entry(entry, &pernet->local_addr_list, list) {
>>> -		if ((!lookup_by_id && mptcp_addresses_equal(&entry->addr, info, true)) ||
>>> +		if ((!lookup_by_id &&
>>> +		     mptcp_addresses_equal(&entry->addr, info, entry->addr.port)) ||
>>
>> It seems like we could have multiple entries in the local_addr_list with
>> the same address, but with different entries having port==0 or nonzero
>> ports. If mptcp_nl_cmd_set_flags() is called with a nonzero port, but this
>> lookup function encounters the port==0 entry first, this will match an
>> unexpected entry in local_addr_list.
>>
>> Is there some other constraint preventing this?
>
> Uhmmm.. I updated the mptcp_addresses_equal() statement to match the
> existing one in lookup_id_by_addr() below.
>
> You are right, this check is uncorrect, and generally speaking the
> mptcp_addresses_equal() invocations are a bit fuzzy. I'll try to
> clarify that with an additional patches:
>
> - mptcp_addresses_equal() on subflow list use port matching if the
> looked-up address has  non 0 port (the current code looks correct)
>
> - mptcp_addresses_equal on local_addr_list should use port matching if
> address_use_port(entry), and entry with port matching should be
> preferred over entries without port-matching. The latter can be
> enforces adding entries with address_use_port(entry) at the list head
> and entries with !address_use_port(entry) at tail
>
> WDYT?

That looks like a good plan - thanks

--
Mat Martineau
Intel