diff mbox series

[mptcp-net,v3] mptcp: ensure snd_nxt is properly initialized on connect

Message ID 3b0ca23eaa8906f967a841f55bafed48d0f895cb.1713367699.git.pabeni@redhat.com (mailing list archive)
State Accepted, archived
Commit b3584792f65e88e4a86318096aafbbe3900f4292
Delegated to: Matthieu Baerts
Headers show
Series [mptcp-net,v3] mptcp: ensure snd_nxt is properly initialized on connect | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch fail total: 1 errors, 3 warnings, 0 checks, 23 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug warning Unstable: 1 failed test(s): packetdrill_sockopts
matttbe/KVM_Validation__btf__only_bpftest_all_ success Success! ✅

Commit Message

Paolo Abeni April 18, 2024, 7:44 a.m. UTC 
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 a client socket, snd_nxt
is not yet initialized and any incoming ack will copy such value
into snd_una. If the mptcp worker (dumbly) tries mptcp-level
re-injection after such ack, that would unconditionally trigger a send
buffer cleanup using 'bad' snd_una values.

We could easily disable re-injection for fallback sockets, but such
dumb behavior already helped catching a few subtle issues and a very
low to zero impact in practice.

Instead address the issue always initializing snd_nxt (and write_seq,
for consistency) at connect time.

Reported-by: Christoph Paasch <cpaasch@apple.com>
Tested-by: Christoph Paasch <cpaasch@apple.com>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/485
Fixes: 8fd738049ac3 ("mptcp: fallback in case of simultaneous connect")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v2 -> v3:
 - init snd_nxt/write_seq instead

v1 -> v2:
 - moved the initialization at established time (Mat)
 - dropped debug code wrongly landed in v1
 - added fixes tag
---
 net/mptcp/protocol.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

MPTCP CI April 18, 2024, 8:36 a.m. UTC | #1 
Hi Paolo,

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: Unstable: 1 failed test(s): packetdrill_sockopts
Mat Martineau April 18, 2024, 4:59 p.m. UTC | #2 
On Thu, 18 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 a client socket, snd_nxt
> is not yet initialized and any incoming ack will copy such value
> into snd_una. If the mptcp worker (dumbly) tries mptcp-level
> re-injection after such ack, that would unconditionally trigger a send
> buffer cleanup using 'bad' snd_una values.
>
> We could easily disable re-injection for fallback sockets, but such
> dumb behavior already helped catching a few subtle issues and a very
> low to zero impact in practice.
>
> Instead address the issue always initializing snd_nxt (and write_seq,
> for consistency) at connect time.
>
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Tested-by: Christoph Paasch <cpaasch@apple.com>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/485
> Fixes: 8fd738049ac3 ("mptcp: fallback in case of simultaneous connect")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v2 -> v3:
> - init snd_nxt/write_seq instead
>
> v1 -> v2:
> - moved the initialization at established time (Mat)
> - dropped debug code wrongly landed in v1
> - added fixes tag
> ---
> net/mptcp/protocol.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3e1b15d76442..4205cef2d45f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1002,8 +1002,13 @@ 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("sk state %d:%d snd_una %llx write_seq %llx dfrag end seq %llx frag len %d bytes sent %lld acked %lld",
> +				       sk->sk_state, msk->first ? msk->first->sk_state: -1, msk->write_seq,
> +				       msk->snd_una, dfrag->data_seq + dfrag->data_len, dfrag->data_len,
> +				       msk->bytes_sent, msk->bytes_acked);
> 				break;
> +			}

Hi Paolo -

You dropped this debug code in v2, do you want it included?

Otherwise, LGTM:

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

>
> 			WRITE_ONCE(msk->first_pending, mptcp_send_next(sk));
> 		}
> @@ -3730,6 +3735,9 @@ 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);
> 	}
> +
> +	WRITE_ONCE(msk->write_seq, subflow->idsn);
> +	WRITE_ONCE(msk->snd_nxt, subflow->idsn);
> 	if (likely(!__mptcp_check_fallback(msk)))
> 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
>
> -- 
> 2.43.2
>
>
>
Paolo Abeni April 19, 2024, 10:33 a.m. UTC | #3 
On Thu, 2024-04-18 at 09:59 -0700, Mat Martineau wrote:
> On Thu, 18 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 a client socket, snd_nxt
> > is not yet initialized and any incoming ack will copy such value
> > into snd_una. If the mptcp worker (dumbly) tries mptcp-level
> > re-injection after such ack, that would unconditionally trigger a send
> > buffer cleanup using 'bad' snd_una values.
> > 
> > We could easily disable re-injection for fallback sockets, but such
> > dumb behavior already helped catching a few subtle issues and a very
> > low to zero impact in practice.
> > 
> > Instead address the issue always initializing snd_nxt (and write_seq,
> > for consistency) at connect time.
> > 
> > Reported-by: Christoph Paasch <cpaasch@apple.com>
> > Tested-by: Christoph Paasch <cpaasch@apple.com>
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/485
> > Fixes: 8fd738049ac3 ("mptcp: fallback in case of simultaneous connect")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > v2 -> v3:
> > - init snd_nxt/write_seq instead
> > 
> > v1 -> v2:
> > - moved the initialization at established time (Mat)
> > - dropped debug code wrongly landed in v1
> > - added fixes tag
> > ---
> > net/mptcp/protocol.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 3e1b15d76442..4205cef2d45f 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1002,8 +1002,13 @@ 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("sk state %d:%d snd_una %llx write_seq %llx dfrag end seq %llx frag len %d bytes sent %lld acked %lld",
> > +				       sk->sk_state, msk->first ? msk->first->sk_state: -1, msk->write_seq,
> > +				       msk->snd_una, dfrag->data_seq + dfrag->data_len, dfrag->data_len,
> > +				       msk->bytes_sent, msk->bytes_acked);
> > 				break;
> > +			}
> 
> Hi Paolo -
> 
> You dropped this debug code in v2, do you want it included?

Nope, it's there by mistake. @Matttbe: could you please drop this chunk
while applying the patch or do you prefer me to resend the patch?

Thanks!

Paolo
Matthieu Baerts April 19, 2024, 10:37 a.m. UTC | #4 
Hi Paolo,

On 19/04/2024 12:33, Paolo Abeni wrote:
> On Thu, 2024-04-18 at 09:59 -0700, Mat Martineau wrote:
>> On Thu, 18 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 a client socket, snd_nxt
>>> is not yet initialized and any incoming ack will copy such value
>>> into snd_una. If the mptcp worker (dumbly) tries mptcp-level
>>> re-injection after such ack, that would unconditionally trigger a send
>>> buffer cleanup using 'bad' snd_una values.
>>>
>>> We could easily disable re-injection for fallback sockets, but such
>>> dumb behavior already helped catching a few subtle issues and a very
>>> low to zero impact in practice.
>>>
>>> Instead address the issue always initializing snd_nxt (and write_seq,
>>> for consistency) at connect time.
>>>
>>> Reported-by: Christoph Paasch <cpaasch@apple.com>
>>> Tested-by: Christoph Paasch <cpaasch@apple.com>
>>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/485
>>> Fixes: 8fd738049ac3 ("mptcp: fallback in case of simultaneous connect")
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> v2 -> v3:
>>> - init snd_nxt/write_seq instead
>>>
>>> v1 -> v2:
>>> - moved the initialization at established time (Mat)
>>> - dropped debug code wrongly landed in v1
>>> - added fixes tag
>>> ---
>>> net/mptcp/protocol.c | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 3e1b15d76442..4205cef2d45f 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -1002,8 +1002,13 @@ 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("sk state %d:%d snd_una %llx write_seq %llx dfrag end seq %llx frag len %d bytes sent %lld acked %lld",
>>> +				       sk->sk_state, msk->first ? msk->first->sk_state: -1, msk->write_seq,
>>> +				       msk->snd_una, dfrag->data_seq + dfrag->data_len, dfrag->data_len,
>>> +				       msk->bytes_sent, msk->bytes_acked);
>>> 				break;
>>> +			}
>>
>> Hi Paolo -
>>
>> You dropped this debug code in v2, do you want it included?
> 
> Nope, it's there by mistake. @Matttbe: could you please drop this chunk
> while applying the patch or do you prefer me to resend the patch?

No problem, I can drop it when applying it.

Cheers,
Matt
Matthieu Baerts April 19, 2024, 3:15 p.m. UTC | #5 
Hi Paolo, Mat, Christoph,

On 18/04/2024 09:44, Paolo Abeni wrote:
> Christoph reported a splat hinting at a corrupted snd_una:
Thank you for the patch, the review, and the bug report + test.

I forgot to send a notification after I applied this patch. In between,
the CI already validated this, and both the build for export and
export-net have the same issue with Packetdrill (fastopen):

- export:

> not ok 32 packetdrill: mptcp/fastopen/server-TCP_FASTOPEN-cookie-req.pkt (ipv4-mapped-v6)
> # server-TCP_FASTOPEN-cookie-req.pkt:23: error handling packet: live packet field ipv4_total_length: expected: 48 (0x30) vs actual: 64 (0x40)
> # script packet:  0.660855 . 1:1(0) ack 1 <dss dack4 3007449510 flags: A>
> # actual packet:  0.651162 . 1:1(0) ack 1 win 256 <dss dack4 3007449509 dsn8 4250179620018965011 ssn 0 dll 1 no_checksum flags: MmAF,nop,nop>

 https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8752422517

- export-net:

> not ok 32 packetdrill: mptcp/fastopen/server-TCP_FASTOPEN-cookie-req.pkt (ipv4-mapped-v6)
> # server-TCP_FASTOPEN-cookie-req.pkt:23: error handling packet: live packet field ipv4_total_length: expected: 48 (0x30) vs actual: 64 (0x40)
> # script packet:  0.667029 . 1:1(0) ack 1 <dss dack4 3007449510 flags: A>
> # actual packet:  0.665988 . 1:1(0) ack 1 win 256 <dss dack4 3007449509 dsn8 4812888791396531368 ssn 0 dll 1 no_checksum flags: MmAF,nop,nop>

 https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8752426856

Do you think this is linked?


New patches for t/upstream-net and t/upstream:
- b3584792f65e: mptcp: ensure snd_nxt is properly initialized on connect
- Results: 428f275bfbb0..e065a4657da5 (export-net)
- Results: b88f530f98b9..47bb272bba61 (export)

Cheers,
Matt
Paolo Abeni April 19, 2024, 4:58 p.m. UTC | #6 
On Fri, 2024-04-19 at 17:15 +0200, Matthieu Baerts wrote:
> Hi Paolo, Mat, Christoph,
> 
> On 18/04/2024 09:44, Paolo Abeni wrote:
> > Christoph reported a splat hinting at a corrupted snd_una:
> Thank you for the patch, the review, and the bug report + test.
> 
> I forgot to send a notification after I applied this patch. In between,
> the CI already validated this, and both the build for export and
> export-net have the same issue with Packetdrill (fastopen):
> 
> - export:
> 
> > not ok 32 packetdrill: mptcp/fastopen/server-TCP_FASTOPEN-cookie-req.pkt (ipv4-mapped-v6)
> > # server-TCP_FASTOPEN-cookie-req.pkt:23: error handling packet: live packet field ipv4_total_length: expected: 48 (0x30) vs actual: 64 (0x40)
> > # script packet:  0.660855 . 1:1(0) ack 1 <dss dack4 3007449510 flags: A>
> > # actual packet:  0.651162 . 1:1(0) ack 1 win 256 <dss dack4 3007449509 dsn8 4250179620018965011 ssn 0 dll 1 no_checksum flags: MmAF,nop,nop>
> 
>  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8752422517
> 
> - export-net:
> 
> > not ok 32 packetdrill: mptcp/fastopen/server-TCP_FASTOPEN-cookie-req.pkt (ipv4-mapped-v6)
> > # server-TCP_FASTOPEN-cookie-req.pkt:23: error handling packet: live packet field ipv4_total_length: expected: 48 (0x30) vs actual: 64 (0x40)
> > # script packet:  0.667029 . 1:1(0) ack 1 <dss dack4 3007449510 flags: A>
> > # actual packet:  0.665988 . 1:1(0) ack 1 win 256 <dss dack4 3007449509 dsn8 4812888791396531368 ssn 0 dll 1 no_checksum flags: MmAF,nop,nop>
> 
>  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8752426856
> 
> Do you think this is linked?

I hope it's not, but the timing hints otherwise :( I'll try to have a
better look.

Thanks for the head-up!

Paolo
diff mbox series

Patch

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3e1b15d76442..4205cef2d45f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1002,8 +1002,13 @@  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("sk state %d:%d snd_una %llx write_seq %llx dfrag end seq %llx frag len %d bytes sent %lld acked %lld",
+				       sk->sk_state, msk->first ? msk->first->sk_state: -1, msk->write_seq,
+				       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 +3735,9 @@  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);
 	}
+
+	WRITE_ONCE(msk->write_seq, subflow->idsn);
+	WRITE_ONCE(msk->snd_nxt, subflow->idsn);
 	if (likely(!__mptcp_check_fallback(msk)))
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);