1. Patchew
  2. MPTCP
  3. View series

[PATCH mptcp-net] mptcp: ensure snd_una is properly initialized on connect

Paolo Abeni posted 1 patch 3 weeks, 1 day ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/2b6d11d0210f683cc0de8bb47a923d568cfa8f47.1712942966.git.pabeni@redhat.com
There is a newer version of this series
net/mptcp/protocol.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
[PATCH mptcp-net] mptcp: ensure snd_una is properly initialized on connect
Posted by Paolo Abeni 3 weeks, 1 day ago 
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);
 	if (likely(!__mptcp_check_fallback(msk)))
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
 
-- 
2.43.2
Re: [PATCH mptcp-net] mptcp: ensure snd_una is properly initialized on connect
Posted by Mat Martineau 3 weeks, 1 day ago 
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
Re: [PATCH mptcp-net] mptcp: ensure snd_una is properly initialized on connect
Posted by Paolo Abeni 2 weeks, 6 days ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.