* [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints
@ 2025-09-22 22:23 Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 1/6] mptcp: pm: in-kernel: remove stale_loss_cnt Matthieu Baerts (NGI0)
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Matthieu Baerts (NGI0) @ 2025-09-22 22:23 UTC (permalink / raw)
To: MPTCP Upstream; +Cc: Matthieu Baerts (NGI0)
Currently, upon the reception of an ADD_ADDR (and when the fullmesh flag
is not used), the in-kernel PM will create new subflows using the local
address the routing configuration will pick.
It would be easier to pick local addresses from a selected list of
endpoints, and use it only once, than relying on routing rules.
Use case: both the client (C) and the server (S) have two addresses (a
and b). The client establishes the connection between C(a) and S(a).
Once established, the server announces its additional address S(b). Once
received, the client connects to it using its second address C(b).
Compared to a situation without the 'address' endpoint for C(b), the
client didn't use this address C(b) to establish a subflow to the
server's primary address S(a). So at the end, we have:
C S
C(a) --- S(a)
C(b) --- S(b)
In case of a 3rd address on each side (C(c) and S(c)), upon the
reception of an ADD_ADDR with S(c), the client should not pick C(b)
because it has already been used. C(c) should then be used.
Note that this situation is currently possible if C doesn't add any
endpoint, but configure the routing in order to pick C(b) for the route
to S(b), and pick C(c) for the route to S(c). That doesn't sound very
practical because it means knowing in advance the IP addresses that
will be used and announced by the server.
Patches 1 & 2: some clean-ups and refactoring.
Patch 3: "standardisation" and small perf improvement.
Patch 4: squash to patches for a commit queued for net-next.
Patch 5: new 'address' endpoints.
Patch 6: validation using selftests.
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (6):
mptcp: pm: in-kernel: remove stale_loss_cnt
mptcp: pm: in-kernel: reduce pernet struct size
mptcp: pm: in-kernel: compare IDs instead of addresses
Squash to "mptcp: pm: in-kernel: usable client side with C-flag"
mptcp: pm: in-kernel: add 'address' endpoints
selftests: mptcp: join: validate new 'address' endpoints
include/uapi/linux/mptcp.h | 6 +-
net/mptcp/pm_kernel.c | 228 ++++++++++++++++--------
net/mptcp/protocol.h | 9 +-
net/mptcp/sockopt.c | 2 +
tools/testing/selftests/net/mptcp/mptcp_join.sh | 56 ++++++
tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 9 +
6 files changed, 229 insertions(+), 81 deletions(-)
---
base-commit: 77807b94c731081ef3d97e96dabcea7aae2bfe15
change-id: 20250918-pm-kern-endp-add_addr-new-a20893e45389
Best regards,
--
Matthieu Baerts (NGI0) <matttbe@kernel.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 1/6] mptcp: pm: in-kernel: remove stale_loss_cnt
2025-09-22 22:23 [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
@ 2025-09-22 22:23 ` Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 2/6] mptcp: pm: in-kernel: reduce pernet struct size Matthieu Baerts (NGI0)
` (5 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Matthieu Baerts (NGI0) @ 2025-09-22 22:23 UTC (permalink / raw)
To: MPTCP Upstream; +Cc: Matthieu Baerts (NGI0)
It is currently not used.
It was in fact never used since its introduction in commit ff5a0b421cb2
("mptcp: faster active backup recovery"). It was probably initially
added to struct pm_nl_pernet during the development of this commit,
before being added to struct mptcp_pernet in ctrl.c, but not removed
from the first place.
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
net/mptcp/pm_kernel.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index e84c381aa3a4e2ad8a05e4b0daa7a5e8daf2c35b..52ae6304b311189ab927857ab74a9ee44ac6d76a 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -19,7 +19,6 @@ struct pm_nl_pernet {
spinlock_t lock;
struct list_head endp_list;
unsigned int endpoints;
- unsigned int stale_loss_cnt;
unsigned int endp_signal_max;
unsigned int endp_subflow_max;
unsigned int limit_add_addr_accepted;
@@ -1461,7 +1460,6 @@ static int __net_init pm_nl_init_net(struct net *net)
/* Cit. 2 subflows ought to be enough for anybody. */
pernet->limit_extra_subflows = 2;
pernet->next_id = 1;
- pernet->stale_loss_cnt = 4;
spin_lock_init(&pernet->lock);
/* No need to initialize other pernet fields, the struct is zeroed at
--
2.51.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 2/6] mptcp: pm: in-kernel: reduce pernet struct size
2025-09-22 22:23 [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 1/6] mptcp: pm: in-kernel: remove stale_loss_cnt Matthieu Baerts (NGI0)
@ 2025-09-22 22:23 ` Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 3/6] mptcp: pm: in-kernel: compare IDs instead of addresses Matthieu Baerts (NGI0)
` (4 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Matthieu Baerts (NGI0) @ 2025-09-22 22:23 UTC (permalink / raw)
To: MPTCP Upstream; +Cc: Matthieu Baerts (NGI0)
All the 'unsigned int' variables from the 'pm_nl_pernet' structure are
bounded to MPTCP_PM_ADDR_MAX, currently set to 8. The endpoint ID is
also bounded by the protocol to 8-bit. MPTCP_PM_ADDR_MAX, if extended
later, will never over 8-bit.
So no need to use 'unsigned int' variables, 'u8' is enough.
Note that the exposed counters in MPTCP_INFO are already limited to
8-bit, same for pm->extra_subflows, and others. So it seems even better
to limit them to 8-bit.
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
net/mptcp/pm_kernel.c | 59 ++++++++++++++++++++-------------------------------
net/mptcp/protocol.h | 8 +++----
2 files changed, 27 insertions(+), 40 deletions(-)
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 52ae6304b311189ab927857ab74a9ee44ac6d76a..ae6ab3178c226fe3ec66d156bb0dc919075043e5 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -18,12 +18,12 @@ struct pm_nl_pernet {
/* protects pernet updates */
spinlock_t lock;
struct list_head endp_list;
- unsigned int endpoints;
- unsigned int endp_signal_max;
- unsigned int endp_subflow_max;
- unsigned int limit_add_addr_accepted;
- unsigned int limit_extra_subflows;
- unsigned int next_id;
+ u8 endpoints;
+ u8 endp_signal_max;
+ u8 endp_subflow_max;
+ u8 limit_add_addr_accepted;
+ u8 limit_extra_subflows;
+ u8 next_id;
DECLARE_BITMAP(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
};
@@ -45,7 +45,7 @@ static struct pm_nl_pernet *genl_info_pm_nl(struct genl_info *info)
return pm_nl_get_pernet(genl_info_net(info));
}
-unsigned int mptcp_pm_get_endp_signal_max(const struct mptcp_sock *msk)
+u8 mptcp_pm_get_endp_signal_max(const struct mptcp_sock *msk)
{
const struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
@@ -53,7 +53,7 @@ unsigned int mptcp_pm_get_endp_signal_max(const struct mptcp_sock *msk)
}
EXPORT_SYMBOL_GPL(mptcp_pm_get_endp_signal_max);
-unsigned int mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk)
+u8 mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk)
{
struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
@@ -61,7 +61,7 @@ unsigned int mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk)
}
EXPORT_SYMBOL_GPL(mptcp_pm_get_endp_subflow_max);
-unsigned int mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk)
+u8 mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk)
{
struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
@@ -69,7 +69,7 @@ unsigned int mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk)
}
EXPORT_SYMBOL_GPL(mptcp_pm_get_limit_add_addr_accepted);
-unsigned int mptcp_pm_get_limit_extra_subflows(const struct mptcp_sock *msk)
+u8 mptcp_pm_get_limit_extra_subflows(const struct mptcp_sock *msk)
{
struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
@@ -185,15 +185,13 @@ fill_remote_addresses_fullmesh(struct mptcp_sock *msk,
struct mptcp_addr_info *local,
struct mptcp_addr_info *addrs)
{
+ u8 limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
bool deny_id0 = READ_ONCE(msk->pm.remote_deny_join_id0);
DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
struct sock *sk = (struct sock *)msk, *ssk;
struct mptcp_subflow_context *subflow;
- unsigned int limit_extra_subflows;
int i = 0;
- limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
-
/* Forbid creation of new subflows matching existing ones, possibly
* already created by incoming ADD_ADDR
*/
@@ -272,20 +270,14 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
{
+ u8 limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
+ struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+ u8 endp_subflow_max = mptcp_pm_get_endp_subflow_max(msk);
+ u8 endp_signal_max = mptcp_pm_get_endp_signal_max(msk);
struct sock *sk = (struct sock *)msk;
- unsigned int limit_extra_subflows;
bool signal_and_subflow = false;
- unsigned int endp_subflow_max;
- unsigned int endp_signal_max;
- struct pm_nl_pernet *pernet;
struct mptcp_pm_local local;
- pernet = pm_nl_get_pernet(sock_net(sk));
-
- endp_signal_max = mptcp_pm_get_endp_signal_max(msk);
- endp_subflow_max = mptcp_pm_get_endp_subflow_max(msk);
- limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(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);
@@ -400,16 +392,15 @@ fill_local_addresses_vec_fullmesh(struct mptcp_sock *msk,
struct mptcp_pm_local *locals,
bool c_flag_case)
{
+ u8 limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
struct sock *sk = (struct sock *)msk;
struct mptcp_pm_addr_entry *entry;
- unsigned int limit_extra_subflows;
struct mptcp_addr_info mpc_addr;
struct mptcp_pm_local *local;
int i = 0;
mptcp_local_address((struct sock_common *)msk, &mpc_addr);
- limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
rcu_read_lock();
list_for_each_entry_rcu(entry, &pernet->endp_list, list) {
@@ -448,16 +439,15 @@ fill_local_addresses_vec_c_flag(struct mptcp_sock *msk,
struct mptcp_addr_info *remote,
struct mptcp_pm_local *locals)
{
- unsigned int endp_subflow_max = mptcp_pm_get_endp_subflow_max(msk);
+ u8 limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+ u8 endp_subflow_max = mptcp_pm_get_endp_subflow_max(msk);
struct sock *sk = (struct sock *)msk;
- unsigned int limit_extra_subflows;
struct mptcp_addr_info mpc_addr;
struct mptcp_pm_local *local;
int i = 0;
mptcp_local_address((struct sock_common *)msk, &mpc_addr);
- limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
while (msk->pm.local_addr_used < endp_subflow_max) {
local = &locals[i];
@@ -534,17 +524,14 @@ fill_local_addresses_vec(struct mptcp_sock *msk, struct mptcp_addr_info *remote,
static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
{
+ u8 limit_add_addr_accepted = mptcp_pm_get_limit_add_addr_accepted(msk);
+ u8 limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
struct mptcp_pm_local locals[MPTCP_PM_ADDR_MAX];
struct sock *sk = (struct sock *)msk;
- unsigned int limit_extra_subflows;
- unsigned int limit_add_addr_accepted;
struct mptcp_addr_info remote;
bool sf_created = false;
int i, nr;
- limit_add_addr_accepted = mptcp_pm_get_limit_add_addr_accepted(msk);
- limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
-
pr_debug("accepted %d:%d remote family %d\n",
msk->pm.add_addr_accepted, limit_add_addr_accepted,
msk->pm.remote.family);
@@ -586,7 +573,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
void mptcp_pm_nl_rm_addr(struct mptcp_sock *msk, u8 rm_id)
{
if (rm_id && WARN_ON_ONCE(msk->pm.add_addr_accepted == 0)) {
- unsigned int limit_add_addr_accepted =
+ u8 limit_add_addr_accepted =
mptcp_pm_get_limit_add_addr_accepted(msk);
/* Note: if the subflow has been closed before, this
@@ -617,8 +604,8 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
bool needs_id, bool replace)
{
struct mptcp_pm_addr_entry *cur, *del_entry = NULL;
- unsigned int addr_max;
int ret = -EINVAL;
+ u8 addr_max;
spin_lock_bh(&pernet->lock);
/* to keep the code simple, don't do IDR-like allocation for address ID,
@@ -1064,8 +1051,8 @@ int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
{
struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
struct mptcp_pm_addr_entry addr, *entry;
- unsigned int addr_max;
struct nlattr *attr;
+ u8 addr_max;
int ret;
if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index a65aedc84ef5db3d8fc489b39d85ca56eb50e30e..027d717ef7cffe150f8de7b3b404916a1899537a 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1177,10 +1177,10 @@ void __init mptcp_pm_userspace_register(void);
void __init mptcp_pm_nl_init(void);
void mptcp_pm_worker(struct mptcp_sock *msk);
void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
-unsigned int mptcp_pm_get_endp_signal_max(const struct mptcp_sock *msk);
-unsigned int mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk);
-unsigned int mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk);
-unsigned int mptcp_pm_get_limit_extra_subflows(const struct mptcp_sock *msk);
+u8 mptcp_pm_get_endp_signal_max(const struct mptcp_sock *msk);
+u8 mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk);
+u8 mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk);
+u8 mptcp_pm_get_limit_extra_subflows(const struct mptcp_sock *msk);
/* called under PM lock */
static inline void __mptcp_pm_close_subflow(struct mptcp_sock *msk)
--
2.51.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 3/6] mptcp: pm: in-kernel: compare IDs instead of addresses
2025-09-22 22:23 [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 1/6] mptcp: pm: in-kernel: remove stale_loss_cnt Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 2/6] mptcp: pm: in-kernel: reduce pernet struct size Matthieu Baerts (NGI0)
@ 2025-09-22 22:23 ` Matthieu Baerts (NGI0)
2025-09-23 3:27 ` Mat Martineau
2025-09-22 22:23 ` [PATCH mptcp-next 4/6] Squash to "mptcp: pm: in-kernel: usable client side with C-flag" Matthieu Baerts (NGI0)
` (3 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Matthieu Baerts (NGI0) @ 2025-09-22 22:23 UTC (permalink / raw)
To: MPTCP Upstream; +Cc: Matthieu Baerts (NGI0)
When receiving an ADD_ADDR right after the 3WHS, the connection will
switch to 'fully established'. It means the MPTCP worker will be called
to treat two events, in this order: ADD_ADDR_RECEIVED, PM_ESTABLISHED.
The MPTCP endpoints cannot have the ID 0, because it is reserved to the
address and port used by the initial subflow. To be able to deal with
this case in different places, msk->mpc_endpoint_id contains the
endpoint ID linked to the initial subflow. This variable was only set
when treating the first PM_ESTABLISHED event, after ADD_ADDR_RECEIVED.
That's why in fill_local_addresses_vec(), the endpoint addresses were
compared with the one of the initial subflow, instead of only comparing
the IDs.
Instead, msk->mpc_endpoint_id is now set when treating ADD_ADDR_RECEIVED
as well, if needed, then the IDs can be compared.
To be able to do so, the code doing that is now in a dedicated helper,
and called from the functions linked to the two actions.
While at it, mptcp_endp_get_local_id() has also been moved up, next to
this new helper, because they are linked, and to be able to use it in
fill_local_addresses_vec() in the next commit.
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
net/mptcp/pm_kernel.c | 81 +++++++++++++++++++++++++++------------------------
1 file changed, 43 insertions(+), 38 deletions(-)
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index ae6ab3178c226fe3ec66d156bb0dc919075043e5..7be454196a41f625100a6e0d6cbf1ee360dff553 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -268,6 +268,44 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
return NULL;
}
+static u8 mptcp_endp_get_local_id(struct mptcp_sock *msk,
+ const struct mptcp_addr_info *addr)
+{
+ return msk->mpc_endpoint_id == addr->id ? 0 : addr->id;
+}
+
+static void mptcp_set_mpc_endpoint_id(struct mptcp_sock *msk)
+{
+ struct mptcp_subflow_context *subflow;
+ struct mptcp_pm_addr_entry *entry;
+ struct mptcp_addr_info mpc_addr;
+ struct pm_nl_pernet *pernet;
+ bool backup = false;
+
+ /* do lazy endpoint usage accounting for the MPC subflows */
+ if (likely(msk->pm.status & BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED)) ||
+ !msk->first)
+ return;
+
+ subflow = mptcp_subflow_ctx(msk->first);
+ pernet = pm_nl_get_pernet_from_msk(msk);
+
+ mptcp_local_address((struct sock_common *)msk->first, &mpc_addr);
+ rcu_read_lock();
+ entry = __lookup_addr(pernet, &mpc_addr);
+ 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();
+
+ if (backup)
+ mptcp_pm_send_ack(msk, subflow, true, backup);
+
+ msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
+}
+
static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
{
u8 limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
@@ -278,28 +316,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
bool signal_and_subflow = false;
struct mptcp_pm_local local;
- /* 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;
- bool backup = false;
-
- mptcp_local_address((struct sock_common *)msk->first, &mpc_addr);
- rcu_read_lock();
- entry = __lookup_addr(pernet, &mpc_addr);
- 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();
-
- if (backup)
- mptcp_pm_send_ack(msk, subflow, true, backup);
-
- msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
- }
+ mptcp_set_mpc_endpoint_id(msk);
pr_debug("local %d:%d signal %d:%d subflows %d:%d\n",
msk->pm.local_addr_used, endp_subflow_max,
@@ -396,12 +413,9 @@ fill_local_addresses_vec_fullmesh(struct mptcp_sock *msk,
struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
struct sock *sk = (struct sock *)msk;
struct mptcp_pm_addr_entry *entry;
- struct mptcp_addr_info mpc_addr;
struct mptcp_pm_local *local;
int i = 0;
- mptcp_local_address((struct sock_common *)msk, &mpc_addr);
-
rcu_read_lock();
list_for_each_entry_rcu(entry, &pernet->endp_list, list) {
if (!(entry->flags & MPTCP_PM_ADDR_FLAG_FULLMESH))
@@ -419,8 +433,7 @@ fill_local_addresses_vec_fullmesh(struct mptcp_sock *msk,
__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
/* Special case for ID0: set the correct ID */
- if (mptcp_addresses_equal(&local->addr, &mpc_addr,
- local->addr.port))
+ if (local->addr.id == msk->mpc_endpoint_id)
local->addr.id = 0;
msk->pm.extra_subflows++;
@@ -443,12 +456,9 @@ fill_local_addresses_vec_c_flag(struct mptcp_sock *msk,
struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
u8 endp_subflow_max = mptcp_pm_get_endp_subflow_max(msk);
struct sock *sk = (struct sock *)msk;
- struct mptcp_addr_info mpc_addr;
struct mptcp_pm_local *local;
int i = 0;
- mptcp_local_address((struct sock_common *)msk, &mpc_addr);
-
while (msk->pm.local_addr_used < endp_subflow_max) {
local = &locals[i];
@@ -460,8 +470,7 @@ fill_local_addresses_vec_c_flag(struct mptcp_sock *msk,
if (!mptcp_pm_addr_families_match(sk, &local->addr, remote))
continue;
- if (mptcp_addresses_equal(&local->addr, &mpc_addr,
- local->addr.port))
+ if (local->addr.id == msk->mpc_endpoint_id)
continue;
msk->pm.local_addr_used++;
@@ -507,6 +516,8 @@ fill_local_addresses_vec(struct mptcp_sock *msk, struct mptcp_addr_info *remote,
bool c_flag_case = remote->id && mptcp_pm_add_addr_c_flag_case(msk);
int i;
+ mptcp_set_mpc_endpoint_id(msk);
+
/* If there is at least one MPTCP endpoint with a fullmesh flag */
i = fill_local_addresses_vec_fullmesh(msk, remote, locals, c_flag_case);
if (i)
@@ -927,12 +938,6 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info)
return ret;
}
-static u8 mptcp_endp_get_local_id(struct mptcp_sock *msk,
- const struct mptcp_addr_info *addr)
-{
- return msk->mpc_endpoint_id == addr->id ? 0 : addr->id;
-}
-
static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
const struct mptcp_addr_info *addr,
bool force)
--
2.51.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 4/6] Squash to "mptcp: pm: in-kernel: usable client side with C-flag"
2025-09-22 22:23 [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
` (2 preceding siblings ...)
2025-09-22 22:23 ` [PATCH mptcp-next 3/6] mptcp: pm: in-kernel: compare IDs instead of addresses Matthieu Baerts (NGI0)
@ 2025-09-22 22:23 ` Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 5/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
` (2 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Matthieu Baerts (NGI0) @ 2025-09-22 22:23 UTC (permalink / raw)
To: MPTCP Upstream; +Cc: Matthieu Baerts (NGI0)
In this special case (fullmesh + subflow + c-flag), local_addr_used
should be incremented for new subflows not involving local ID0.
Similar to what is done when receiving an ADD_ADR in the non-fullmesh
case, or in the subflow only case.
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
net/mptcp/pm_kernel.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 7be454196a41f625100a6e0d6cbf1ee360dff553..fbd0a4ade8469ee75d99083bf640ad91a6fb714e 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -429,9 +429,13 @@ fill_local_addresses_vec_fullmesh(struct mptcp_sock *msk,
local->flags = entry->flags;
local->ifindex = entry->ifindex;
- if (c_flag_case && (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW))
+ if (c_flag_case && (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)) {
__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
+ if (local->addr.id != msk->mpc_endpoint_id)
+ msk->pm.local_addr_used++;
+ }
+
/* Special case for ID0: set the correct ID */
if (local->addr.id == msk->mpc_endpoint_id)
local->addr.id = 0;
--
2.51.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 5/6] mptcp: pm: in-kernel: add 'address' endpoints
2025-09-22 22:23 [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
` (3 preceding siblings ...)
2025-09-22 22:23 ` [PATCH mptcp-next 4/6] Squash to "mptcp: pm: in-kernel: usable client side with C-flag" Matthieu Baerts (NGI0)
@ 2025-09-22 22:23 ` Matthieu Baerts (NGI0)
2025-09-23 5:17 ` Mat Martineau
2025-09-22 22:23 ` [PATCH mptcp-next 6/6] selftests: mptcp: join: validate new " Matthieu Baerts (NGI0)
2025-09-23 0:20 ` [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add " MPTCP CI
6 siblings, 1 reply; 13+ messages in thread
From: Matthieu Baerts (NGI0) @ 2025-09-22 22:23 UTC (permalink / raw)
To: MPTCP Upstream; +Cc: Matthieu Baerts (NGI0)
Currently, upon the reception of an ADD_ADDR (and when the fullmesh flag
is not used), the in-kernel PM will create new subflows using the local
address the routing configuration will pick.
It would be easier to pick local addresses from a selected list of
endpoints, and use it only once, than relying on routing rules.
Use case: both the client (C) and the server (S) have two addresses (a
and b). The client establishes the connection between C(a) and S(a).
Once established, the server announces its additional address S(b). Once
received, the client connects to it using its second address C(b).
Compared to a situation without the 'address' endpoint for C(b), the
client didn't use this address C(b) to establish a subflow to the
server's primary address S(a). So at the end, we have:
C S
C(a) --- S(a)
C(b) --- S(b)
In case of a 3rd address on each side (C(c) and S(c)), upon the
reception of an ADD_ADDR with S(c), the client should not pick C(b)
because it has already been used. C(c) should then be used.
Note that this situation is currently possible if C doesn't add any
endpoint, but configure the routing in order to pick C(b) for the route
to S(b), and pick C(c) for the route to S(c). That doesn't sound very
practical because it means knowing in advance the IP addresses that
will be used and announced by the server.
In the code, the new endpoint type is added. Similar to the other
subflow types, an MPTCP_INFO counter is added. While at it, hole are now
commented in struct mptcp_info, to remember next time that these holes
can no longer be used.
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/503
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
include/uapi/linux/mptcp.h | 6 +++-
net/mptcp/pm_kernel.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++
net/mptcp/protocol.h | 1 +
net/mptcp/sockopt.c | 2 ++
4 files changed, 90 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 5ec996977b3fa2351222e6d01b814770b34348e9..65dc069e9063325ad2e1ffb1da21cc4a4b6efd32 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -39,6 +39,7 @@
#define MPTCP_PM_ADDR_FLAG_BACKUP _BITUL(2)
#define MPTCP_PM_ADDR_FLAG_FULLMESH _BITUL(3)
#define MPTCP_PM_ADDR_FLAG_IMPLICIT _BITUL(4)
+#define MPTCP_PM_ADDR_FLAG_ADDRESS _BITUL(5)
struct mptcp_info {
__u8 mptcpi_subflows;
@@ -51,6 +52,7 @@ struct mptcp_info {
#define mptcpi_endp_signal_max mptcpi_add_addr_signal_max
__u8 mptcpi_add_addr_accepted_max;
#define mptcpi_limit_add_addr_accepted mptcpi_add_addr_accepted_max
+ /* 16-bit hole that can no longer be filled */
__u32 mptcpi_flags;
__u32 mptcpi_token;
__u64 mptcpi_write_seq;
@@ -60,13 +62,15 @@ struct mptcp_info {
__u8 mptcpi_local_addr_max;
#define mptcpi_endp_subflow_max mptcpi_local_addr_max
__u8 mptcpi_csum_enabled;
+ /* 8-bit hole that can no longer be filled */
__u32 mptcpi_retransmits;
__u64 mptcpi_bytes_retrans;
__u64 mptcpi_bytes_sent;
__u64 mptcpi_bytes_received;
__u64 mptcpi_bytes_acked;
__u8 mptcpi_subflows_total;
- __u8 reserved[3];
+ __u8 mptcpi_endp_address_max;
+ __u8 reserved[2];
__u32 mptcpi_last_data_sent;
__u32 mptcpi_last_data_recv;
__u32 mptcpi_last_ack_recv;
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index fbd0a4ade8469ee75d99083bf640ad91a6fb714e..790dd7bc7f79e95a1fb73cbfb065087aa28f8f4b 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -21,6 +21,7 @@ struct pm_nl_pernet {
u8 endpoints;
u8 endp_signal_max;
u8 endp_subflow_max;
+ u8 endp_address_max;
u8 limit_add_addr_accepted;
u8 limit_extra_subflows;
u8 next_id;
@@ -61,6 +62,14 @@ u8 mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk)
}
EXPORT_SYMBOL_GPL(mptcp_pm_get_endp_subflow_max);
+u8 mptcp_pm_get_endp_address_max(const struct mptcp_sock *msk)
+{
+ struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+
+ return READ_ONCE(pernet->endp_address_max);
+}
+EXPORT_SYMBOL_GPL(mptcp_pm_get_endp_address_max);
+
u8 mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk)
{
struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
@@ -451,6 +460,66 @@ fill_local_addresses_vec_fullmesh(struct mptcp_sock *msk,
return i;
}
+static unsigned int
+fill_local_addresses_vec_address(struct mptcp_sock *msk,
+ struct mptcp_addr_info *remote,
+ struct mptcp_pm_local *locals)
+{
+ struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+ DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
+ struct mptcp_subflow_context *subflow;
+ struct sock *sk = (struct sock *)msk;
+ struct mptcp_pm_addr_entry *entry;
+ struct mptcp_pm_local *local;
+ int i = 0;
+
+ /* Forbid creation of new subflows matching existing ones, possibly
+ * already created by 'subflow' endpoints
+ */
+ bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
+ mptcp_for_each_subflow(msk, subflow) {
+ struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+ if ((1 << inet_sk_state_load(ssk)) &
+ (TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING | TCPF_CLOSE))
+ continue;
+
+ __set_bit(READ_ONCE(subflow->local_id), unavail_id);
+ }
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &pernet->endp_list, list) {
+ if (!(entry->flags & MPTCP_PM_ADDR_FLAG_ADDRESS))
+ continue;
+
+ if (!mptcp_pm_addr_families_match(sk, &entry->addr, remote))
+ continue;
+
+ if (test_bit(mptcp_endp_get_local_id(msk, &entry->addr),
+ unavail_id))
+ continue;
+
+ local = &locals[i];
+ local->addr = entry->addr;
+ local->flags = entry->flags;
+ local->ifindex = entry->ifindex;
+
+ if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
+ __clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
+
+ if (local->addr.id != msk->mpc_endpoint_id)
+ msk->pm.local_addr_used++;
+ }
+
+ msk->pm.extra_subflows++;
+ i++;
+ break;
+ }
+ rcu_read_unlock();
+
+ return i;
+}
+
static unsigned int
fill_local_addresses_vec_c_flag(struct mptcp_sock *msk,
struct mptcp_addr_info *remote,
@@ -527,6 +596,10 @@ fill_local_addresses_vec(struct mptcp_sock *msk, struct mptcp_addr_info *remote,
if (i)
return i;
+ /* If there is at least one MPTCP endpoint with an address flag */
+ if (mptcp_pm_get_endp_address_max(msk))
+ return fill_local_addresses_vec_address(msk, remote, locals);
+
/* Special case: peer sets the C flag, accept one ADD_ADDR if default
* limits are used -- accepting no ADD_ADDR -- and use subflow endpoints
*/
@@ -701,6 +774,10 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
addr_max = pernet->endp_subflow_max;
WRITE_ONCE(pernet->endp_subflow_max, addr_max + 1);
}
+ if (entry->flags & MPTCP_PM_ADDR_FLAG_ADDRESS) {
+ addr_max = pernet->endp_address_max;
+ WRITE_ONCE(pernet->endp_address_max, addr_max + 1);
+ }
pernet->endpoints++;
if (!entry->addr.port)
@@ -1095,6 +1172,10 @@ int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
addr_max = pernet->endp_subflow_max;
WRITE_ONCE(pernet->endp_subflow_max, addr_max - 1);
}
+ if (entry->flags & MPTCP_PM_ADDR_FLAG_ADDRESS) {
+ addr_max = pernet->endp_address_max;
+ WRITE_ONCE(pernet->endp_address_max, addr_max - 1);
+ }
pernet->endpoints--;
list_del_rcu(&entry->list);
@@ -1177,6 +1258,7 @@ static void __reset_counters(struct pm_nl_pernet *pernet)
{
WRITE_ONCE(pernet->endp_signal_max, 0);
WRITE_ONCE(pernet->endp_subflow_max, 0);
+ WRITE_ONCE(pernet->endp_address_max, 0);
pernet->endpoints = 0;
}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 027d717ef7cffe150f8de7b3b404916a1899537a..57e4db26e0ae1c5e82bc5a262ccb9d5e36508543 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1179,6 +1179,7 @@ void mptcp_pm_worker(struct mptcp_sock *msk);
void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
u8 mptcp_pm_get_endp_signal_max(const struct mptcp_sock *msk);
u8 mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk);
+u8 mptcp_pm_get_endp_address_max(const struct mptcp_sock *msk);
u8 mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk);
u8 mptcp_pm_get_limit_extra_subflows(const struct mptcp_sock *msk);
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 92a2a274262732a345b9ab185efd7da1f0a5773a..3cdc35323cc18de3585169fe729a51cab25a4cba 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -980,6 +980,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
mptcp_pm_get_limit_add_addr_accepted(msk);
info->mptcpi_endp_subflow_max =
mptcp_pm_get_endp_subflow_max(msk);
+ info->mptcpi_endp_address_max =
+ mptcp_pm_get_endp_address_max(msk);
}
if (__mptcp_check_fallback(msk))
--
2.51.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-next 6/6] selftests: mptcp: join: validate new 'address' endpoints
2025-09-22 22:23 [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
` (4 preceding siblings ...)
2025-09-22 22:23 ` [PATCH mptcp-next 5/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
@ 2025-09-22 22:23 ` Matthieu Baerts (NGI0)
2025-09-23 0:20 ` [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add " MPTCP CI
6 siblings, 0 replies; 13+ messages in thread
From: Matthieu Baerts (NGI0) @ 2025-09-22 22:23 UTC (permalink / raw)
To: MPTCP Upstream; +Cc: Matthieu Baerts (NGI0)
Here are a few sub-tests for mptcp_join.sh, validating the new 'address'
endpoint type.
In a setup where subflows created using the routing rules would be
rejected by the listener, and where the latter announces one IP address,
some cases are verified:
- Without any 'address' endpoints: no new subflows are created.
- With one 'address' endpoints: a second subflow is created.
- With multiple 'address' endpoints: 2 IPv4 subflows are created.
- With one 'address' endpoints, but the server announcing a second IP
address, only one subflow is created.
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
tools/testing/selftests/net/mptcp/mptcp_join.sh | 56 +++++++++++++++++++++++++
tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 9 ++++
2 files changed, 65 insertions(+)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index a94b3960ad5e009dbead66b6ff2aa01f70aa3e1f..b0d935e3c8381ea34e74c3878e869610b1a6fa41 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2320,6 +2320,61 @@ signal_address_tests()
fi
}
+address_endp_tests()
+{
+ # no address endpoints: routing rules are used
+ if reset_with_tcp_filter "without address endpoint" ns1 10.0.2.2 REJECT &&
+ mptcp_lib_kallsyms_has "mptcp_pm_get_endp_address_max$"; then
+ pm_nl_set_limits $ns1 0 2
+ pm_nl_set_limits $ns2 2 2
+ pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+ run_tests $ns1 $ns2 10.0.1.1
+ join_syn_tx=1 \
+ chk_join_nr 0 0 0
+ chk_add_nr 1 1
+ fi
+
+ # address endpoints: this endpoint is used
+ if reset_with_tcp_filter "with address endpoint" ns1 10.0.2.2 REJECT &&
+ mptcp_lib_kallsyms_has "mptcp_pm_get_endp_address_max$"; then
+ pm_nl_set_limits $ns1 0 2
+ pm_nl_set_limits $ns2 2 2
+ pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+ pm_nl_add_endpoint $ns2 10.0.3.2 flags address
+ run_tests $ns1 $ns2 10.0.1.1
+ chk_join_nr 1 1 1
+ chk_add_nr 1 1
+ fi
+
+ # address endpoints: these endpoints are used
+ if reset_with_tcp_filter "with multiple address endpoints" ns1 10.0.2.2 REJECT &&
+ mptcp_lib_kallsyms_has "mptcp_pm_get_endp_address_max$"; then
+ pm_nl_set_limits $ns1 0 2
+ pm_nl_set_limits $ns2 2 2
+ pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+ pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
+ pm_nl_add_endpoint $ns2 dead:beef:3::2 flags address
+ pm_nl_add_endpoint $ns2 10.0.3.2 flags address
+ pm_nl_add_endpoint $ns2 10.0.4.2 flags address
+ run_tests $ns1 $ns2 10.0.1.1
+ chk_join_nr 2 2 2
+ chk_add_nr 2 2
+ fi
+
+ # address endpoints: only one endpoint is used
+ if reset_with_tcp_filter "single address endpoints" ns1 10.0.2.2 REJECT &&
+ mptcp_lib_kallsyms_has "mptcp_pm_get_endp_address_max$"; then
+ pm_nl_set_limits $ns1 0 2
+ pm_nl_set_limits $ns2 2 2
+ pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+ pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
+ pm_nl_add_endpoint $ns2 10.0.3.2 flags address
+ run_tests $ns1 $ns2 10.0.1.1
+ chk_join_nr 1 1 1
+ chk_add_nr 2 2
+ fi
+}
+
link_failure_tests()
{
# accept and use add_addr with additional subflows and link loss
@@ -4109,6 +4164,7 @@ all_tests_sorted=(
f@subflows_tests
e@subflows_error_tests
s@signal_address_tests
+ A@address_endp_tests
l@link_failure_tests
t@add_addr_timeout_tests
r@remove_tests
diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index d4981b76693bbddca74169437a540ad6294cf1d5..9164c2e797bd003d933d274a54791b23928748f6 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -830,6 +830,8 @@ int add_addr(int fd, int pm_family, int argc, char *argv[])
flags |= MPTCP_PM_ADDR_FLAG_SUBFLOW;
else if (!strcmp(tok, "signal"))
flags |= MPTCP_PM_ADDR_FLAG_SIGNAL;
+ else if (!strcmp(tok, "address"))
+ flags |= MPTCP_PM_ADDR_FLAG_ADDRESS;
else if (!strcmp(tok, "backup"))
flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
else if (!strcmp(tok, "fullmesh"))
@@ -1018,6 +1020,13 @@ static void print_addr(struct rtattr *attrs, int len)
printf(",");
}
+ if (flags & MPTCP_PM_ADDR_FLAG_ADDRESS) {
+ printf("address");
+ flags &= ~MPTCP_PM_ADDR_FLAG_ADDRESS;
+ if (flags)
+ printf(",");
+ }
+
if (flags & MPTCP_PM_ADDR_FLAG_BACKUP) {
printf("backup");
flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP;
--
2.51.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints
2025-09-22 22:23 [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
` (5 preceding siblings ...)
2025-09-22 22:23 ` [PATCH mptcp-next 6/6] selftests: mptcp: join: validate new " Matthieu Baerts (NGI0)
@ 2025-09-23 0:20 ` MPTCP CI
6 siblings, 0 replies; 13+ messages in thread
From: MPTCP CI @ 2025-09-23 0:20 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: mptcp
Hi Matthieu,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/17930472338
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/0a3de317c9c0
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1005050
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-normal
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 (NGI0 Core)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 3/6] mptcp: pm: in-kernel: compare IDs instead of addresses
2025-09-22 22:23 ` [PATCH mptcp-next 3/6] mptcp: pm: in-kernel: compare IDs instead of addresses Matthieu Baerts (NGI0)
@ 2025-09-23 3:27 ` Mat Martineau
2025-09-23 8:09 ` Matthieu Baerts
0 siblings, 1 reply; 13+ messages in thread
From: Mat Martineau @ 2025-09-23 3:27 UTC (permalink / raw)
To: Matthieu Baerts (NGI0); +Cc: MPTCP Upstream
On Tue, 23 Sep 2025, Matthieu Baerts (NGI0) wrote:
> When receiving an ADD_ADDR right after the 3WHS, the connection will
> switch to 'fully established'. It means the MPTCP worker will be called
> to treat two events, in this order: ADD_ADDR_RECEIVED, PM_ESTABLISHED.
>
> The MPTCP endpoints cannot have the ID 0, because it is reserved to the
> address and port used by the initial subflow. To be able to deal with
> this case in different places, msk->mpc_endpoint_id contains the
> endpoint ID linked to the initial subflow. This variable was only set
> when treating the first PM_ESTABLISHED event, after ADD_ADDR_RECEIVED.
> That's why in fill_local_addresses_vec(), the endpoint addresses were
> compared with the one of the initial subflow, instead of only comparing
> the IDs.
>
> Instead, msk->mpc_endpoint_id is now set when treating ADD_ADDR_RECEIVED
> as well, if needed, then the IDs can be compared.
>
> To be able to do so, the code doing that is now in a dedicated helper,
> and called from the functions linked to the two actions.
>
> While at it, mptcp_endp_get_local_id() has also been moved up, next to
> this new helper, because they are linked, and to be able to use it in
> fill_local_addresses_vec() in the next commit.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> net/mptcp/pm_kernel.c | 81 +++++++++++++++++++++++++++------------------------
> 1 file changed, 43 insertions(+), 38 deletions(-)
>
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index ae6ab3178c226fe3ec66d156bb0dc919075043e5..7be454196a41f625100a6e0d6cbf1ee360dff553 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -268,6 +268,44 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
> return NULL;
> }
>
> +static u8 mptcp_endp_get_local_id(struct mptcp_sock *msk,
> + const struct mptcp_addr_info *addr)
> +{
> + return msk->mpc_endpoint_id == addr->id ? 0 : addr->id;
> +}
> +
> +static void mptcp_set_mpc_endpoint_id(struct mptcp_sock *msk)
Hi Matthieu -
I see how this was pulled out of the function below, but it is both
setting the id and taking action to send the MP_PRIO. Is there a better
name for what it's doing? mptcp_mpc_endpoint_setup() ? Or maybe handle
MP_PRIO in a separate helper?
> +{
> + struct mptcp_subflow_context *subflow;
> + struct mptcp_pm_addr_entry *entry;
> + struct mptcp_addr_info mpc_addr;
> + struct pm_nl_pernet *pernet;
> + bool backup = false;
> +
> + /* do lazy endpoint usage accounting for the MPC subflows */
> + if (likely(msk->pm.status & BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED)) ||
> + !msk->first)
> + return;
> +
> + subflow = mptcp_subflow_ctx(msk->first);
> + pernet = pm_nl_get_pernet_from_msk(msk);
> +
> + mptcp_local_address((struct sock_common *)msk->first, &mpc_addr);
> + rcu_read_lock();
> + entry = __lookup_addr(pernet, &mpc_addr);
> + 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();
> +
> + if (backup)
> + mptcp_pm_send_ack(msk, subflow, true, backup);
> +
> + msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
> +}
> +
> static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> {
> u8 limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
> @@ -278,28 +316,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> bool signal_and_subflow = false;
> struct mptcp_pm_local local;
>
> - /* 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;
> - bool backup = false;
> -
> - mptcp_local_address((struct sock_common *)msk->first, &mpc_addr);
> - rcu_read_lock();
> - entry = __lookup_addr(pernet, &mpc_addr);
> - 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();
> -
> - if (backup)
> - mptcp_pm_send_ack(msk, subflow, true, backup);
> -
> - msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
> - }
> + mptcp_set_mpc_endpoint_id(msk);
>
> pr_debug("local %d:%d signal %d:%d subflows %d:%d\n",
> msk->pm.local_addr_used, endp_subflow_max,
> @@ -396,12 +413,9 @@ fill_local_addresses_vec_fullmesh(struct mptcp_sock *msk,
> struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
> struct sock *sk = (struct sock *)msk;
> struct mptcp_pm_addr_entry *entry;
> - struct mptcp_addr_info mpc_addr;
> struct mptcp_pm_local *local;
> int i = 0;
>
> - mptcp_local_address((struct sock_common *)msk, &mpc_addr);
> -
> rcu_read_lock();
> list_for_each_entry_rcu(entry, &pernet->endp_list, list) {
> if (!(entry->flags & MPTCP_PM_ADDR_FLAG_FULLMESH))
> @@ -419,8 +433,7 @@ fill_local_addresses_vec_fullmesh(struct mptcp_sock *msk,
> __clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
>
> /* Special case for ID0: set the correct ID */
> - if (mptcp_addresses_equal(&local->addr, &mpc_addr,
> - local->addr.port))
> + if (local->addr.id == msk->mpc_endpoint_id)
> local->addr.id = 0;
>
> msk->pm.extra_subflows++;
> @@ -443,12 +456,9 @@ fill_local_addresses_vec_c_flag(struct mptcp_sock *msk,
> struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
> u8 endp_subflow_max = mptcp_pm_get_endp_subflow_max(msk);
> struct sock *sk = (struct sock *)msk;
> - struct mptcp_addr_info mpc_addr;
> struct mptcp_pm_local *local;
> int i = 0;
>
> - mptcp_local_address((struct sock_common *)msk, &mpc_addr);
> -
> while (msk->pm.local_addr_used < endp_subflow_max) {
> local = &locals[i];
>
> @@ -460,8 +470,7 @@ fill_local_addresses_vec_c_flag(struct mptcp_sock *msk,
> if (!mptcp_pm_addr_families_match(sk, &local->addr, remote))
> continue;
>
> - if (mptcp_addresses_equal(&local->addr, &mpc_addr,
> - local->addr.port))
> + if (local->addr.id == msk->mpc_endpoint_id)
> continue;
>
> msk->pm.local_addr_used++;
> @@ -507,6 +516,8 @@ fill_local_addresses_vec(struct mptcp_sock *msk, struct mptcp_addr_info *remote,
> bool c_flag_case = remote->id && mptcp_pm_add_addr_c_flag_case(msk);
> int i;
>
> + mptcp_set_mpc_endpoint_id(msk);
> +
What do you think about moving this before the call to
fill_addresses_vec() in mptcp_pm_nl_add_addr_received()? Again, taking
this action is different from filling the vector.
- Mat
> /* If there is at least one MPTCP endpoint with a fullmesh flag */
> i = fill_local_addresses_vec_fullmesh(msk, remote, locals, c_flag_case);
> if (i)
> @@ -927,12 +938,6 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info)
> return ret;
> }
>
> -static u8 mptcp_endp_get_local_id(struct mptcp_sock *msk,
> - const struct mptcp_addr_info *addr)
> -{
> - return msk->mpc_endpoint_id == addr->id ? 0 : addr->id;
> -}
> -
> static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
> const struct mptcp_addr_info *addr,
> bool force)
>
> --
> 2.51.0
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 5/6] mptcp: pm: in-kernel: add 'address' endpoints
2025-09-22 22:23 ` [PATCH mptcp-next 5/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
@ 2025-09-23 5:17 ` Mat Martineau
2025-09-23 9:05 ` Matthieu Baerts
0 siblings, 1 reply; 13+ messages in thread
From: Mat Martineau @ 2025-09-23 5:17 UTC (permalink / raw)
To: Matthieu Baerts (NGI0); +Cc: MPTCP Upstream
On Tue, 23 Sep 2025, Matthieu Baerts (NGI0) wrote:
> Currently, upon the reception of an ADD_ADDR (and when the fullmesh flag
> is not used), the in-kernel PM will create new subflows using the local
> address the routing configuration will pick.
>
> It would be easier to pick local addresses from a selected list of
> endpoints, and use it only once, than relying on routing rules.
>
> Use case: both the client (C) and the server (S) have two addresses (a
> and b). The client establishes the connection between C(a) and S(a).
> Once established, the server announces its additional address S(b). Once
> received, the client connects to it using its second address C(b).
> Compared to a situation without the 'address' endpoint for C(b), the
> client didn't use this address C(b) to establish a subflow to the
> server's primary address S(a). So at the end, we have:
>
> C S
> C(a) --- S(a)
> C(b) --- S(b)
>
> In case of a 3rd address on each side (C(c) and S(c)), upon the
> reception of an ADD_ADDR with S(c), the client should not pick C(b)
> because it has already been used. C(c) should then be used.
>
> Note that this situation is currently possible if C doesn't add any
> endpoint, but configure the routing in order to pick C(b) for the route
> to S(b), and pick C(c) for the route to S(c). That doesn't sound very
> practical because it means knowing in advance the IP addresses that
> will be used and announced by the server.
>
> In the code, the new endpoint type is added. Similar to the other
> subflow types, an MPTCP_INFO counter is added. While at it, hole are now
> commented in struct mptcp_info, to remember next time that these holes
> can no longer be used.
Hi Matthieu -
I think this patch brings up a few larger topics of discussion: path
manager strategy (in-kernel/userspace/bpf), interaction of in-kernel PM
flags, and (once again!) naming.
I'm not sure the reply chain for this patch is the right place to have the
discussion, but adding another in-kernel PM "mode" makes me think we need
a community-level (MPTCP) discussion on our path manager strategy. The
original plan was to have a single general in-kernel PM, and rely on
userspace/mptcpd for anything else. We've obviously made some
changes to that plan, adding fullmesh and having the BPF PM in
progress. This has made path management more complex to understand, use,
and maintain - so I want to be sure we are making a careful choice about
which PM features to add.
Our userspace API for the in-kernel PM also makes it complicated to
explain what happens when there is a mix of endpoint types. Typical use
would probably be reasonable (all fullmesh, all 'address', etc). But it's
good to avoid confusion, and more importantly bugs!
As for naming, unfortunately "address" is a very frequently used word in
our subsystem! In mptcpd the similar plugin is called "sspi" (single
subflow per interface). I'm definitely open to other ideas that are
identifiable and descriptive.
I think there are good use cases for this feature, that's why we included
the similar feature in mptcpd! I'd like to get our core group (you, me,
Geliang, and Paolo) aligned on a general direction for path management,
does that sound reasonable?
One technical question down below too
|
v
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/503
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> include/uapi/linux/mptcp.h | 6 +++-
> net/mptcp/pm_kernel.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++
> net/mptcp/protocol.h | 1 +
> net/mptcp/sockopt.c | 2 ++
> 4 files changed, 90 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index 5ec996977b3fa2351222e6d01b814770b34348e9..65dc069e9063325ad2e1ffb1da21cc4a4b6efd32 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -39,6 +39,7 @@
> #define MPTCP_PM_ADDR_FLAG_BACKUP _BITUL(2)
> #define MPTCP_PM_ADDR_FLAG_FULLMESH _BITUL(3)
> #define MPTCP_PM_ADDR_FLAG_IMPLICIT _BITUL(4)
> +#define MPTCP_PM_ADDR_FLAG_ADDRESS _BITUL(5)
>
> struct mptcp_info {
> __u8 mptcpi_subflows;
> @@ -51,6 +52,7 @@ struct mptcp_info {
> #define mptcpi_endp_signal_max mptcpi_add_addr_signal_max
> __u8 mptcpi_add_addr_accepted_max;
> #define mptcpi_limit_add_addr_accepted mptcpi_add_addr_accepted_max
> + /* 16-bit hole that can no longer be filled */
> __u32 mptcpi_flags;
> __u32 mptcpi_token;
> __u64 mptcpi_write_seq;
> @@ -60,13 +62,15 @@ struct mptcp_info {
> __u8 mptcpi_local_addr_max;
> #define mptcpi_endp_subflow_max mptcpi_local_addr_max
> __u8 mptcpi_csum_enabled;
> + /* 8-bit hole that can no longer be filled */
> __u32 mptcpi_retransmits;
> __u64 mptcpi_bytes_retrans;
> __u64 mptcpi_bytes_sent;
> __u64 mptcpi_bytes_received;
> __u64 mptcpi_bytes_acked;
> __u8 mptcpi_subflows_total;
> - __u8 reserved[3];
> + __u8 mptcpi_endp_address_max;
> + __u8 reserved[2];
> __u32 mptcpi_last_data_sent;
> __u32 mptcpi_last_data_recv;
> __u32 mptcpi_last_ack_recv;
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index fbd0a4ade8469ee75d99083bf640ad91a6fb714e..790dd7bc7f79e95a1fb73cbfb065087aa28f8f4b 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -21,6 +21,7 @@ struct pm_nl_pernet {
> u8 endpoints;
> u8 endp_signal_max;
> u8 endp_subflow_max;
> + u8 endp_address_max;
> u8 limit_add_addr_accepted;
> u8 limit_extra_subflows;
> u8 next_id;
> @@ -61,6 +62,14 @@ u8 mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk)
> }
> EXPORT_SYMBOL_GPL(mptcp_pm_get_endp_subflow_max);
>
> +u8 mptcp_pm_get_endp_address_max(const struct mptcp_sock *msk)
> +{
> + struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
> +
> + return READ_ONCE(pernet->endp_address_max);
> +}
> +EXPORT_SYMBOL_GPL(mptcp_pm_get_endp_address_max);
> +
> u8 mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk)
> {
> struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
> @@ -451,6 +460,66 @@ fill_local_addresses_vec_fullmesh(struct mptcp_sock *msk,
> return i;
> }
>
> +static unsigned int
> +fill_local_addresses_vec_address(struct mptcp_sock *msk,
> + struct mptcp_addr_info *remote,
> + struct mptcp_pm_local *locals)
> +{
> + struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
> + DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
> + struct mptcp_subflow_context *subflow;
> + struct sock *sk = (struct sock *)msk;
> + struct mptcp_pm_addr_entry *entry;
> + struct mptcp_pm_local *local;
> + int i = 0;
> +
> + /* Forbid creation of new subflows matching existing ones, possibly
> + * already created by 'subflow' endpoints
> + */
> + bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
> + mptcp_for_each_subflow(msk, subflow) {
> + struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> + if ((1 << inet_sk_state_load(ssk)) &
> + (TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING | TCPF_CLOSE))
> + continue;
> +
> + __set_bit(READ_ONCE(subflow->local_id), unavail_id);
> + }
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(entry, &pernet->endp_list, list) {
> + if (!(entry->flags & MPTCP_PM_ADDR_FLAG_ADDRESS))
> + continue;
> +
> + if (!mptcp_pm_addr_families_match(sk, &entry->addr, remote))
> + continue;
> +
> + if (test_bit(mptcp_endp_get_local_id(msk, &entry->addr),
> + unavail_id))
> + continue;
> +
> + local = &locals[i];
Looks like 'i' is always 0 here, since the only code path from here leads
to 'break'. Would be clearer to hardcode 0 and clarify the variable name
for the return value.
Thanks,
Mat
> + local->addr = entry->addr;
> + local->flags = entry->flags;
> + local->ifindex = entry->ifindex;
> +
> + if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
> + __clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
> +
> + if (local->addr.id != msk->mpc_endpoint_id)
> + msk->pm.local_addr_used++;
> + }
> +
> + msk->pm.extra_subflows++;
> + i++;
> + break;
> + }
> + rcu_read_unlock();
> +
> + return i;
> +}
> +
> static unsigned int
> fill_local_addresses_vec_c_flag(struct mptcp_sock *msk,
> struct mptcp_addr_info *remote,
> @@ -527,6 +596,10 @@ fill_local_addresses_vec(struct mptcp_sock *msk, struct mptcp_addr_info *remote,
> if (i)
> return i;
>
> + /* If there is at least one MPTCP endpoint with an address flag */
> + if (mptcp_pm_get_endp_address_max(msk))
> + return fill_local_addresses_vec_address(msk, remote, locals);
> +
> /* Special case: peer sets the C flag, accept one ADD_ADDR if default
> * limits are used -- accepting no ADD_ADDR -- and use subflow endpoints
> */
> @@ -701,6 +774,10 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
> addr_max = pernet->endp_subflow_max;
> WRITE_ONCE(pernet->endp_subflow_max, addr_max + 1);
> }
> + if (entry->flags & MPTCP_PM_ADDR_FLAG_ADDRESS) {
> + addr_max = pernet->endp_address_max;
> + WRITE_ONCE(pernet->endp_address_max, addr_max + 1);
> + }
>
> pernet->endpoints++;
> if (!entry->addr.port)
> @@ -1095,6 +1172,10 @@ int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
> addr_max = pernet->endp_subflow_max;
> WRITE_ONCE(pernet->endp_subflow_max, addr_max - 1);
> }
> + if (entry->flags & MPTCP_PM_ADDR_FLAG_ADDRESS) {
> + addr_max = pernet->endp_address_max;
> + WRITE_ONCE(pernet->endp_address_max, addr_max - 1);
> + }
>
> pernet->endpoints--;
> list_del_rcu(&entry->list);
> @@ -1177,6 +1258,7 @@ static void __reset_counters(struct pm_nl_pernet *pernet)
> {
> WRITE_ONCE(pernet->endp_signal_max, 0);
> WRITE_ONCE(pernet->endp_subflow_max, 0);
> + WRITE_ONCE(pernet->endp_address_max, 0);
> pernet->endpoints = 0;
> }
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 027d717ef7cffe150f8de7b3b404916a1899537a..57e4db26e0ae1c5e82bc5a262ccb9d5e36508543 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1179,6 +1179,7 @@ void mptcp_pm_worker(struct mptcp_sock *msk);
> void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
> u8 mptcp_pm_get_endp_signal_max(const struct mptcp_sock *msk);
> u8 mptcp_pm_get_endp_subflow_max(const struct mptcp_sock *msk);
> +u8 mptcp_pm_get_endp_address_max(const struct mptcp_sock *msk);
> u8 mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk);
> u8 mptcp_pm_get_limit_extra_subflows(const struct mptcp_sock *msk);
>
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 92a2a274262732a345b9ab185efd7da1f0a5773a..3cdc35323cc18de3585169fe729a51cab25a4cba 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -980,6 +980,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
> mptcp_pm_get_limit_add_addr_accepted(msk);
> info->mptcpi_endp_subflow_max =
> mptcp_pm_get_endp_subflow_max(msk);
> + info->mptcpi_endp_address_max =
> + mptcp_pm_get_endp_address_max(msk);
> }
>
> if (__mptcp_check_fallback(msk))
>
> --
> 2.51.0
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 3/6] mptcp: pm: in-kernel: compare IDs instead of addresses
2025-09-23 3:27 ` Mat Martineau
@ 2025-09-23 8:09 ` Matthieu Baerts
0 siblings, 0 replies; 13+ messages in thread
From: Matthieu Baerts @ 2025-09-23 8:09 UTC (permalink / raw)
To: Mat Martineau; +Cc: MPTCP Upstream
Hi Mat,
On 23/09/2025 04:27, Mat Martineau wrote:
> On Tue, 23 Sep 2025, Matthieu Baerts (NGI0) wrote:
>
>> When receiving an ADD_ADDR right after the 3WHS, the connection will
>> switch to 'fully established'. It means the MPTCP worker will be called
>> to treat two events, in this order: ADD_ADDR_RECEIVED, PM_ESTABLISHED.
>>
>> The MPTCP endpoints cannot have the ID 0, because it is reserved to the
>> address and port used by the initial subflow. To be able to deal with
>> this case in different places, msk->mpc_endpoint_id contains the
>> endpoint ID linked to the initial subflow. This variable was only set
>> when treating the first PM_ESTABLISHED event, after ADD_ADDR_RECEIVED.
>> That's why in fill_local_addresses_vec(), the endpoint addresses were
>> compared with the one of the initial subflow, instead of only comparing
>> the IDs.
>>
>> Instead, msk->mpc_endpoint_id is now set when treating ADD_ADDR_RECEIVED
>> as well, if needed, then the IDs can be compared.
>>
>> To be able to do so, the code doing that is now in a dedicated helper,
>> and called from the functions linked to the two actions.
>>
>> While at it, mptcp_endp_get_local_id() has also been moved up, next to
>> this new helper, because they are linked, and to be able to use it in
>> fill_local_addresses_vec() in the next commit.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> net/mptcp/pm_kernel.c | 81 ++++++++++++++++++++++++++
>> +------------------------
>> 1 file changed, 43 insertions(+), 38 deletions(-)
>>
>> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
>> index
>> ae6ab3178c226fe3ec66d156bb0dc919075043e5..7be454196a41f625100a6e0d6cbf1ee360dff553 100644
>> --- a/net/mptcp/pm_kernel.c
>> +++ b/net/mptcp/pm_kernel.c
>> @@ -268,6 +268,44 @@ __lookup_addr(struct pm_nl_pernet *pernet, const
>> struct mptcp_addr_info *info)
>> return NULL;
>> }
>>
>> +static u8 mptcp_endp_get_local_id(struct mptcp_sock *msk,
>> + const struct mptcp_addr_info *addr)
>> +{
>> + return msk->mpc_endpoint_id == addr->id ? 0 : addr->id;
>> +}
>> +
>> +static void mptcp_set_mpc_endpoint_id(struct mptcp_sock *msk)
>
> Hi Matthieu -
>
> I see how this was pulled out of the function below, but it is both
> setting the id and taking action to send the MP_PRIO. Is there a better
> name for what it's doing? mptcp_mpc_endpoint_setup() ?
Good idea!
> Or maybe handle MP_PRIO in a separate helper?
Mmh, it should be done only once, when looking at the MPC endpoint,
under the same conditions. I think it might be easier to keep it there,
but rename the helper, no?
>> +{
>> + struct mptcp_subflow_context *subflow;
>> + struct mptcp_pm_addr_entry *entry;
>> + struct mptcp_addr_info mpc_addr;
>> + struct pm_nl_pernet *pernet;
>> + bool backup = false;
>> +
>> + /* do lazy endpoint usage accounting for the MPC subflows */
>> + if (likely(msk->pm.status & BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED)) ||
>> + !msk->first)
>> + return;
>> +
>> + subflow = mptcp_subflow_ctx(msk->first);
>> + pernet = pm_nl_get_pernet_from_msk(msk);
>> +
>> + mptcp_local_address((struct sock_common *)msk->first, &mpc_addr);
>> + rcu_read_lock();
>> + entry = __lookup_addr(pernet, &mpc_addr);
>> + 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();
>> +
>> + if (backup)
>> + mptcp_pm_send_ack(msk, subflow, true, backup);
>> +
>> + msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
>> +}
>> +
>> static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock
>> *msk)
>> {
>> u8 limit_extra_subflows = mptcp_pm_get_limit_extra_subflows(msk);
>> @@ -278,28 +316,7 @@ static void
>> mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>> bool signal_and_subflow = false;
>> struct mptcp_pm_local local;
>>
>> - /* 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;
>> - bool backup = false;
>> -
>> - mptcp_local_address((struct sock_common *)msk->first,
>> &mpc_addr);
>> - rcu_read_lock();
>> - entry = __lookup_addr(pernet, &mpc_addr);
>> - 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();
>> -
>> - if (backup)
>> - mptcp_pm_send_ack(msk, subflow, true, backup);
>> -
>> - msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
>> - }
>> + mptcp_set_mpc_endpoint_id(msk);
>>
>> pr_debug("local %d:%d signal %d:%d subflows %d:%d\n",
>> msk->pm.local_addr_used, endp_subflow_max,
>> @@ -396,12 +413,9 @@ fill_local_addresses_vec_fullmesh(struct
>> mptcp_sock *msk,
>> struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
>> struct sock *sk = (struct sock *)msk;
>> struct mptcp_pm_addr_entry *entry;
>> - struct mptcp_addr_info mpc_addr;
>> struct mptcp_pm_local *local;
>> int i = 0;
>>
>> - mptcp_local_address((struct sock_common *)msk, &mpc_addr);
>> -
>> rcu_read_lock();
>> list_for_each_entry_rcu(entry, &pernet->endp_list, list) {
>> if (!(entry->flags & MPTCP_PM_ADDR_FLAG_FULLMESH))
>> @@ -419,8 +433,7 @@ fill_local_addresses_vec_fullmesh(struct
>> mptcp_sock *msk,
>> __clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
>>
>> /* Special case for ID0: set the correct ID */
>> - if (mptcp_addresses_equal(&local->addr, &mpc_addr,
>> - local->addr.port))
>> + if (local->addr.id == msk->mpc_endpoint_id)
>> local->addr.id = 0;
>>
>> msk->pm.extra_subflows++;
>> @@ -443,12 +456,9 @@ fill_local_addresses_vec_c_flag(struct mptcp_sock
>> *msk,
>> struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
>> u8 endp_subflow_max = mptcp_pm_get_endp_subflow_max(msk);
>> struct sock *sk = (struct sock *)msk;
>> - struct mptcp_addr_info mpc_addr;
>> struct mptcp_pm_local *local;
>> int i = 0;
>>
>> - mptcp_local_address((struct sock_common *)msk, &mpc_addr);
>> -
>> while (msk->pm.local_addr_used < endp_subflow_max) {
>> local = &locals[i];
>>
>> @@ -460,8 +470,7 @@ fill_local_addresses_vec_c_flag(struct mptcp_sock
>> *msk,
>> if (!mptcp_pm_addr_families_match(sk, &local->addr, remote))
>> continue;
>>
>> - if (mptcp_addresses_equal(&local->addr, &mpc_addr,
>> - local->addr.port))
>> + if (local->addr.id == msk->mpc_endpoint_id)
>> continue;
>>
>> msk->pm.local_addr_used++;
>> @@ -507,6 +516,8 @@ fill_local_addresses_vec(struct mptcp_sock *msk,
>> struct mptcp_addr_info *remote,
>> bool c_flag_case = remote->id && mptcp_pm_add_addr_c_flag_case(msk);
>> int i;
>>
>> + mptcp_set_mpc_endpoint_id(msk);
>> +
>
> What do you think about moving this before the call to
> fill_addresses_vec() in mptcp_pm_nl_add_addr_received()? Again, taking
> this action is different from filling the vector.
Good idea! I was so focus working around fill_addresses_vec(), I forgot
it was called from mptcp_pm_nl_add_addr_received()!
>
>> /* If there is at least one MPTCP endpoint with a fullmesh flag */
>> i = fill_local_addresses_vec_fullmesh(msk, remote, locals,
>> c_flag_case);
>> if (i)
>> @@ -927,12 +938,6 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff
>> *skb, struct genl_info *info)
>> return ret;
>> }
>>
>> -static u8 mptcp_endp_get_local_id(struct mptcp_sock *msk,
>> - const struct mptcp_addr_info *addr)
>> -{
>> - return msk->mpc_endpoint_id == addr->id ? 0 : addr->id;
>> -}
>> -
>> static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
>> const struct mptcp_addr_info *addr,
>> bool force)
>>
>> --
>> 2.51.0
>>
>>
>>
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 5/6] mptcp: pm: in-kernel: add 'address' endpoints
2025-09-23 5:17 ` Mat Martineau
@ 2025-09-23 9:05 ` Matthieu Baerts
2025-09-23 22:32 ` Mat Martineau
0 siblings, 1 reply; 13+ messages in thread
From: Matthieu Baerts @ 2025-09-23 9:05 UTC (permalink / raw)
To: Mat Martineau; +Cc: MPTCP Upstream
Hi Mat,
On 23/09/2025 06:17, Mat Martineau wrote:
> On Tue, 23 Sep 2025, Matthieu Baerts (NGI0) wrote:
>
>> Currently, upon the reception of an ADD_ADDR (and when the fullmesh flag
>> is not used), the in-kernel PM will create new subflows using the local
>> address the routing configuration will pick.
>>
>> It would be easier to pick local addresses from a selected list of
>> endpoints, and use it only once, than relying on routing rules.
>>
>> Use case: both the client (C) and the server (S) have two addresses (a
>> and b). The client establishes the connection between C(a) and S(a).
>> Once established, the server announces its additional address S(b). Once
>> received, the client connects to it using its second address C(b).
>> Compared to a situation without the 'address' endpoint for C(b), the
>> client didn't use this address C(b) to establish a subflow to the
>> server's primary address S(a). So at the end, we have:
>>
>> C S
>> C(a) --- S(a)
>> C(b) --- S(b)
>>
>> In case of a 3rd address on each side (C(c) and S(c)), upon the
>> reception of an ADD_ADDR with S(c), the client should not pick C(b)
>> because it has already been used. C(c) should then be used.
>>
>> Note that this situation is currently possible if C doesn't add any
>> endpoint, but configure the routing in order to pick C(b) for the route
>> to S(b), and pick C(c) for the route to S(c). That doesn't sound very
>> practical because it means knowing in advance the IP addresses that
>> will be used and announced by the server.
>>
>> In the code, the new endpoint type is added. Similar to the other
>> subflow types, an MPTCP_INFO counter is added. While at it, hole are now
>> commented in struct mptcp_info, to remember next time that these holes
>> can no longer be used.
>
> Hi Matthieu -
>
> I think this patch brings up a few larger topics of discussion: path
> manager strategy (in-kernel/userspace/bpf), interaction of in-kernel PM
> flags, and (once again!) naming.
>
> I'm not sure the reply chain for this patch is the right place to have
> the discussion, but adding another in-kernel PM "mode" makes me think we
> need a community-level (MPTCP) discussion on our path manager strategy.
> The original plan was to have a single general in-kernel PM, and rely on
> userspace/mptcpd for anything else. We've obviously made some changes to
> that plan, adding fullmesh and having the BPF PM in progress. This has
> made path management more complex to understand, use, and maintain - so
> I want to be sure we are making a careful choice about which PM features
> to add.
Good point!
(Regarding the BPF PM, it is similar to the userspace PM, but can be
used in environment handling loads of connections in parallel ; and it
introduces a better separation between the different PMs, which makes it
worth it for the maintenance to me.)
> Our userspace API for the in-kernel PM also makes it complicated to
> explain what happens when there is a mix of endpoint types. Typical use
> would probably be reasonable (all fullmesh, all 'address', etc). But
> it's good to avoid confusion, and more importantly bugs!
Indeed, I fixed quite a few issues and inconsistencies last year, around
the same time I opened this issue #503.
I have to admit that the 'fullmesh' mode added quite a bit of unexpected
complexity for such a niche use-case (but it allows "workarounds"). I
think the new mode suggested here is simple, targeting one specific part
of the code (an ADD_ADDR is received) and fix the lack of control on
what source IP address is used to create new subflows when an ADD_ADDR
is received. Not as invasive as the 'fullmesh' one.
> As for naming, unfortunately "address" is a very frequently used word in
> our subsystem! In mptcpd the similar plugin is called "sspi" (single
> subflow per interface). I'm definitely open to other ideas that are
> identifiable and descriptive.
Me too! I initially picked "add-addr", but I wasn't happy with the minus
sign, nor by its name. I guess a good name could be "endpoint used when
an ADD_ADDR is received", or maybe "received-add-addr", but that seems
too long, and more than one word.
I ended up picking "address", because it is short, and similar to
"signal" and "subflow" (which are not very clear, but hard to do better
with one word...).
> I think there are good use cases for this feature, that's why we
> included the similar feature in mptcpd! I'd like to get our core group
> (you, me, Geliang, and Paolo) aligned on a general direction for path
> management, does that sound reasonable?
Yes indeed, it is important.
To be honest, I thought we already had this discussion at the meeting
following the opening of this ticket #503, but that was a bit more than
one year ago. I started working on it because the lack of control on
what source IP address is used to create new subflows when an ADD_ADDR
is received came back in a few discussions, even prior #503. Even
recently when a client wanted to use dedicated interface per path. It
feels like something is missing in this in-kernel Netlink API to use
endpoints to create subflows to addresses announced by the server.
But yes, we are exposing a new option to the userspace, and if it is
accepted -- and not modified/reverted in the new 10 weeks -- we will
have to maintain it for a long time. So better not to get it wrong. I
think it is "self-contain" and worth it. I would like a better name than
"address", but I didn't find one. At the end, I'm happy with it as it is
similar to the others and will have a longer description in the doc, but
I'm open to another name :)
> One technical question down below too
> |
> v
>
>
>>
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/503
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> include/uapi/linux/mptcp.h | 6 +++-
>> net/mptcp/pm_kernel.c | 82 ++++++++++++++++++++++++++++++++++++++
>> ++++++++
>> net/mptcp/protocol.h | 1 +
>> net/mptcp/sockopt.c | 2 ++
>> 4 files changed, 90 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
>> index
>> 5ec996977b3fa2351222e6d01b814770b34348e9..65dc069e9063325ad2e1ffb1da21cc4a4b6efd32 100644
>> --- a/include/uapi/linux/mptcp.h
>> +++ b/include/uapi/linux/mptcp.h
>> @@ -39,6 +39,7 @@
>> #define MPTCP_PM_ADDR_FLAG_BACKUP _BITUL(2)
>> #define MPTCP_PM_ADDR_FLAG_FULLMESH _BITUL(3)
>> #define MPTCP_PM_ADDR_FLAG_IMPLICIT _BITUL(4)
>> +#define MPTCP_PM_ADDR_FLAG_ADDRESS _BITUL(5)
>>
>> struct mptcp_info {
>> __u8 mptcpi_subflows;
>> @@ -51,6 +52,7 @@ struct mptcp_info {
>> #define mptcpi_endp_signal_max mptcpi_add_addr_signal_max
>> __u8 mptcpi_add_addr_accepted_max;
>> #define mptcpi_limit_add_addr_accepted mptcpi_add_addr_accepted_max
>> + /* 16-bit hole that can no longer be filled */
>> __u32 mptcpi_flags;
>> __u32 mptcpi_token;
>> __u64 mptcpi_write_seq;
>> @@ -60,13 +62,15 @@ struct mptcp_info {
>> __u8 mptcpi_local_addr_max;
>> #define mptcpi_endp_subflow_max mptcpi_local_addr_max
>> __u8 mptcpi_csum_enabled;
>> + /* 8-bit hole that can no longer be filled */
>> __u32 mptcpi_retransmits;
>> __u64 mptcpi_bytes_retrans;
>> __u64 mptcpi_bytes_sent;
>> __u64 mptcpi_bytes_received;
>> __u64 mptcpi_bytes_acked;
>> __u8 mptcpi_subflows_total;
>> - __u8 reserved[3];
>> + __u8 mptcpi_endp_address_max;
>> + __u8 reserved[2];
>> __u32 mptcpi_last_data_sent;
>> __u32 mptcpi_last_data_recv;
>> __u32 mptcpi_last_ack_recv;
>> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
>> index
>> fbd0a4ade8469ee75d99083bf640ad91a6fb714e..790dd7bc7f79e95a1fb73cbfb065087aa28f8f4b 100644
>> --- a/net/mptcp/pm_kernel.c
>> +++ b/net/mptcp/pm_kernel.c
>> @@ -21,6 +21,7 @@ struct pm_nl_pernet {
>> u8 endpoints;
>> u8 endp_signal_max;
>> u8 endp_subflow_max;
>> + u8 endp_address_max;
>> u8 limit_add_addr_accepted;
>> u8 limit_extra_subflows;
>> u8 next_id;
>> @@ -61,6 +62,14 @@ u8 mptcp_pm_get_endp_subflow_max(const struct
>> mptcp_sock *msk)
>> }
>> EXPORT_SYMBOL_GPL(mptcp_pm_get_endp_subflow_max);
>>
>> +u8 mptcp_pm_get_endp_address_max(const struct mptcp_sock *msk)
>> +{
>> + struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
>> +
>> + return READ_ONCE(pernet->endp_address_max);
>> +}
>> +EXPORT_SYMBOL_GPL(mptcp_pm_get_endp_address_max);
>> +
>> u8 mptcp_pm_get_limit_add_addr_accepted(const struct mptcp_sock *msk)
>> {
>> struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
>> @@ -451,6 +460,66 @@ fill_local_addresses_vec_fullmesh(struct
>> mptcp_sock *msk,
>> return i;
>> }
>>
>> +static unsigned int
>> +fill_local_addresses_vec_address(struct mptcp_sock *msk,
>> + struct mptcp_addr_info *remote,
>> + struct mptcp_pm_local *locals)
>> +{
>> + struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
>> + DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
>> + struct mptcp_subflow_context *subflow;
>> + struct sock *sk = (struct sock *)msk;
>> + struct mptcp_pm_addr_entry *entry;
>> + struct mptcp_pm_local *local;
>> + int i = 0;
>> +
>> + /* Forbid creation of new subflows matching existing ones, possibly
>> + * already created by 'subflow' endpoints
>> + */
>> + bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
>> + mptcp_for_each_subflow(msk, subflow) {
>> + struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>> +
>> + if ((1 << inet_sk_state_load(ssk)) &
>> + (TCPF_FIN_WAIT1 | TCPF_FIN_WAIT2 | TCPF_CLOSING |
>> TCPF_CLOSE))
>> + continue;
>> +
>> + __set_bit(READ_ONCE(subflow->local_id), unavail_id);
>> + }
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(entry, &pernet->endp_list, list) {
>> + if (!(entry->flags & MPTCP_PM_ADDR_FLAG_ADDRESS))
>> + continue;
>> +
>> + if (!mptcp_pm_addr_families_match(sk, &entry->addr, remote))
>> + continue;
>> +
>> + if (test_bit(mptcp_endp_get_local_id(msk, &entry->addr),
>> + unavail_id))
>> + continue;
>> +
>> + local = &locals[i];
>
> Looks like 'i' is always 0 here, since the only code path from here
> leads to 'break'. Would be clearer to hardcode 0 and clarify the
> variable name for the return value.
Good idea! I can also rename the function.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-next 5/6] mptcp: pm: in-kernel: add 'address' endpoints
2025-09-23 9:05 ` Matthieu Baerts
@ 2025-09-23 22:32 ` Mat Martineau
0 siblings, 0 replies; 13+ messages in thread
From: Mat Martineau @ 2025-09-23 22:32 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: MPTCP Upstream
[-- Attachment #1: Type: text/plain, Size: 6528 bytes --]
On Tue, 23 Sep 2025, Matthieu Baerts wrote:
> Hi Mat,
>
> On 23/09/2025 06:17, Mat Martineau wrote:
>> On Tue, 23 Sep 2025, Matthieu Baerts (NGI0) wrote:
>>
>>> Currently, upon the reception of an ADD_ADDR (and when the fullmesh flag
>>> is not used), the in-kernel PM will create new subflows using the local
>>> address the routing configuration will pick.
>>>
>>> It would be easier to pick local addresses from a selected list of
>>> endpoints, and use it only once, than relying on routing rules.
>>>
>>> Use case: both the client (C) and the server (S) have two addresses (a
>>> and b). The client establishes the connection between C(a) and S(a).
>>> Once established, the server announces its additional address S(b). Once
>>> received, the client connects to it using its second address C(b).
>>> Compared to a situation without the 'address' endpoint for C(b), the
>>> client didn't use this address C(b) to establish a subflow to the
>>> server's primary address S(a). So at the end, we have:
>>>
>>> C S
>>> C(a) --- S(a)
>>> C(b) --- S(b)
>>>
>>> In case of a 3rd address on each side (C(c) and S(c)), upon the
>>> reception of an ADD_ADDR with S(c), the client should not pick C(b)
>>> because it has already been used. C(c) should then be used.
>>>
>>> Note that this situation is currently possible if C doesn't add any
>>> endpoint, but configure the routing in order to pick C(b) for the route
>>> to S(b), and pick C(c) for the route to S(c). That doesn't sound very
>>> practical because it means knowing in advance the IP addresses that
>>> will be used and announced by the server.
>>>
>>> In the code, the new endpoint type is added. Similar to the other
>>> subflow types, an MPTCP_INFO counter is added. While at it, hole are now
>>> commented in struct mptcp_info, to remember next time that these holes
>>> can no longer be used.
>>
>> Hi Matthieu -
>>
>> I think this patch brings up a few larger topics of discussion: path
>> manager strategy (in-kernel/userspace/bpf), interaction of in-kernel PM
>> flags, and (once again!) naming.
>>
>> I'm not sure the reply chain for this patch is the right place to have
>> the discussion, but adding another in-kernel PM "mode" makes me think we
>> need a community-level (MPTCP) discussion on our path manager strategy.
>> The original plan was to have a single general in-kernel PM, and rely on
>> userspace/mptcpd for anything else. We've obviously made some changes to
>> that plan, adding fullmesh and having the BPF PM in progress. This has
>> made path management more complex to understand, use, and maintain - so
>> I want to be sure we are making a careful choice about which PM features
>> to add.
>
> Good point!
>
> (Regarding the BPF PM, it is similar to the userspace PM, but can be
> used in environment handling loads of connections in parallel ; and it
> introduces a better separation between the different PMs, which makes it
> worth it for the maintenance to me.)
>
>> Our userspace API for the in-kernel PM also makes it complicated to
>> explain what happens when there is a mix of endpoint types. Typical use
>> would probably be reasonable (all fullmesh, all 'address', etc). But
>> it's good to avoid confusion, and more importantly bugs!
>
> Indeed, I fixed quite a few issues and inconsistencies last year, around
> the same time I opened this issue #503.
>
> I have to admit that the 'fullmesh' mode added quite a bit of unexpected
> complexity for such a niche use-case (but it allows "workarounds"). I
> think the new mode suggested here is simple, targeting one specific part
> of the code (an ADD_ADDR is received) and fix the lack of control on
> what source IP address is used to create new subflows when an ADD_ADDR
> is received. Not as invasive as the 'fullmesh' one.
>
Hi Matthieu -
Yes, I agree this addition is more targeted, and I also think it's a very
useful capability. All of these small additions do add up over time, and I
want to be sure we keep track of the "big picture" and think about that
full context when deciding how to expand our PM features.
>> As for naming, unfortunately "address" is a very frequently used word in
>> our subsystem! In mptcpd the similar plugin is called "sspi" (single
>> subflow per interface). I'm definitely open to other ideas that are
>> identifiable and descriptive.
>
> Me too! I initially picked "add-addr", but I wasn't happy with the minus
> sign, nor by its name. I guess a good name could be "endpoint used when
> an ADD_ADDR is received", or maybe "received-add-addr", but that seems
> too long, and more than one word.
>
> I ended up picking "address", because it is short, and similar to
> "signal" and "subflow" (which are not very clear, but hard to do better
> with one word...).
>
I still get 'signal' and 'subflow' mixed up sometimes and think we can
improve our next choice :)
>> I think there are good use cases for this feature, that's why we
>> included the similar feature in mptcpd! I'd like to get our core group
>> (you, me, Geliang, and Paolo) aligned on a general direction for path
>> management, does that sound reasonable?
>
> Yes indeed, it is important.
>
> To be honest, I thought we already had this discussion at the meeting
> following the opening of this ticket #503, but that was a bit more than
> one year ago. I started working on it because the lack of control on
> what source IP address is used to create new subflows when an ADD_ADDR
> is received came back in a few discussions, even prior #503. Even
> recently when a client wanted to use dedicated interface per path. It
> feels like something is missing in this in-kernel Netlink API to use
> endpoints to create subflows to addresses announced by the server.
>
> But yes, we are exposing a new option to the userspace, and if it is
> accepted -- and not modified/reverted in the new 10 weeks -- we will
> have to maintain it for a long time. So better not to get it wrong. I
> think it is "self-contain" and worth it. I would like a better name than
> "address", but I didn't find one. At the end, I'm happy with it as it is
> similar to the others and will have a longer description in the doc, but
> I'm open to another name :)
>
Ok, thanks for clarifying. I think our discussion of #503 15 months ago
was fairly brief and I'm guessing the significance of the additional
endpoint type didn't sink in at the time. Definitely easier to understand
the implications seeing the patches!
Will discuss the naming details in the v2 thread.
- Mat
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-09-23 22:32 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-22 22:23 [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 1/6] mptcp: pm: in-kernel: remove stale_loss_cnt Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 2/6] mptcp: pm: in-kernel: reduce pernet struct size Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 3/6] mptcp: pm: in-kernel: compare IDs instead of addresses Matthieu Baerts (NGI0)
2025-09-23 3:27 ` Mat Martineau
2025-09-23 8:09 ` Matthieu Baerts
2025-09-22 22:23 ` [PATCH mptcp-next 4/6] Squash to "mptcp: pm: in-kernel: usable client side with C-flag" Matthieu Baerts (NGI0)
2025-09-22 22:23 ` [PATCH mptcp-next 5/6] mptcp: pm: in-kernel: add 'address' endpoints Matthieu Baerts (NGI0)
2025-09-23 5:17 ` Mat Martineau
2025-09-23 9:05 ` Matthieu Baerts
2025-09-23 22:32 ` Mat Martineau
2025-09-22 22:23 ` [PATCH mptcp-next 6/6] selftests: mptcp: join: validate new " Matthieu Baerts (NGI0)
2025-09-23 0:20 ` [PATCH mptcp-next 0/6] mptcp: pm: in-kernel: add " MPTCP CI
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.