1 | With the in-kernel path-manager, it is possible to change the 'fullmesh' | 1 | Here are a few patches linked to the 'set-flags' interface with the |
---|---|---|---|
2 | flag. The code in mptcp_pm_nl_fullmesh() expects to change it only on | 2 | in-kernel path-manager. |
3 | 'subflow' endpoints, to recreate more or less subflows using the linked | ||
4 | address. | ||
5 | 3 | ||
6 | Unfortunately, the set_flags() hook was a bit more permissive, and | 4 | Patch 1: a small cleanup. For -next. |
7 | allowed 'implicit' endpoints to get the 'fullmesh' flag while it is not | ||
8 | allowed before. | ||
9 | 5 | ||
10 | That's what syzbot found, triggering the following warning: | 6 | Patch 2: a small optimisation not to iterate over all subflows to do |
7 | nothing. For -next. | ||
11 | 8 | ||
12 | WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 __mark_subflow_endp_available net/mptcp/pm_netlink.c:1496 [inline] | ||
13 | WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 mptcp_pm_nl_fullmesh net/mptcp/pm_netlink.c:1980 [inline] | ||
14 | WARNING: CPU: 0 PID: 6499 at net/mptcp/pm_netlink.c:1496 mptcp_nl_set_flags net/mptcp/pm_netlink.c:2003 [inline] | ||
15 | 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 | ||
16 | Modules linked in: | ||
17 | CPU: 0 UID: 0 PID: 6499 Comm: syz.1.413 Not tainted 6.13.0-rc5-syzkaller-00172-gd1bf27c4e176 #0 | ||
18 | Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 09/13/2024 | ||
19 | RIP: 0010:__mark_subflow_endp_available net/mptcp/pm_netlink.c:1496 [inline] | ||
20 | RIP: 0010:mptcp_pm_nl_fullmesh net/mptcp/pm_netlink.c:1980 [inline] | ||
21 | RIP: 0010:mptcp_nl_set_flags net/mptcp/pm_netlink.c:2003 [inline] | ||
22 | RIP: 0010:mptcp_pm_nl_set_flags+0x974/0xdc0 net/mptcp/pm_netlink.c:2064 | ||
23 | 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 | ||
24 | RSP: 0018:ffffc9000d307240 EFLAGS: 00010293 | ||
25 | RAX: ffffffff8bb72e03 RBX: 0000000000000000 RCX: ffff88807da88000 | ||
26 | RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 | ||
27 | RBP: ffffc9000d307430 R08: ffffffff8bb72cf0 R09: 1ffff1100b842a5e | ||
28 | R10: dffffc0000000000 R11: ffffed100b842a5f R12: ffff88801e2e5ac0 | ||
29 | R13: ffff88805c214800 R14: ffff88805c2152e8 R15: 1ffff1100b842a5d | ||
30 | FS: 00005555619f6500(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000 | ||
31 | CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 | ||
32 | CR2: 0000000020002840 CR3: 00000000247e6000 CR4: 00000000003526f0 | ||
33 | DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 | ||
34 | DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 | ||
35 | Call Trace: | ||
36 | <TASK> | ||
37 | genl_family_rcv_msg_doit net/netlink/genetlink.c:1115 [inline] | ||
38 | genl_family_rcv_msg net/netlink/genetlink.c:1195 [inline] | ||
39 | genl_rcv_msg+0xb14/0xec0 net/netlink/genetlink.c:1210 | ||
40 | netlink_rcv_skb+0x1e3/0x430 net/netlink/af_netlink.c:2542 | ||
41 | genl_rcv+0x28/0x40 net/netlink/genetlink.c:1219 | ||
42 | netlink_unicast_kernel net/netlink/af_netlink.c:1321 [inline] | ||
43 | netlink_unicast+0x7f6/0x990 net/netlink/af_netlink.c:1347 | ||
44 | netlink_sendmsg+0x8e4/0xcb0 net/netlink/af_netlink.c:1891 | ||
45 | sock_sendmsg_nosec net/socket.c:711 [inline] | ||
46 | __sock_sendmsg+0x221/0x270 net/socket.c:726 | ||
47 | ____sys_sendmsg+0x52a/0x7e0 net/socket.c:2583 | ||
48 | ___sys_sendmsg net/socket.c:2637 [inline] | ||
49 | __sys_sendmsg+0x269/0x350 net/socket.c:2669 | ||
50 | do_syscall_x64 arch/x86/entry/common.c:52 [inline] | ||
51 | do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 | ||
52 | entry_SYSCALL_64_after_hwframe+0x77/0x7f | ||
53 | RIP: 0033:0x7f5fe8785d29 | ||
54 | 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 | ||
55 | RSP: 002b:00007fff571f5558 EFLAGS: 00000246 ORIG_RAX: 000000000000002e | ||
56 | RAX: ffffffffffffffda RBX: 00007f5fe8975fa0 RCX: 00007f5fe8785d29 | ||
57 | RDX: 0000000000000000 RSI: 0000000020000480 RDI: 0000000000000007 | ||
58 | RBP: 00007f5fe8801b08 R08: 0000000000000000 R09: 0000000000000000 | ||
59 | R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 | ||
60 | R13: 00007f5fe8975fa0 R14: 00007f5fe8975fa0 R15: 00000000000011f4 | ||
61 | </TASK> | ||
62 | |||
63 | Here, syzbot managed to set the 'fullmesh' flag on an 'implicit' and | ||
64 | used -- according to 'id_avail_bitmap' -- endpoint, causing the PM to | ||
65 | try decrement the local_addr_used counter which is only incremented for | ||
66 | the 'subflow' endpoint. | ||
67 | |||
68 | Note that 'no type' endpoints -- not 'subflow', 'signal', 'implicit' -- | ||
69 | are fine, because their ID will not be marked as used in the 'id_avail' | ||
70 | bitmap, and setting 'fullmesh' can help forcing the creation of subflow | ||
71 | when receiving an ADD_ADDR. | ||
72 | |||
73 | Fixes: 73c762c1f07d ("mptcp: set fullmesh flag in pm_netlink") | ||
74 | Reported-by: syzbot+cd16e79c1e45f3fe0377@syzkaller.appspotmail.com | ||
75 | Closes: https://lore.kernel.org/6786ac51.050a0220.216c54.00a6.GAE@google.com | ||
76 | Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/540 | ||
77 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> | 9 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> |
78 | --- | 10 | --- |
79 | net/mptcp/pm_netlink.c | 3 ++- | 11 | Changes in v3: |
80 | 1 file changed, 2 insertions(+), 1 deletion(-) | 12 | - the previous patch 1 has been applied |
13 | - patch 1: add Mat's RvB tag | ||
14 | - patch 2: clarify the modifications: new comment and description (Mat) | ||
15 | - Link to v2: https://lore.kernel.org/r/20250117-mptcp-issue-540-v2-0-a194740fb380@kernel.org | ||
81 | 16 | ||
82 | diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c | 17 | Changes in v2: |
83 | index XXXXXXX..XXXXXXX 100644 | 18 | - patch 1: check the two flags in one instruction. (Paolo) |
84 | --- a/net/mptcp/pm_netlink.c | 19 | - patch 2-3: new patches, for next. |
85 | +++ b/net/mptcp/pm_netlink.c | 20 | - Link to v1: https://lore.kernel.org/r/20250116-mptcp-issue-540-v1-1-b0ba2c126eb8@kernel.org |
86 | @@ -XXX,XX +XXX,XX @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local, | ||
87 | return -EINVAL; | ||
88 | } | ||
89 | if ((local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH) && | ||
90 | - (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) { | ||
91 | + ((entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL) || | ||
92 | + (entry->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT))) { | ||
93 | spin_unlock_bh(&pernet->lock); | ||
94 | NL_SET_ERR_MSG_ATTR(info->extack, attr, "invalid addr flags"); | ||
95 | return -EINVAL; | ||
96 | 21 | ||
97 | --- | 22 | --- |
98 | base-commit: 9f10cc97addfb3a81b4aaf4b13f59b36b5bcc7a6 | 23 | Matthieu Baerts (NGI0) (2): |
24 | mptcp: pm: remove unused ret value to set flags | ||
25 | mptcp: pm: change to fullmesh only for 'subflow' | ||
26 | |||
27 | net/mptcp/pm_netlink.c | 24 ++++++++++++------------ | ||
28 | 1 file changed, 12 insertions(+), 12 deletions(-) | ||
29 | --- | ||
30 | base-commit: 8c4d55afa2c3c2391af5d975ab3b1a5bcc140101 | ||
99 | change-id: 20250116-mptcp-issue-540-49d1db3346a9 | 31 | change-id: 20250116-mptcp-issue-540-49d1db3346a9 |
100 | 32 | ||
101 | Best regards, | 33 | Best regards, |
102 | -- | 34 | -- |
103 | Matthieu Baerts (NGI0) <matttbe@kernel.org> | 35 | Matthieu Baerts (NGI0) <matttbe@kernel.org> | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | The returned value is not used, it can then be dropped. | ||
1 | 2 | ||
3 | Reviewed-by: Mat Martineau <martineau@kernel.org> | ||
4 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> | ||
5 | --- | ||
6 | net/mptcp/pm_netlink.c | 10 ++++------ | ||
7 | 1 file changed, 4 insertions(+), 6 deletions(-) | ||
8 | |||
9 | diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c | ||
10 | index XXXXXXX..XXXXXXX 100644 | ||
11 | --- a/net/mptcp/pm_netlink.c | ||
12 | +++ b/net/mptcp/pm_netlink.c | ||
13 | @@ -XXX,XX +XXX,XX @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk, | ||
14 | spin_unlock_bh(&msk->pm.lock); | ||
15 | } | ||
16 | |||
17 | -static int mptcp_nl_set_flags(struct net *net, | ||
18 | - struct mptcp_addr_info *addr, | ||
19 | - u8 bkup, u8 changed) | ||
20 | +static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr, | ||
21 | + u8 bkup, u8 changed) | ||
22 | { | ||
23 | long s_slot = 0, s_num = 0; | ||
24 | struct mptcp_sock *msk; | ||
25 | - int ret = -EINVAL; | ||
26 | |||
27 | while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) { | ||
28 | struct sock *sk = (struct sock *)msk; | ||
29 | @@ -XXX,XX +XXX,XX @@ static int mptcp_nl_set_flags(struct net *net, | ||
30 | |||
31 | lock_sock(sk); | ||
32 | if (changed & MPTCP_PM_ADDR_FLAG_BACKUP) | ||
33 | - ret = mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup); | ||
34 | + mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup); | ||
35 | if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH) | ||
36 | mptcp_pm_nl_fullmesh(msk, addr); | ||
37 | release_sock(sk); | ||
38 | @@ -XXX,XX +XXX,XX @@ static int mptcp_nl_set_flags(struct net *net, | ||
39 | cond_resched(); | ||
40 | } | ||
41 | |||
42 | - return ret; | ||
43 | + return; | ||
44 | } | ||
45 | |||
46 | int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local, | ||
47 | |||
48 | -- | ||
49 | 2.47.1 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | If an endpoint doesn't have the 'subflow' endpoint -- in fact, has no | ||
2 | type, so not 'subflow', 'signal', nor 'implicit' -- there are then no | ||
3 | subflows created from this local endpoint to at least the initial | ||
4 | destination address. In this case, no need to call mptcp_pm_nl_fullmesh() | ||
5 | which is there to recreate the subflows to reflect the new value of the | ||
6 | fullmesh attribute. | ||
1 | 7 | ||
8 | Similarly, there is then no need to iterate over all connections to do | ||
9 | nothing, if only the 'fullmesh' flag has been changed, and the endpoint | ||
10 | doesn't have the 'subflow' one. So stop early when dealing with this | ||
11 | specific case. | ||
12 | |||
13 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> | ||
14 | --- | ||
15 | Notes: | ||
16 | v2: | ||
17 | - clarify the modifications: new comment and updated description (Mat) | ||
18 | --- | ||
19 | net/mptcp/pm_netlink.c | 16 +++++++++------- | ||
20 | 1 file changed, 9 insertions(+), 7 deletions(-) | ||
21 | |||
22 | diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c | ||
23 | index XXXXXXX..XXXXXXX 100644 | ||
24 | --- a/net/mptcp/pm_netlink.c | ||
25 | +++ b/net/mptcp/pm_netlink.c | ||
26 | @@ -XXX,XX +XXX,XX @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk, | ||
27 | } | ||
28 | |||
29 | static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr, | ||
30 | - u8 bkup, u8 changed) | ||
31 | + u8 flags, u8 changed) | ||
32 | { | ||
33 | + u8 is_subflow = !!(flags & MPTCP_PM_ADDR_FLAG_SUBFLOW); | ||
34 | + u8 bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP); | ||
35 | long s_slot = 0, s_num = 0; | ||
36 | struct mptcp_sock *msk; | ||
37 | |||
38 | + if (changed == MPTCP_PM_ADDR_FLAG_FULLMESH && !is_subflow) | ||
39 | + return; | ||
40 | + | ||
41 | while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) { | ||
42 | struct sock *sk = (struct sock *)msk; | ||
43 | |||
44 | @@ -XXX,XX +XXX,XX @@ static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr, | ||
45 | lock_sock(sk); | ||
46 | if (changed & MPTCP_PM_ADDR_FLAG_BACKUP) | ||
47 | mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup); | ||
48 | - if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH) | ||
49 | + /* Subflows will only be recreated if the SUBFLOW flag is set */ | ||
50 | + if (is_subflow && (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)) | ||
51 | mptcp_pm_nl_fullmesh(msk, addr); | ||
52 | release_sock(sk); | ||
53 | |||
54 | @@ -XXX,XX +XXX,XX @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local, | ||
55 | struct mptcp_pm_addr_entry *entry; | ||
56 | struct pm_nl_pernet *pernet; | ||
57 | u8 lookup_by_id = 0; | ||
58 | - u8 bkup = 0; | ||
59 | |||
60 | pernet = pm_nl_get_pernet(net); | ||
61 | |||
62 | @@ -XXX,XX +XXX,XX @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local, | ||
63 | } | ||
64 | } | ||
65 | |||
66 | - if (local->flags & MPTCP_PM_ADDR_FLAG_BACKUP) | ||
67 | - bkup = 1; | ||
68 | - | ||
69 | spin_lock_bh(&pernet->lock); | ||
70 | entry = lookup_by_id ? __lookup_addr_by_id(pernet, local->addr.id) : | ||
71 | __lookup_addr(pernet, &local->addr); | ||
72 | @@ -XXX,XX +XXX,XX @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local, | ||
73 | *local = *entry; | ||
74 | spin_unlock_bh(&pernet->lock); | ||
75 | |||
76 | - mptcp_nl_set_flags(net, &local->addr, bkup, changed); | ||
77 | + mptcp_nl_set_flags(net, &local->addr, entry->flags, changed); | ||
78 | return 0; | ||
79 | } | ||
80 | |||
81 | |||
82 | -- | ||
83 | 2.47.1 | diff view generated by jsdifflib |