| Message ID | 2b6d11d0210f683cc0de8bb47a923d568cfa8f47.1712942966.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [mptcp-net] mptcp: ensure snd_una is properly initialized on connect | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | warning | total: 0 errors, 1 warnings, 1 checks, 26 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | warning | Unstable: 1 failed test(s): packetdrill_sockopts |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
Hi Paolo, Thank you for your modifications, that's great! Our CI did some validations and here is its report: - KVM Validation: normal: Unstable: 1 failed test(s): packetdrill_sockopts
On Fri, 12 Apr 2024, Paolo Abeni wrote: > Christoph reported a splat hinting at a corrupted snd_una: > > WARNING: CPU: 1 PID: 38 at net/mptcp/protocol.c:1005 __mptcp_clean_una+0x4b3/0x620 net/mptcp/protocol.c:1005 > Modules linked in: > CPU: 1 PID: 38 Comm: kworker/1:1 Not tainted 6.9.0-rc1-gbbeac67456c9 #59 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014 > Workqueue: events mptcp_worker > RIP: 0010:__mptcp_clean_una+0x4b3/0x620 net/mptcp/protocol.c:1005 > Code: be 06 01 00 00 bf 06 01 00 00 e8 a8 12 e7 fe e9 00 fe ff ff e8 > 8e 1a e7 fe 0f b7 ab 3e 02 00 00 e9 d3 fd ff ff e8 7d 1a e7 fe > <0f> 0b 4c 8b bb e0 05 00 00 e9 74 fc ff ff e8 6a 1a e7 fe 0f 0b e9 > RSP: 0018:ffffc9000013fd48 EFLAGS: 00010293 > RAX: 0000000000000000 RBX: ffff8881029bd280 RCX: ffffffff82382fe4 > RDX: ffff8881003cbd00 RSI: ffffffff823833c3 RDI: 0000000000000001 > RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000 > R10: 0000000000000000 R11: fefefefefefefeff R12: ffff888138ba8000 > R13: 0000000000000106 R14: ffff8881029bd908 R15: ffff888126560000 > FS: 0000000000000000(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007f604a5dae38 CR3: 0000000101dac002 CR4: 0000000000170ef0 > Call Trace: > <TASK> > __mptcp_clean_una_wakeup net/mptcp/protocol.c:1055 [inline] > mptcp_clean_una_wakeup net/mptcp/protocol.c:1062 [inline] > __mptcp_retrans+0x7f/0x7e0 net/mptcp/protocol.c:2615 > mptcp_worker+0x434/0x740 net/mptcp/protocol.c:2767 > process_one_work+0x1e0/0x560 kernel/workqueue.c:3254 > process_scheduled_works kernel/workqueue.c:3335 [inline] > worker_thread+0x3c7/0x640 kernel/workqueue.c:3416 > kthread+0x121/0x170 kernel/kthread.c:388 > ret_from_fork+0x44/0x50 arch/x86/kernel/process.c:147 > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:243 > </TASK> > > When fallback to TCP happens early on client socket, and the mptcp > worker (dumbly) tries mptcp-level re-injection, the snd_una is > not yet initialized, and the worker tries to use it to clean-up > the send buffer. > > Address the issue always initializing snd_una for active sockets > at connect time. > > Reported-by: Christoph Paasch <cpaasch@apple.com> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/485 > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > @Christoph: could you please test this vs the above issue reproducer? > The repro doesn't work for me. > --- > net/mptcp/protocol.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 3e1b15d76442..68247075069b 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -1002,8 +1002,12 @@ static void __mptcp_clean_una(struct sock *sk) > > if (unlikely(dfrag == msk->first_pending)) { > /* in recovery mode can see ack after the current snd head */ > - if (WARN_ON_ONCE(!msk->recovery)) > + if (WARN_ON_ONCE(!msk->recovery)) { > + pr_err("snd_una %llx dfrag end seq %llx frag len %d bytes sent %lld acked %lld", > + msk->snd_una, dfrag->data_seq + dfrag->data_len, dfrag->data_len, > + msk->bytes_sent, msk->bytes_acked); > break; > + } > > WRITE_ONCE(msk->first_pending, mptcp_send_next(sk)); > } > @@ -3730,6 +3734,13 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) > MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_TOKENFALLBACKINIT); > mptcp_subflow_early_fallback(msk, subflow); > } > + > + /* after the connect, the worker (and re-injections) can trigger > + * at any time, and that will need snd_una properly initialized > + */ > + mptcp_data_lock(sk); > + WRITE_ONCE(msk->snd_una, subflow->idsn); > + mptcp_data_unlock(sk); Hi Paolo - msk->write_seq, msk->snd_nxt, and msk->snd_una are initialized as a group in mptcp_sk_clone_init() (on the accept() side). Does it make sense to also initialize them together on the connect() side? Looks like msk->write_seq and msk->snd_nxt are initialized together in __mptcp_sync_state() and I think setting msk->snd_una would fit better there. The attempt to re-inject means that a transmit happened, so msk->write_seq and msk->snd_nxt must have been initialized in __mptcp_sync_state(). - Mat
On Fri, 2024-04-12 at 18:22 +0000, MPTCP CI wrote: > Hi Paolo, > > Thank you for your modifications, that's great! > > Our CI did some validations and here is its report: > > - KVM Validation: normal: Unstable: 1 failed test(s): packetdrill_sockopts
On Fri, 2024-04-12 at 14:05 -0700, Mat Martineau wrote: > On Fri, 12 Apr 2024, Paolo Abeni wrote: > > > Christoph reported a splat hinting at a corrupted snd_una: > > > > WARNING: CPU: 1 PID: 38 at net/mptcp/protocol.c:1005 __mptcp_clean_una+0x4b3/0x620 net/mptcp/protocol.c:1005 > > Modules linked in: > > CPU: 1 PID: 38 Comm: kworker/1:1 Not tainted 6.9.0-rc1-gbbeac67456c9 #59 > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014 > > Workqueue: events mptcp_worker > > RIP: 0010:__mptcp_clean_una+0x4b3/0x620 net/mptcp/protocol.c:1005 > > Code: be 06 01 00 00 bf 06 01 00 00 e8 a8 12 e7 fe e9 00 fe ff ff e8 > > 8e 1a e7 fe 0f b7 ab 3e 02 00 00 e9 d3 fd ff ff e8 7d 1a e7 fe > > <0f> 0b 4c 8b bb e0 05 00 00 e9 74 fc ff ff e8 6a 1a e7 fe 0f 0b e9 > > RSP: 0018:ffffc9000013fd48 EFLAGS: 00010293 > > RAX: 0000000000000000 RBX: ffff8881029bd280 RCX: ffffffff82382fe4 > > RDX: ffff8881003cbd00 RSI: ffffffff823833c3 RDI: 0000000000000001 > > RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000 > > R10: 0000000000000000 R11: fefefefefefefeff R12: ffff888138ba8000 > > R13: 0000000000000106 R14: ffff8881029bd908 R15: ffff888126560000 > > FS: 0000000000000000(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 00007f604a5dae38 CR3: 0000000101dac002 CR4: 0000000000170ef0 > > Call Trace: > > <TASK> > > __mptcp_clean_una_wakeup net/mptcp/protocol.c:1055 [inline] > > mptcp_clean_una_wakeup net/mptcp/protocol.c:1062 [inline] > > __mptcp_retrans+0x7f/0x7e0 net/mptcp/protocol.c:2615 > > mptcp_worker+0x434/0x740 net/mptcp/protocol.c:2767 > > process_one_work+0x1e0/0x560 kernel/workqueue.c:3254 > > process_scheduled_works kernel/workqueue.c:3335 [inline] > > worker_thread+0x3c7/0x640 kernel/workqueue.c:3416 > > kthread+0x121/0x170 kernel/kthread.c:388 > > ret_from_fork+0x44/0x50 arch/x86/kernel/process.c:147 > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:243 > > </TASK> > > > > When fallback to TCP happens early on client socket, and the mptcp > > worker (dumbly) tries mptcp-level re-injection, the snd_una is > > not yet initialized, and the worker tries to use it to clean-up > > the send buffer. > > > > Address the issue always initializing snd_una for active sockets > > at connect time. > > > > Reported-by: Christoph Paasch <cpaasch@apple.com> > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/485 > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > @Christoph: could you please test this vs the above issue reproducer? > > The repro doesn't work for me. > > --- > > net/mptcp/protocol.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > index 3e1b15d76442..68247075069b 100644 > > --- a/net/mptcp/protocol.c > > +++ b/net/mptcp/protocol.c > > @@ -1002,8 +1002,12 @@ static void __mptcp_clean_una(struct sock *sk) > > > > if (unlikely(dfrag == msk->first_pending)) { > > /* in recovery mode can see ack after the current snd head */ > > - if (WARN_ON_ONCE(!msk->recovery)) > > + if (WARN_ON_ONCE(!msk->recovery)) { > > + pr_err("snd_una %llx dfrag end seq %llx frag len %d bytes sent %lld acked %lld", > > + msk->snd_una, dfrag->data_seq + dfrag->data_len, dfrag->data_len, > > + msk->bytes_sent, msk->bytes_acked); > > break; > > + } > > > > WRITE_ONCE(msk->first_pending, mptcp_send_next(sk)); > > } > > @@ -3730,6 +3734,13 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) > > MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_TOKENFALLBACKINIT); > > mptcp_subflow_early_fallback(msk, subflow); > > } > > + > > + /* after the connect, the worker (and re-injections) can trigger > > + * at any time, and that will need snd_una properly initialized > > + */ > > + mptcp_data_lock(sk); > > + WRITE_ONCE(msk->snd_una, subflow->idsn); > > + mptcp_data_unlock(sk); > > Hi Paolo - > > msk->write_seq, msk->snd_nxt, and msk->snd_una are initialized as a group > in mptcp_sk_clone_init() (on the accept() side). > > Does it make sense to also initialize them together on the connect() side? Could make sense for consistency, but it should not solve any additional issues. > Looks like msk->write_seq and msk->snd_nxt are initialized together in > __mptcp_sync_state() and I think setting msk->snd_una would fit better > there. The attempt to re-inject means that a transmit happened, so > msk->write_seq and msk->snd_nxt must have been initialized in > __mptcp_sync_state(). I think you are right. Moving the initialization there should be cleaner and equally safer. Let me do some more testing, Thanks! Paolo
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 3e1b15d76442..68247075069b 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1002,8 +1002,12 @@ static void __mptcp_clean_una(struct sock *sk) if (unlikely(dfrag == msk->first_pending)) { /* in recovery mode can see ack after the current snd head */ - if (WARN_ON_ONCE(!msk->recovery)) + if (WARN_ON_ONCE(!msk->recovery)) { + pr_err("snd_una %llx dfrag end seq %llx frag len %d bytes sent %lld acked %lld", + msk->snd_una, dfrag->data_seq + dfrag->data_len, dfrag->data_len, + msk->bytes_sent, msk->bytes_acked); break; + } WRITE_ONCE(msk->first_pending, mptcp_send_next(sk)); } @@ -3730,6 +3734,13 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_TOKENFALLBACKINIT); mptcp_subflow_early_fallback(msk, subflow); } + + /* after the connect, the worker (and re-injections) can trigger + * at any time, and that will need snd_una properly initialized + */ + mptcp_data_lock(sk); + WRITE_ONCE(msk->snd_una, subflow->idsn); + mptcp_data_unlock(sk); if (likely(!__mptcp_check_fallback(msk))) MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
Christoph reported a splat hinting at a corrupted snd_una: WARNING: CPU: 1 PID: 38 at net/mptcp/protocol.c:1005 __mptcp_clean_una+0x4b3/0x620 net/mptcp/protocol.c:1005 Modules linked in: CPU: 1 PID: 38 Comm: kworker/1:1 Not tainted 6.9.0-rc1-gbbeac67456c9 #59 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014 Workqueue: events mptcp_worker RIP: 0010:__mptcp_clean_una+0x4b3/0x620 net/mptcp/protocol.c:1005 Code: be 06 01 00 00 bf 06 01 00 00 e8 a8 12 e7 fe e9 00 fe ff ff e8 8e 1a e7 fe 0f b7 ab 3e 02 00 00 e9 d3 fd ff ff e8 7d 1a e7 fe <0f> 0b 4c 8b bb e0 05 00 00 e9 74 fc ff ff e8 6a 1a e7 fe 0f 0b e9 RSP: 0018:ffffc9000013fd48 EFLAGS: 00010293 RAX: 0000000000000000 RBX: ffff8881029bd280 RCX: ffffffff82382fe4 RDX: ffff8881003cbd00 RSI: ffffffff823833c3 RDI: 0000000000000001 RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000 R10: 0000000000000000 R11: fefefefefefefeff R12: ffff888138ba8000 R13: 0000000000000106 R14: ffff8881029bd908 R15: ffff888126560000 FS: 0000000000000000(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f604a5dae38 CR3: 0000000101dac002 CR4: 0000000000170ef0 Call Trace: <TASK> __mptcp_clean_una_wakeup net/mptcp/protocol.c:1055 [inline] mptcp_clean_una_wakeup net/mptcp/protocol.c:1062 [inline] __mptcp_retrans+0x7f/0x7e0 net/mptcp/protocol.c:2615 mptcp_worker+0x434/0x740 net/mptcp/protocol.c:2767 process_one_work+0x1e0/0x560 kernel/workqueue.c:3254 process_scheduled_works kernel/workqueue.c:3335 [inline] worker_thread+0x3c7/0x640 kernel/workqueue.c:3416 kthread+0x121/0x170 kernel/kthread.c:388 ret_from_fork+0x44/0x50 arch/x86/kernel/process.c:147 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:243 </TASK> When fallback to TCP happens early on client socket, and the mptcp worker (dumbly) tries mptcp-level re-injection, the snd_una is not yet initialized, and the worker tries to use it to clean-up the send buffer. Address the issue always initializing snd_una for active sockets at connect time. Reported-by: Christoph Paasch <cpaasch@apple.com> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/485 Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- @Christoph: could you please test this vs the above issue reproducer? The repro doesn't work for me. --- net/mptcp/protocol.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)