Syzkaller managed to find a combination of actions that was generating this warning: WARNING: net/mptcp/pm_kernel.c:1074 at __mark_subflow_endp_available net/mptcp/pm_kernel.c:1074 [inline], CPU#1: syz.7.48/2535 WARNING: net/mptcp/pm_kernel.c:1074 at mptcp_pm_nl_fullmesh net/mptcp/pm_kernel.c:1446 [inline], CPU#1: syz.7.48/2535 WARNING: net/mptcp/pm_kernel.c:1074 at mptcp_pm_nl_set_flags_all net/mptcp/pm_kernel.c:1474 [inline], CPU#1: syz.7.48/2535 WARNING: net/mptcp/pm_kernel.c:1074 at mptcp_pm_nl_set_flags+0x5de/0x640 net/mptcp/pm_kernel.c:1538, CPU#1: syz.7.48/2535 Modules linked in: CPU: 1 UID: 0 PID: 2535 Comm: syz.7.48 Not tainted 6.18.0-03987-gea5f5e676cf5 #17 PREEMPT(voluntary) Hardware name: QEMU Ubuntu 25.10 PC (i440FX + PIIX, 1996), BIOS 1.17.0-debian-1.17.0-1 04/01/2014 RIP: 0010:__mark_subflow_endp_available net/mptcp/pm_kernel.c:1074 [inline] RIP: 0010:mptcp_pm_nl_fullmesh net/mptcp/pm_kernel.c:1446 [inline] RIP: 0010:mptcp_pm_nl_set_flags_all net/mptcp/pm_kernel.c:1474 [inline] RIP: 0010:mptcp_pm_nl_set_flags+0x5de/0x640 net/mptcp/pm_kernel.c:1538 Code: 89 c7 e8 c5 8c 73 fe e9 f7 fd ff ff 49 83 ef 80 e8 b7 8c 73 fe 4c 89 ff be 03 00 00 00 e8 4a 29 e3 fe eb ac e8 a3 8c 73 fe 90 <0f> 0b 90 e9 3d ff ff ff e8 95 8c 73 fe b8 a1 ff ff ff eb 1a e8 89 RSP: 0018:ffffc9001535b820 EFLAGS: 00010287 netdevsim0: tun_chr_ioctl cmd 1074025677 RAX: ffffffff82da294d RBX: 0000000000000001 RCX: 0000000000080000 RDX: ffffc900096d0000 RSI: 00000000000006d6 RDI: 00000000000006d7 netdevsim0: linktype set to 823 RBP: ffff88802cdb2240 R08: 00000000000104ae R09: ffffffffffffffff R10: ffffffff82da27d4 R11: 0000000000000000 R12: 0000000000000000 R13: ffff88801246d8c0 R14: ffffc9001535b8b8 R15: ffff88802cdb1800 FS: 00007fc6ac5a76c0(0000) GS:ffff8880f90c8000(0000) knlGS:0000000000000000 netlink: 'syz.3.50': attribute type 5 has an invalid length. CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 netlink: 1232 bytes leftover after parsing attributes in process `syz.3.50'. CR2: 0000200000010000 CR3: 0000000025b1a000 CR4: 0000000000350ef0 Call Trace: mptcp_pm_set_flags net/mptcp/pm_netlink.c:277 [inline] mptcp_pm_nl_set_flags_doit+0x1d7/0x210 net/mptcp/pm_netlink.c:282 genl_family_rcv_msg_doit+0x117/0x180 net/netlink/genetlink.c:1115 genl_family_rcv_msg net/netlink/genetlink.c:1195 [inline] genl_rcv_msg+0x3a8/0x3f0 net/netlink/genetlink.c:1210 netlink_rcv_skb+0x16d/0x240 net/netlink/af_netlink.c:2550 genl_rcv+0x28/0x40 net/netlink/genetlink.c:1219 netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline] netlink_unicast+0x3e9/0x4c0 net/netlink/af_netlink.c:1344 netlink_sendmsg+0x4ab/0x5b0 net/netlink/af_netlink.c:1894 sock_sendmsg_nosec net/socket.c:718 [inline] __sock_sendmsg+0xc9/0xf0 net/socket.c:733 ____sys_sendmsg+0x272/0x3b0 net/socket.c:2608 ___sys_sendmsg+0x2de/0x320 net/socket.c:2662 __sys_sendmsg net/socket.c:2694 [inline] __do_sys_sendmsg net/socket.c:2699 [inline] __se_sys_sendmsg net/socket.c:2697 [inline] __x64_sys_sendmsg+0x110/0x1a0 net/socket.c:2697 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] do_syscall_64+0xed/0x360 arch/x86/entry/syscall_64.c:94 entry_SYSCALL_64_after_hwframe+0x77/0x7f RIP: 0033:0x7fc6adb66f6d Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 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 e8 ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007fc6ac5a6ff8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e RAX: ffffffffffffffda RBX: 00007fc6addf5fa0 RCX: 00007fc6adb66f6d RDX: 0000000000048084 RSI: 00002000000002c0 RDI: 000000000000000e RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 netlink: 'syz.5.51': attribute type 2 has an invalid length. R13: 00007fff25e91fe0 R14: 00007fc6ac5a7ce4 R15: 00007fff25e920d7 The actions that caused that seem to be: - Create an MPTCP endpoint for address A without any flags - Create a new MPTCP connection from address A - Remove the MPTCP endpoint: the corresponding subflows will be removed - Recreate the endpoint with the same ID, but with the subflow flag - Change the same endpoint to add the fullmesh flag In this case, msk->pm.local_addr_used has been kept to 0 as expected, but the corresponding bit in msk->pm.id_avail_bitmap was still unset after having removed the endpoint, causing the splat later on. When removing an endpoint, the corresponding endpoint ID was only marked as available for "signal" types with an announced address, plus all "subflow" types, but not the other types like an endpoint corresponding to the initial subflow. In these cases, re-creating an endpoint with the same ID didn't signal/create anything. Here, adding the fullmesh flag was creating the splat when calling __mark_subflow_endp_available() from mptcp_pm_nl_fullmesh(), because msk->pm.local_addr_used was set to 0 while the ID was marked as used. To fix this issue, the corresponding bit in msk->pm.id_avail_bitmap can always be set as available when removing an MPTCP in-kernel endpoint. In other words, moving the call to __set_bit() to do it in all cases, except for "subflow" types where this bit is handled in a dedicated helper. Note: instead of adding a new spin_(un)lock_bh that would be taken in all cases, do all the actions requiring the spin lock under the same block. This modification potentially fixes another issue reported by syzbot, see [1]. But without a reproducer or more details about what exactly happened before, it is hard to confirm. Fixes: e255683c06df ("mptcp: pm: re-using ID of unused removed ADD_ADDR") Cc: stable@vger.kernel.org Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/606 Reported-by: syzbot+f56f7d56e2c6e11a01b6@syzkaller.appspotmail.com Closes: https://lore.kernel.org/68fcfc4a.050a0220.346f24.02fb.GAE@google.com [1] Reviewed-by: Mat Martineau Signed-off-by: Matthieu Baerts (NGI0) --- v2: clarify commit message: local_addr_used was not incremented. --- net/mptcp/pm_kernel.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c index b26675054b0d..4972c19fc73e 100644 --- a/net/mptcp/pm_kernel.c +++ b/net/mptcp/pm_kernel.c @@ -1056,10 +1056,8 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk, ret = mptcp_remove_anno_list_by_saddr(msk, addr); if (ret || force) { spin_lock_bh(&msk->pm.lock); - if (ret) { - __set_bit(addr->id, msk->pm.id_avail_bitmap); + if (ret) msk->pm.add_addr_signaled--; - } mptcp_pm_remove_addr(msk, &list); spin_unlock_bh(&msk->pm.lock); } @@ -1097,17 +1095,15 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net, !(entry->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT)); list.ids[0] = mptcp_endp_get_local_id(msk, addr); - if (remove_subflow) { - spin_lock_bh(&msk->pm.lock); - mptcp_pm_rm_subflow(msk, &list); - spin_unlock_bh(&msk->pm.lock); - } - if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) { - spin_lock_bh(&msk->pm.lock); + spin_lock_bh(&msk->pm.lock); + if (remove_subflow) + mptcp_pm_rm_subflow(msk, &list); + if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) __mark_subflow_endp_available(msk, list.ids[0]); - spin_unlock_bh(&msk->pm.lock); - } + else /* mark endp ID as available, e.g. Signal or MPC endp */ + __set_bit(addr->id, msk->pm.id_avail_bitmap); + spin_unlock_bh(&msk->pm.lock); if (msk->mpc_endpoint_id == entry->addr.id) msk->mpc_endpoint_id = 0; -- 2.51.0 The variable 'ret' was used, but it was not cleared what it was, and probably led to an issue [1]. Rename it to 'announced' to avoid confusions. While at it, remove the returned value of the helper: it is only used in one place, and the returned value is not used. Link: https://github.com/multipath-tcp/mptcp_net-next/issues/606 [1] Reviewed-by: Mat Martineau Signed-off-by: Matthieu Baerts (NGI0) --- net/mptcp/pm_kernel.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c index 4972c19fc73e..b5316a6c7d1b 100644 --- a/net/mptcp/pm_kernel.c +++ b/net/mptcp/pm_kernel.c @@ -1044,24 +1044,23 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info) return ret; } -static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk, +static void mptcp_pm_remove_anno_addr(struct mptcp_sock *msk, const struct mptcp_addr_info *addr, bool force) { struct mptcp_rm_list list = { .nr = 0 }; - bool ret; + bool announced; list.ids[list.nr++] = mptcp_endp_get_local_id(msk, addr); - ret = mptcp_remove_anno_list_by_saddr(msk, addr); - if (ret || force) { + announced = mptcp_remove_anno_list_by_saddr(msk, addr); + if (announced || force) { spin_lock_bh(&msk->pm.lock); - if (ret) + if (announced) msk->pm.add_addr_signaled--; mptcp_pm_remove_addr(msk, &list); spin_unlock_bh(&msk->pm.lock); } - return ret; } static void __mark_subflow_endp_available(struct mptcp_sock *msk, u8 id) -- 2.51.0 The following warnings were visible: $ ./scripts/kernel-doc -Wall -none \ net/mptcp/ include/net/mptcp.h include/uapi/linux/mptcp*.h \ include/trace/events/mptcp.h Warning: net/mptcp/token.c:108 No description found for return value of 'mptcp_token_new_request' Warning: net/mptcp/token.c:151 No description found for return value of 'mptcp_token_new_connect' Warning: net/mptcp/token.c:246 No description found for return value of 'mptcp_token_get_sock' Warning: net/mptcp/token.c:298 No description found for return value of 'mptcp_token_iter_next' Warning: net/mptcp/protocol.c:4431 No description found for return value of 'mptcp_splice_read' Warning: include/uapi/linux/mptcp_pm.h:13 missing initial short description on line: * enum mptcp_event_type Address all of them: either by using the 'Return:' keyword, or by adding a missing initial short description. The MPTCP CI will soon report issues with kdoc to avoid introducing new issues and being flagged by the Netdev CI. Reviewed-by: Geliang Tang Reviewed-by: Randy Dunlap Signed-off-by: Matthieu Baerts (NGI0) --- Cc: Donald Hunter v2: also modify D/n/s/mptcp_pm.yaml, used to generate i/u/l/mptcp_pm.h. --- Documentation/netlink/specs/mptcp_pm.yaml | 1 + include/uapi/linux/mptcp_pm.h | 2 +- net/mptcp/token.c | 16 +++++++++------- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/Documentation/netlink/specs/mptcp_pm.yaml b/Documentation/netlink/specs/mptcp_pm.yaml index ba30a40b9dbf..39f3facc38e5 100644 --- a/Documentation/netlink/specs/mptcp_pm.yaml +++ b/Documentation/netlink/specs/mptcp_pm.yaml @@ -15,6 +15,7 @@ definitions: type: enum name: event-type enum-name: mptcp-event-type + doc: Netlink MPTCP event types name-prefix: mptcp-event- entries: - diff --git a/include/uapi/linux/mptcp_pm.h b/include/uapi/linux/mptcp_pm.h index c97d060ee90b..fe9863d75350 100644 --- a/include/uapi/linux/mptcp_pm.h +++ b/include/uapi/linux/mptcp_pm.h @@ -11,7 +11,7 @@ #define MPTCP_PM_VER 1 /** - * enum mptcp_event_type + * enum mptcp_event_type - Netlink MPTCP event types * @MPTCP_EVENT_UNSPEC: unused event * @MPTCP_EVENT_CREATED: A new MPTCP connection has been created. It is the * good time to allocate memory and send ADD_ADDR if needed. Depending on the diff --git a/net/mptcp/token.c b/net/mptcp/token.c index 5bb924534387..f1a50f367add 100644 --- a/net/mptcp/token.c +++ b/net/mptcp/token.c @@ -103,7 +103,7 @@ static void mptcp_crypto_key_gen_sha(u64 *key, u32 *token, u64 *idsn) * It creates a unique token to identify the new mptcp connection, * a secret local key and the initial data sequence number (idsn). * - * Returns 0 on success. + * Return: 0 on success. */ int mptcp_token_new_request(struct request_sock *req) { @@ -146,7 +146,7 @@ int mptcp_token_new_request(struct request_sock *req) * the computed token at a later time, this is needed to process * join requests. * - * returns 0 on success. + * Return: 0 on success. */ int mptcp_token_new_connect(struct sock *ssk) { @@ -241,7 +241,7 @@ bool mptcp_token_exists(u32 token) * This function returns the mptcp connection structure with the given token. * A reference count on the mptcp socket returned is taken. * - * returns NULL if no connection with the given token value exists. + * Return: NULL if no connection with the given token value exists. */ struct mptcp_sock *mptcp_token_get_sock(struct net *net, u32 token) { @@ -288,11 +288,13 @@ EXPORT_SYMBOL_GPL(mptcp_token_get_sock); * @s_slot: start slot number * @s_num: start number inside the given lock * - * This function returns the first mptcp connection structure found inside the - * token container starting from the specified position, or NULL. + * Description: + * On successful iteration, the iterator is moved to the next position and a + * reference to the returned socket is acquired. * - * On successful iteration, the iterator is moved to the next position and - * a reference to the returned socket is acquired. + * Return: + * The first mptcp connection structure found inside the token container + * starting from the specified position, or NULL. */ struct mptcp_sock *mptcp_token_iter_next(const struct net *net, long *s_slot, long *s_num) -- 2.51.0 This warning can be seen with GCC 15.2: mptcp_connect.c: In function ‘main_loop’: mptcp_connect.c:1422:37: warning: ‘peer’ may be used uninitialized [-Wmaybe-uninitialized] 1422 | if (connect(fd, peer->ai_addr, peer->ai_addrlen)) | ~~~~^~~~~~~~~ mptcp_connect.c:1377:26: note: ‘peer’ was declared here 1377 | struct addrinfo *peer; | ^~~~ This variable is set in sock_connect_mptcp() in some conditions. If not, this helper returns an error, and the program stops. So this is a false positive, but better removing it by initialising peer to NULL. Reviewed-by: Geliang Tang Signed-off-by: Matthieu Baerts (NGI0) --- Cc: Shuah Khan Cc: linux-kselftest@vger.kernel.org --- tools/testing/selftests/net/mptcp/mptcp_connect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c index 10f6f99cfd4e..24b4abac8687 100644 --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c @@ -1296,8 +1296,8 @@ void xdisconnect(int fd) int main_loop(void) { + struct addrinfo *peer = NULL; int fd = 0, ret, fd_in = 0; - struct addrinfo *peer; struct wstate winfo; if (cfg_input && cfg_sockopt_types.mptfo) { -- 2.51.0