[PATCH mptcp-net v2 0/3] mptcp: pm: only set fullmesh for subflow endp

[PATCH mptcp-net v2 0/3] mptcp: pm: only set fullmesh for subflow endp

[Matthieu Baerts (NGI0)]

Here are a few patches linked to the 'set-flags' interface with the
in-kernel path-manager.

Patch 1: a fix for an issue reported by syzbot. For -net.

Patch 2: a small cleanup. For -next.

Patch 3: a small optimisation not to iterate over all subflows to do
         nothing. For -next.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v2:
- patch 1: check the two flags in one instruction. (Paolo)
- patch 2-3: new patches, for next.
- Link to v1: https://lore.kernel.org/r/20250116-mptcp-issue-540-v1-1-b0ba2c126eb8@kernel.org

---
Matthieu Baerts (NGI0) (3):
      mptcp: pm: only set fullmesh for subflow endp
      mptcp: pm: remove unused ret value to set flags
      mptcp: pm: change to fullmesh only for 'subflow'

 net/mptcp/pm_netlink.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
---
base-commit: c466642ac4d22d6f294d669d8a1a3535610cb45c
change-id: 20250116-mptcp-issue-540-49d1db3346a9

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-net v2 1/3] mptcp: pm: only set fullmesh for subflow endp

[Matthieu Baerts (NGI0)]

With the in-kernel path-manager, it is possible to change the 'fullmesh'
flag. The code in mptcp_pm_nl_fullmesh() expects to change it only on
'subflow' endpoints, to recreate more or less subflows using the linked
address.

Unfortunately, the set_flags() hook was a bit more permissive, and
allowed 'implicit' endpoints to get the 'fullmesh' flag while it is not
allowed before.

That's what syzbot found, triggering the following warning:

  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 __mark_subflow_endp_available net/mptcp/pm_netlink.c:1496 [inline]
  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 mptcp_pm_nl_fullmesh net/mptcp/pm_netlink.c:1980 [inline]
  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 mptcp_nl_set_flags net/mptcp/pm_netlink.c:2003 [inline]
  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 mptcp_pm_nl_set_flags+0x974/0xdc0 net/mptcp/pm_netlink.c:2064
  Modules linked in:
  CPU: 0 UID: 0 PID: 6499 Comm: syz.1.413 Not tainted 6.13.0-rc5-syzkaller-00172-gd1bf27c4e176 #0
  Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 09/13/2024
  RIP: 0010:__mark_subflow_endp_available net/mptcp/pm_netlink.c:1496 [inline]
  RIP: 0010:mptcp_pm_nl_fullmesh net/mptcp/pm_netlink.c:1980 [inline]
  RIP: 0010:mptcp_nl_set_flags net/mptcp/pm_netlink.c:2003 [inline]
  RIP: 0010:mptcp_pm_nl_set_flags+0x974/0xdc0 net/mptcp/pm_netlink.c:2064
  Code: 01 00 00 49 89 c5 e8 fb 45 e8 f5 e9 b8 fc ff ff e8 f1 45 e8 f5 4c 89 f7 be 03 00 00 00 e8 44 1d 0b f9 eb a0 e8 dd 45 e8 f5 90 <0f> 0b 90 e9 17 ff ff ff 89 d9 80 e1 07 38 c1 0f 8c c9 fc ff ff 48
  RSP: 0018:ffffc9000d307240 EFLAGS: 00010293
  RAX: ffffffff8bb72e03 RBX: 0000000000000000 RCX: ffff88807da88000
  RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
  RBP: ffffc9000d307430 R08: ffffffff8bb72cf0 R09: 1ffff1100b842a5e
  R10: dffffc0000000000 R11: ffffed100b842a5f R12: ffff88801e2e5ac0
  R13: ffff88805c214800 R14: ffff88805c2152e8 R15: 1ffff1100b842a5d
  FS:  00005555619f6500(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 0000000020002840 CR3: 00000000247e6000 CR4: 00000000003526f0
  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  Call Trace:
   <TASK>
   genl_family_rcv_msg_doit net/netlink/genetlink.c:1115 [inline]
   genl_family_rcv_msg net/netlink/genetlink.c:1195 [inline]
   genl_rcv_msg+0xb14/0xec0 net/netlink/genetlink.c:1210
   netlink_rcv_skb+0x1e3/0x430 net/netlink/af_netlink.c:2542
   genl_rcv+0x28/0x40 net/netlink/genetlink.c:1219
   netlink_unicast_kernel net/netlink/af_netlink.c:1321 [inline]
   netlink_unicast+0x7f6/0x990 net/netlink/af_netlink.c:1347
   netlink_sendmsg+0x8e4/0xcb0 net/netlink/af_netlink.c:1891
   sock_sendmsg_nosec net/socket.c:711 [inline]
   __sock_sendmsg+0x221/0x270 net/socket.c:726
   ____sys_sendmsg+0x52a/0x7e0 net/socket.c:2583
   ___sys_sendmsg net/socket.c:2637 [inline]
   __sys_sendmsg+0x269/0x350 net/socket.c:2669
   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
   do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
   entry_SYSCALL_64_after_hwframe+0x77/0x7f
  RIP: 0033:0x7f5fe8785d29
  Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
  RSP: 002b:00007fff571f5558 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
  RAX: ffffffffffffffda RBX: 00007f5fe8975fa0 RCX: 00007f5fe8785d29
  RDX: 0000000000000000 RSI: 0000000020000480 RDI: 0000000000000007
  RBP: 00007f5fe8801b08 R08: 0000000000000000 R09: 0000000000000000
  R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
  R13: 00007f5fe8975fa0 R14: 00007f5fe8975fa0 R15: 00000000000011f4
   </TASK>

Here, syzbot managed to set the 'fullmesh' flag on an 'implicit' and
used -- according to 'id_avail_bitmap' -- endpoint, causing the PM to
try decrement the local_addr_used counter which is only incremented for
the 'subflow' endpoint.

Note that 'no type' endpoints -- not 'subflow', 'signal', 'implicit' --
are fine, because their ID will not be marked as used in the 'id_avail'
bitmap, and setting 'fullmesh' can help forcing the creation of subflow
when receiving an ADD_ADDR.

Fixes: 73c762c1f07d ("mptcp: set fullmesh flag in pm_netlink")
Reported-by: syzbot+cd16e79c1e45f3fe0377@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/6786ac51.050a0220.216c54.00a6.GAE@google.com
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/540
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
- v2: check the two flags in one instruction. (Paolo)
---
 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 fef01692eaed404e272359df691264f797240d10..99705a9c2238c6be96e320e8cd1d12bfa0e0e7f0 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1986,7 +1986,8 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 		return -EINVAL;
 	}
 	if ((local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
-	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
+	    (entry->flags & (MPTCP_PM_ADDR_FLAG_SIGNAL |
+			     MPTCP_PM_ADDR_FLAG_IMPLICIT))) {
 		spin_unlock_bh(&pernet->lock);
 		NL_SET_ERR_MSG_ATTR(info->extack, attr, "invalid addr flags");
 		return -EINVAL;

-- 
2.47.1

[PATCH mptcp-net v2 2/3] mptcp: pm: remove unused ret value to set flags

[Matthieu Baerts (NGI0)]

The returned value is not used, it can then be dropped.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 99705a9c2238c6be96e320e8cd1d12bfa0e0e7f0..ff1e5695dc1db5e32d5f45bef7cf22e43aea0ef1 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1922,13 +1922,11 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
 	spin_unlock_bh(&msk->pm.lock);
 }
 
-static int mptcp_nl_set_flags(struct net *net,
-			      struct mptcp_addr_info *addr,
-			      u8 bkup, u8 changed)
+static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
+			       u8 bkup, u8 changed)
 {
 	long s_slot = 0, s_num = 0;
 	struct mptcp_sock *msk;
-	int ret = -EINVAL;
 
 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
 		struct sock *sk = (struct sock *)msk;
@@ -1938,7 +1936,7 @@ static int mptcp_nl_set_flags(struct net *net,
 
 		lock_sock(sk);
 		if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
-			ret = mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
+			mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
 		if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
 			mptcp_pm_nl_fullmesh(msk, addr);
 		release_sock(sk);
@@ -1948,7 +1946,7 @@ static int mptcp_nl_set_flags(struct net *net,
 		cond_resched();
 	}
 
-	return ret;
+	return;
 }
 
 int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,

-- 
2.47.1

[PATCH mptcp-net v2 3/3] mptcp: pm: change to fullmesh only for 'subflow'

[Matthieu Baerts (NGI0)]

If an entrypoint has no type -- so not 'subflow', 'signal', 'implicit' --
there are then no subflows to re-create from this local endpoint.

In this case, there is then no need to iterate over all connections to
do nothing. So stop early when this case is present.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index ff1e5695dc1db5e32d5f45bef7cf22e43aea0ef1..b1fe2a74fcfe97896de8f9eaee9a1afa5378fabb 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1923,11 +1923,16 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
 }
 
 static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
-			       u8 bkup, u8 changed)
+			       u8 flags, u8 changed)
 {
+	u8 is_subflow = !!(flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
+	u8 bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 	long s_slot = 0, s_num = 0;
 	struct mptcp_sock *msk;
 
+	if (changed == MPTCP_PM_ADDR_FLAG_FULLMESH && !is_subflow)
+		return;
+
 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
 		struct sock *sk = (struct sock *)msk;
 
@@ -1937,7 +1942,7 @@ static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
 		lock_sock(sk);
 		if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
 			mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
-		if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
+		if (is_subflow && (changed & MPTCP_PM_ADDR_FLAG_FULLMESH))
 			mptcp_pm_nl_fullmesh(msk, addr);
 		release_sock(sk);
 
@@ -1959,7 +1964,6 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
 	u8 lookup_by_id = 0;
-	u8 bkup = 0;
 
 	pernet = pm_nl_get_pernet(net);
 
@@ -1972,9 +1976,6 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 		}
 	}
 
-	if (local->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
-		bkup = 1;
-
 	spin_lock_bh(&pernet->lock);
 	entry = lookup_by_id ? __lookup_addr_by_id(pernet, local->addr.id) :
 			       __lookup_addr(pernet, &local->addr);
@@ -1996,7 +1997,7 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 	*local = *entry;
 	spin_unlock_bh(&pernet->lock);
 
-	mptcp_nl_set_flags(net, &local->addr, bkup, changed);
+	mptcp_nl_set_flags(net, &local->addr, entry->flags, changed);
 	return 0;
 }
 

-- 
2.47.1

Re: [PATCH mptcp-net v2 0/3] mptcp: pm: only set fullmesh for subflow endp

[MPTCP CI]

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: Critical: 1 Call Trace(s) ❌
- 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/12826636303

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/0abf0f42d0e0
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=926404


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)

Re: [PATCH mptcp-net v2 1/3] mptcp: pm: only set fullmesh for subflow endp

[Mat Martineau]

On Fri, 17 Jan 2025, Matthieu Baerts (NGI0) wrote:

> With the in-kernel path-manager, it is possible to change the 'fullmesh'
> flag. The code in mptcp_pm_nl_fullmesh() expects to change it only on
> 'subflow' endpoints, to recreate more or less subflows using the linked
> address.
>
> Unfortunately, the set_flags() hook was a bit more permissive, and
> allowed 'implicit' endpoints to get the 'fullmesh' flag while it is not
> allowed before.
>
> That's what syzbot found, triggering the following warning:
>
>  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 __mark_subflow_endp_available net/mptcp/pm_netlink.c:1496 [inline]
>  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 mptcp_pm_nl_fullmesh net/mptcp/pm_netlink.c:1980 [inline]
>  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 mptcp_nl_set_flags net/mptcp/pm_netlink.c:2003 [inline]
>  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 mptcp_pm_nl_set_flags+0x974/0xdc0 net/mptcp/pm_netlink.c:2064
>  Modules linked in:
>  CPU: 0 UID: 0 PID: 6499 Comm: syz.1.413 Not tainted 6.13.0-rc5-syzkaller-00172-gd1bf27c4e176 #0
>  Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 09/13/2024
>  RIP: 0010:__mark_subflow_endp_available net/mptcp/pm_netlink.c:1496 [inline]
>  RIP: 0010:mptcp_pm_nl_fullmesh net/mptcp/pm_netlink.c:1980 [inline]
>  RIP: 0010:mptcp_nl_set_flags net/mptcp/pm_netlink.c:2003 [inline]
>  RIP: 0010:mptcp_pm_nl_set_flags+0x974/0xdc0 net/mptcp/pm_netlink.c:2064
>  Code: 01 00 00 49 89 c5 e8 fb 45 e8 f5 e9 b8 fc ff ff e8 f1 45 e8 f5 4c 89 f7 be 03 00 00 00 e8 44 1d 0b f9 eb a0 e8 dd 45 e8 f5 90 <0f> 0b 90 e9 17 ff ff ff 89 d9 80 e1 07 38 c1 0f 8c c9 fc ff ff 48
>  RSP: 0018:ffffc9000d307240 EFLAGS: 00010293
>  RAX: ffffffff8bb72e03 RBX: 0000000000000000 RCX: ffff88807da88000
>  RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
>  RBP: ffffc9000d307430 R08: ffffffff8bb72cf0 R09: 1ffff1100b842a5e
>  R10: dffffc0000000000 R11: ffffed100b842a5f R12: ffff88801e2e5ac0
>  R13: ffff88805c214800 R14: ffff88805c2152e8 R15: 1ffff1100b842a5d
>  FS:  00005555619f6500(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 0000000020002840 CR3: 00000000247e6000 CR4: 00000000003526f0
>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  Call Trace:
>   <TASK>
>   genl_family_rcv_msg_doit net/netlink/genetlink.c:1115 [inline]
>   genl_family_rcv_msg net/netlink/genetlink.c:1195 [inline]
>   genl_rcv_msg+0xb14/0xec0 net/netlink/genetlink.c:1210
>   netlink_rcv_skb+0x1e3/0x430 net/netlink/af_netlink.c:2542
>   genl_rcv+0x28/0x40 net/netlink/genetlink.c:1219
>   netlink_unicast_kernel net/netlink/af_netlink.c:1321 [inline]
>   netlink_unicast+0x7f6/0x990 net/netlink/af_netlink.c:1347
>   netlink_sendmsg+0x8e4/0xcb0 net/netlink/af_netlink.c:1891
>   sock_sendmsg_nosec net/socket.c:711 [inline]
>   __sock_sendmsg+0x221/0x270 net/socket.c:726
>   ____sys_sendmsg+0x52a/0x7e0 net/socket.c:2583
>   ___sys_sendmsg net/socket.c:2637 [inline]
>   __sys_sendmsg+0x269/0x350 net/socket.c:2669
>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>   do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
>  RIP: 0033:0x7f5fe8785d29
>  Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
>  RSP: 002b:00007fff571f5558 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>  RAX: ffffffffffffffda RBX: 00007f5fe8975fa0 RCX: 00007f5fe8785d29
>  RDX: 0000000000000000 RSI: 0000000020000480 RDI: 0000000000000007
>  RBP: 00007f5fe8801b08 R08: 0000000000000000 R09: 0000000000000000
>  R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
>  R13: 00007f5fe8975fa0 R14: 00007f5fe8975fa0 R15: 00000000000011f4
>   </TASK>
>
> Here, syzbot managed to set the 'fullmesh' flag on an 'implicit' and
> used -- according to 'id_avail_bitmap' -- endpoint, causing the PM to
> try decrement the local_addr_used counter which is only incremented for
> the 'subflow' endpoint.
>
> Note that 'no type' endpoints -- not 'subflow', 'signal', 'implicit' --
> are fine, because their ID will not be marked as used in the 'id_avail'
> bitmap, and setting 'fullmesh' can help forcing the creation of subflow
> when receiving an ADD_ADDR.
>
> Fixes: 73c762c1f07d ("mptcp: set fullmesh flag in pm_netlink")
> Reported-by: syzbot+cd16e79c1e45f3fe0377@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/6786ac51.050a0220.216c54.00a6.GAE@google.com
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/540
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
> - v2: check the two flags in one instruction. (Paolo)

v2 LGTM:

Reviewed-by: Mat Martineau <martineau@kernel.org>

> ---
> 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 fef01692eaed404e272359df691264f797240d10..99705a9c2238c6be96e320e8cd1d12bfa0e0e7f0 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1986,7 +1986,8 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
> 		return -EINVAL;
> 	}
> 	if ((local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
> -	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> +	    (entry->flags & (MPTCP_PM_ADDR_FLAG_SIGNAL |
> +			     MPTCP_PM_ADDR_FLAG_IMPLICIT))) {
> 		spin_unlock_bh(&pernet->lock);
> 		NL_SET_ERR_MSG_ATTR(info->extack, attr, "invalid addr flags");
> 		return -EINVAL;
>
> -- 
> 2.47.1
>
>
>

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: remove unused ret value to set flags

[Mat Martineau]

On Fri, 17 Jan 2025, Matthieu Baerts (NGI0) wrote:

> The returned value is not used, it can then be dropped.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Thanks Matthieu

Reviewed-by: Mat Martineau <martineau@kernel.org>

> ---
> net/mptcp/pm_netlink.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 99705a9c2238c6be96e320e8cd1d12bfa0e0e7f0..ff1e5695dc1db5e32d5f45bef7cf22e43aea0ef1 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1922,13 +1922,11 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
> 	spin_unlock_bh(&msk->pm.lock);
> }
>
> -static int mptcp_nl_set_flags(struct net *net,
> -			      struct mptcp_addr_info *addr,
> -			      u8 bkup, u8 changed)
> +static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
> +			       u8 bkup, u8 changed)
> {
> 	long s_slot = 0, s_num = 0;
> 	struct mptcp_sock *msk;
> -	int ret = -EINVAL;
>
> 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
> 		struct sock *sk = (struct sock *)msk;
> @@ -1938,7 +1936,7 @@ static int mptcp_nl_set_flags(struct net *net,
>
> 		lock_sock(sk);
> 		if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
> -			ret = mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
> +			mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
> 		if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
> 			mptcp_pm_nl_fullmesh(msk, addr);
> 		release_sock(sk);
> @@ -1948,7 +1946,7 @@ static int mptcp_nl_set_flags(struct net *net,
> 		cond_resched();
> 	}
>
> -	return ret;
> +	return;
> }
>
> int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
>
> -- 
> 2.47.1
>
>
>

Re: [PATCH mptcp-net v2 3/3] mptcp: pm: change to fullmesh only for 'subflow'

[Mat Martineau]

On Fri, 17 Jan 2025, Matthieu Baerts (NGI0) wrote:

Hi Matthieu -

> If an entrypoint

Did you mean "endpoint" here?

> has no type -- so not 'subflow', 'signal', 'implicit' --
> there are then no subflows to re-create from this local endpoint.
>
> In this case, there is then no need to iterate over all connections to
> do nothing. So stop early when this case is present.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> net/mptcp/pm_netlink.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index ff1e5695dc1db5e32d5f45bef7cf22e43aea0ef1..b1fe2a74fcfe97896de8f9eaee9a1afa5378fabb 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1923,11 +1923,16 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
> }
>
> static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
> -			       u8 bkup, u8 changed)
> +			       u8 flags, u8 changed)
> {
> +	u8 is_subflow = !!(flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
> +	u8 bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> 	long s_slot = 0, s_num = 0;
> 	struct mptcp_sock *msk;
>
> +	if (changed == MPTCP_PM_ADDR_FLAG_FULLMESH && !is_subflow)
> +		return;
> +
> 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
> 		struct sock *sk = (struct sock *)msk;
>
> @@ -1937,7 +1942,7 @@ static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
> 		lock_sock(sk);
> 		if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
> 			mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
> -		if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
> +		if (is_subflow && (changed & MPTCP_PM_ADDR_FLAG_FULLMESH))

Could you add something to the beginning of the commit message explaining 
this part of the change? It matches the subject line but the body of the 
commit message only explains the loop optimization above.

Thanks,
Mat

> 			mptcp_pm_nl_fullmesh(msk, addr);
> 		release_sock(sk);
>
> @@ -1959,7 +1964,6 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
> 	struct mptcp_pm_addr_entry *entry;
> 	struct pm_nl_pernet *pernet;
> 	u8 lookup_by_id = 0;
> -	u8 bkup = 0;
>
> 	pernet = pm_nl_get_pernet(net);
>
> @@ -1972,9 +1976,6 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
> 		}
> 	}
>
> -	if (local->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
> -		bkup = 1;
> -
> 	spin_lock_bh(&pernet->lock);
> 	entry = lookup_by_id ? __lookup_addr_by_id(pernet, local->addr.id) :
> 			       __lookup_addr(pernet, &local->addr);
> @@ -1996,7 +1997,7 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
> 	*local = *entry;
> 	spin_unlock_bh(&pernet->lock);
>
> -	mptcp_nl_set_flags(net, &local->addr, bkup, changed);
> +	mptcp_nl_set_flags(net, &local->addr, entry->flags, changed);
> 	return 0;
> }
>
>
> -- 
> 2.47.1
>
>
>

Re: [PATCH mptcp-net v2 3/3] mptcp: pm: change to fullmesh only for 'subflow'

[Matthieu Baerts]

Hi Mat,

Thank you for the different reviews!

On 22/01/2025 00:40, Mat Martineau wrote:
> On Fri, 17 Jan 2025, Matthieu Baerts (NGI0) wrote:
> 
> Hi Matthieu -
> 
>> If an entrypoint
> 
> Did you mean "endpoint" here?

Yes indeed, good catch, that's not the first time I mix up the two words :)

>> has no type -- so not 'subflow', 'signal', 'implicit' --
>> there are then no subflows to re-create from this local endpoint.
>>
>> In this case, there is then no need to iterate over all connections to
>> do nothing. So stop early when this case is present.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> net/mptcp/pm_netlink.c | 15 ++++++++-------
>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index
>> ff1e5695dc1db5e32d5f45bef7cf22e43aea0ef1..b1fe2a74fcfe97896de8f9eaee9a1afa5378fabb 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -1923,11 +1923,16 @@ static void mptcp_pm_nl_fullmesh(struct
>> mptcp_sock *msk,
>> }
>>
>> static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info
>> *addr,
>> -                   u8 bkup, u8 changed)
>> +                   u8 flags, u8 changed)
>> {
>> +    u8 is_subflow = !!(flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
>> +    u8 bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
>>     long s_slot = 0, s_num = 0;
>>     struct mptcp_sock *msk;
>>
>> +    if (changed == MPTCP_PM_ADDR_FLAG_FULLMESH && !is_subflow)
>> +        return;
>> +
>>     while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
>>         struct sock *sk = (struct sock *)msk;
>>
>> @@ -1937,7 +1942,7 @@ static void mptcp_nl_set_flags(struct net *net,
>> struct mptcp_addr_info *addr,
>>         lock_sock(sk);
>>         if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
>>             mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
>> -        if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
>> +        if (is_subflow && (changed & MPTCP_PM_ADDR_FLAG_FULLMESH))
> 
> Could you add something to the beginning of the commit message
> explaining this part of the change? It matches the subject line but the
> body of the commit message only explains the loop optimization above.

Ah yes, I see. The first paragraph was supposed to describe that:

> If an endpoint has no type -- so not 'subflow', 'signal', 'implicit' --
> there are then no subflows to re-create from this local endpoint.

Maybe clearer if I add this comment just above?

  /* Subflows will only be "re-created" if the SUBFLOW flag is set */

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2 1/3] mptcp: pm: only set fullmesh for subflow endp

[Matthieu Baerts]

Hi Mat,

On 21/01/2025 23:59, Mat Martineau wrote:
> On Fri, 17 Jan 2025, Matthieu Baerts (NGI0) wrote:
> 
>> With the in-kernel path-manager, it is possible to change the 'fullmesh'
>> flag. The code in mptcp_pm_nl_fullmesh() expects to change it only on
>> 'subflow' endpoints, to recreate more or less subflows using the linked
>> address.
>>
>> Unfortunately, the set_flags() hook was a bit more permissive, and
>> allowed 'implicit' endpoints to get the 'fullmesh' flag while it is not
>> allowed before.
>>
>> That's what syzbot found, triggering the following warning:
>>
>>  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496
>> __mark_subflow_endp_available net/mptcp/pm_netlink.c:1496 [inline]
>>  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496
>> mptcp_pm_nl_fullmesh net/mptcp/pm_netlink.c:1980 [inline]
>>  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496
>> mptcp_nl_set_flags net/mptcp/pm_netlink.c:2003 [inline]
>>  WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496
>> mptcp_pm_nl_set_flags+0x974/0xdc0 net/mptcp/pm_netlink.c:2064
>>  Modules linked in:
>>  CPU: 0 UID: 0 PID: 6499 Comm: syz.1.413 Not tainted 6.13.0-rc5-
>> syzkaller-00172-gd1bf27c4e176 #0
>>  Hardware name: Google Compute Engine/Google Compute Engine, BIOS
>> Google 09/13/2024
>>  RIP: 0010:__mark_subflow_endp_available net/mptcp/pm_netlink.c:1496
>> [inline]
>>  RIP: 0010:mptcp_pm_nl_fullmesh net/mptcp/pm_netlink.c:1980 [inline]
>>  RIP: 0010:mptcp_nl_set_flags net/mptcp/pm_netlink.c:2003 [inline]
>>  RIP: 0010:mptcp_pm_nl_set_flags+0x974/0xdc0 net/mptcp/pm_netlink.c:2064
>>  Code: 01 00 00 49 89 c5 e8 fb 45 e8 f5 e9 b8 fc ff ff e8 f1 45 e8 f5
>> 4c 89 f7 be 03 00 00 00 e8 44 1d 0b f9 eb a0 e8 dd 45 e8 f5 90 <0f> 0b
>> 90 e9 17 ff ff ff 89 d9 80 e1 07 38 c1 0f 8c c9 fc ff ff 48
>>  RSP: 0018:ffffc9000d307240 EFLAGS: 00010293
>>  RAX: ffffffff8bb72e03 RBX: 0000000000000000 RCX: ffff88807da88000
>>  RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
>>  RBP: ffffc9000d307430 R08: ffffffff8bb72cf0 R09: 1ffff1100b842a5e
>>  R10: dffffc0000000000 R11: ffffed100b842a5f R12: ffff88801e2e5ac0
>>  R13: ffff88805c214800 R14: ffff88805c2152e8 R15: 1ffff1100b842a5d
>>  FS:  00005555619f6500(0000) GS:ffff8880b8600000(0000)
>> knlGS:0000000000000000
>>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>  CR2: 0000000020002840 CR3: 00000000247e6000 CR4: 00000000003526f0
>>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>  Call Trace:
>>   <TASK>
>>   genl_family_rcv_msg_doit net/netlink/genetlink.c:1115 [inline]
>>   genl_family_rcv_msg net/netlink/genetlink.c:1195 [inline]
>>   genl_rcv_msg+0xb14/0xec0 net/netlink/genetlink.c:1210
>>   netlink_rcv_skb+0x1e3/0x430 net/netlink/af_netlink.c:2542
>>   genl_rcv+0x28/0x40 net/netlink/genetlink.c:1219
>>   netlink_unicast_kernel net/netlink/af_netlink.c:1321 [inline]
>>   netlink_unicast+0x7f6/0x990 net/netlink/af_netlink.c:1347
>>   netlink_sendmsg+0x8e4/0xcb0 net/netlink/af_netlink.c:1891
>>   sock_sendmsg_nosec net/socket.c:711 [inline]
>>   __sock_sendmsg+0x221/0x270 net/socket.c:726
>>   ____sys_sendmsg+0x52a/0x7e0 net/socket.c:2583
>>   ___sys_sendmsg net/socket.c:2637 [inline]
>>   __sys_sendmsg+0x269/0x350 net/socket.c:2669
>>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>>   do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>  RIP: 0033:0x7f5fe8785d29
>>  Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48
>> 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d
>> 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
>>  RSP: 002b:00007fff571f5558 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>>  RAX: ffffffffffffffda RBX: 00007f5fe8975fa0 RCX: 00007f5fe8785d29
>>  RDX: 0000000000000000 RSI: 0000000020000480 RDI: 0000000000000007
>>  RBP: 00007f5fe8801b08 R08: 0000000000000000 R09: 0000000000000000
>>  R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
>>  R13: 00007f5fe8975fa0 R14: 00007f5fe8975fa0 R15: 00000000000011f4
>>   </TASK>
>>
>> Here, syzbot managed to set the 'fullmesh' flag on an 'implicit' and
>> used -- according to 'id_avail_bitmap' -- endpoint, causing the PM to
>> try decrement the local_addr_used counter which is only incremented for
>> the 'subflow' endpoint.
>>
>> Note that 'no type' endpoints -- not 'subflow', 'signal', 'implicit' --
>> are fine, because their ID will not be marked as used in the 'id_avail'
>> bitmap, and setting 'fullmesh' can help forcing the creation of subflow
>> when receiving an ADD_ADDR.
>>
>> Fixes: 73c762c1f07d ("mptcp: set fullmesh flag in pm_netlink")
>> Reported-by: syzbot+cd16e79c1e45f3fe0377@syzkaller.appspotmail.com
>> Closes: https://
>> lore.kernel.org/6786ac51.050a0220.216c54.00a6.GAE@google.com
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/540
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Notes:
>> - v2: check the two flags in one instruction. (Paolo)
> 
> v2 LGTM:
> 
> Reviewed-by: Mat Martineau <martineau@kernel.org>

Thank you for the review!

Now in our tree (fixes for -net):

New patches for t/upstream-net and t/upstream:
- c3a63555780b: mptcp: pm: only set fullmesh for subflow endp
- Results: e7c5c5982f54..28c8f5a4574b (export-net)
- 6cce766067be: conflict in t/mptcp-add-local-parameter-for-set_flags
- Results: a9a670c7f9f9..8c4d55afa2c3 (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/cd6d39abeeb0ad570a1f34adef4731098cec9cb0/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/3a8173007814d102c6043c8f4830f6621e87d435/checks

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.