Squash-to: mptcp: refactor passive socket initialization

[PATCH mptcp-next] Squash-to: mptcp: refactor passive socket initialization

Posted by Paolo Abeni 1 year, 2 months ago

Since the squash-to commit, msk->first is set even for unaccepted
sockets: mptcp_mp_fail_no_response() can take action and reset
the subflow even when msk->first is still in the accept queue,
after that incoming reset already closed such socket.

That in turn will end-up calling tcp_done() - via mptcp_subflow_reset()
- on already closed sockets, which is illegal.

Let's fix the issue explicitly checking the subflow socket status
in mptcp_subflow_reset().

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Since is a squash-to patch this will likely lose the reported-by
tag, but perhaps we can preserve the:
Tested-by: Christoph Paasch <cpaasch@apple.com>
Also:
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/357
but I think it's better to handle the issue workflow manually here
---
 net/mptcp/subflow.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index a14c3a0c7c00..28c64811a8af 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -397,6 +397,12 @@ void mptcp_subflow_reset(struct sock *ssk)
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	struct sock *sk = subflow->conn;
 
+	/* mptcp_mp_fail_no_response() can reach here on an already closed
+	 * socket
+	 */
+	if (ssk->sk_state == TCP_CLOSE)
+		return;
+
 	/* must hold: tcp_done() could drop last reference on parent */
 	sock_hold(sk);
 
-- 
2.39.2

Re: [PATCH mptcp-next] Squash-to: mptcp: refactor passive socket initialization

Posted by Matthieu Baerts 1 year, 2 months ago

Hi Paolo,

On 20/02/2023 15:27, Paolo Abeni wrote:
> Since the squash-to commit, msk->first is set even for unaccepted
> sockets: mptcp_mp_fail_no_response() can take action and reset
> the subflow even when msk->first is still in the accept queue,
> after that incoming reset already closed such socket.
> 
> That in turn will end-up calling tcp_done() - via mptcp_subflow_reset()
> - on already closed sockets, which is illegal.

Should we not add a WARN_ON() there for our CI (and syzkaller) to catch
that earlier?

> Let's fix the issue explicitly checking the subflow socket status
> in mptcp_subflow_reset().

Thank you for the patch, it makes sense!


New patches for t/upstream:
- 7dd3244ceddc: Squash-to: mptcp: refactor passive socket initialization
- Results: 07db02c0176e..1e2b17c72708 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230220T145128


> Since is a squash-to patch this will likely lose the reported-by
> tag, but perhaps we can preserve the:
> Tested-by: Christoph Paasch <cpaasch@apple.com>

I added this on the refactor commit.

- 74e8c30799a7: tg:msg: add Christoph' Tested-by tag

> Also:
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/357
> but I think it's better to handle the issue workflow manually here

Indeed, I can close it manually.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] Squash-to: mptcp: refactor passive socket initialization

Posted by Paolo Abeni 1 year, 2 months ago

On Mon, 2023-02-20 at 16:01 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 20/02/2023 15:27, Paolo Abeni wrote:
> > Since the squash-to commit, msk->first is set even for unaccepted
> > sockets: mptcp_mp_fail_no_response() can take action and reset
> > the subflow even when msk->first is still in the accept queue,
> > after that incoming reset already closed such socket.
> > 
> > That in turn will end-up calling tcp_done() - via mptcp_subflow_reset()
> > - on already closed sockets, which is illegal.
> 
> Should we not add a WARN_ON() there for our CI (and syzkaller) to catch
> that earlier?

If you mean as a non upstreamable patch touching tcp_done(), that could
be useful.

I think it's better not touching the upstream code enforcing such
constraint (either tcp or mptcp one) because it should be an "obvious"
one.

Cheers,

Paolo

Re: [PATCH mptcp-next] Squash-to: mptcp: refactor passive socket initialization

Posted by Matthieu Baerts 1 year, 2 months ago


On 20/02/2023 17:09, Paolo Abeni wrote:
> On Mon, 2023-02-20 at 16:01 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 20/02/2023 15:27, Paolo Abeni wrote:
>>> Since the squash-to commit, msk->first is set even for unaccepted
>>> sockets: mptcp_mp_fail_no_response() can take action and reset
>>> the subflow even when msk->first is still in the accept queue,
>>> after that incoming reset already closed such socket.
>>>
>>> That in turn will end-up calling tcp_done() - via mptcp_subflow_reset()
>>> - on already closed sockets, which is illegal.
>>
>> Should we not add a WARN_ON() there for our CI (and syzkaller) to catch
>> that earlier?
> 
> If you mean as a non upstreamable patch touching tcp_done(), that could
> be useful.

Yes, a non upstreamable patch, similar to:

  mptcp: use kmalloc on kasan build

> I think it's better not touching the upstream code enforcing such
> constraint (either tcp or mptcp one) because it should be an "obvious"
> one.

Indeed but it is just to warn once before having real issues :)
But it makes sense not to upstream this.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] Squash-to: mptcp: refactor passive socket initialization

Posted by Matthieu Baerts 1 year, 2 months ago

On 20/02/2023 17:26, Matthieu Baerts wrote:
> 
> 
> On 20/02/2023 17:09, Paolo Abeni wrote:
>> On Mon, 2023-02-20 at 16:01 +0100, Matthieu Baerts wrote:
>>> Hi Paolo,
>>>
>>> On 20/02/2023 15:27, Paolo Abeni wrote:
>>>> Since the squash-to commit, msk->first is set even for unaccepted
>>>> sockets: mptcp_mp_fail_no_response() can take action and reset
>>>> the subflow even when msk->first is still in the accept queue,
>>>> after that incoming reset already closed such socket.
>>>>
>>>> That in turn will end-up calling tcp_done() - via mptcp_subflow_reset()
>>>> - on already closed sockets, which is illegal.
>>>
>>> Should we not add a WARN_ON() there for our CI (and syzkaller) to catch
>>> that earlier?
>>
>> If you mean as a non upstreamable patch touching tcp_done(), that could
>> be useful.
> 
> Yes, a non upstreamable patch, similar to:
> 
>   mptcp: use kmalloc on kasan build
> 
>> I think it's better not touching the upstream code enforcing such
>> constraint (either tcp or mptcp one) because it should be an "obvious"
>> one.
> 
> Indeed but it is just to warn once before having real issues :)
> But it makes sense not to upstream this.

I just tried this patch on top of the export branch and I got a warning
with the 2nd test of "mptcp_join.sh" :-D

------------------- 8< -------------------
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 33f559f491c8..d21ec9bdd453 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4639,6 +4639,8 @@ void tcp_done(struct sock *sk)
         */
        req = rcu_dereference_protected(tcp_sk(sk)->fastopen_rsk, 1);

+       WARN_ON_ONCE(sk->sk_state == TCP_CLOSE);
+
        if (sk->sk_state == TCP_SYN_SENT || sk->sk_state == TCP_SYN_RECV)
                TCP_INC_STATS(sock_net(sk), TCP_MIB_ATTEMPTFAILS);


------------------- 8< -------------------

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] Squash-to: mptcp: refactor passive socket initialization

Posted by Paolo Abeni 1 year, 2 months ago

On Mon, 2023-02-20 at 17:43 +0100, Matthieu Baerts wrote:
> On 20/02/2023 17:26, Matthieu Baerts wrote:
> > 
> > 
> > On 20/02/2023 17:09, Paolo Abeni wrote:
> > > On Mon, 2023-02-20 at 16:01 +0100, Matthieu Baerts wrote:
> > > > Hi Paolo,
> > > > 
> > > > On 20/02/2023 15:27, Paolo Abeni wrote:
> > > > > Since the squash-to commit, msk->first is set even for unaccepted
> > > > > sockets: mptcp_mp_fail_no_response() can take action and reset
> > > > > the subflow even when msk->first is still in the accept queue,
> > > > > after that incoming reset already closed such socket.
> > > > > 
> > > > > That in turn will end-up calling tcp_done() - via mptcp_subflow_reset()
> > > > > - on already closed sockets, which is illegal.
> > > > 
> > > > Should we not add a WARN_ON() there for our CI (and syzkaller) to catch
> > > > that earlier?
> > > 
> > > If you mean as a non upstreamable patch touching tcp_done(), that could
> > > be useful.
> > 
> > Yes, a non upstreamable patch, similar to:
> > 
> >   mptcp: use kmalloc on kasan build
> > 
> > > I think it's better not touching the upstream code enforcing such
> > > constraint (either tcp or mptcp one) because it should be an "obvious"
> > > one.
> > 
> > Indeed but it is just to warn once before having real issues :)
> > But it makes sense not to upstream this.
> 
> I just tried this patch on top of the export branch and I got a warning
> with the 2nd test of "mptcp_join.sh" :-D
> 
> ------------------- 8< -------------------
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 33f559f491c8..d21ec9bdd453 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4639,6 +4639,8 @@ void tcp_done(struct sock *sk)
>          */
>         req = rcu_dereference_protected(tcp_sk(sk)->fastopen_rsk, 1);
> 
> +       WARN_ON_ONCE(sk->sk_state == TCP_CLOSE);
> +
>         if (sk->sk_state == TCP_SYN_SENT || sk->sk_state == TCP_SYN_RECV)
>                 TCP_INC_STATS(sock_net(sk), TCP_MIB_ATTEMPTFAILS);
> 
> 
> ------------------- 8< -------------------

Could you please share the (decoded) stack trace?

Thanks!

/P

Re: [PATCH mptcp-next] Squash-to: mptcp: refactor passive socket initialization

Posted by Matthieu Baerts 1 year, 2 months ago

Hi Paolo,

On 21/02/2023 16:18, Paolo Abeni wrote:
> On Mon, 2023-02-20 at 17:43 +0100, Matthieu Baerts wrote:
>> On 20/02/2023 17:26, Matthieu Baerts wrote:
>>>
>>>
>>> On 20/02/2023 17:09, Paolo Abeni wrote:
>>>> On Mon, 2023-02-20 at 16:01 +0100, Matthieu Baerts wrote:
>>>>> Hi Paolo,
>>>>>
>>>>> On 20/02/2023 15:27, Paolo Abeni wrote:
>>>>>> Since the squash-to commit, msk->first is set even for unaccepted
>>>>>> sockets: mptcp_mp_fail_no_response() can take action and reset
>>>>>> the subflow even when msk->first is still in the accept queue,
>>>>>> after that incoming reset already closed such socket.
>>>>>>
>>>>>> That in turn will end-up calling tcp_done() - via mptcp_subflow_reset()
>>>>>> - on already closed sockets, which is illegal.
>>>>>
>>>>> Should we not add a WARN_ON() there for our CI (and syzkaller) to catch
>>>>> that earlier?
>>>>
>>>> If you mean as a non upstreamable patch touching tcp_done(), that could
>>>> be useful.
>>>
>>> Yes, a non upstreamable patch, similar to:
>>>
>>>   mptcp: use kmalloc on kasan build
>>>
>>>> I think it's better not touching the upstream code enforcing such
>>>> constraint (either tcp or mptcp one) because it should be an "obvious"
>>>> one.
>>>
>>> Indeed but it is just to warn once before having real issues :)
>>> But it makes sense not to upstream this.
>>
>> I just tried this patch on top of the export branch and I got a warning
>> with the 2nd test of "mptcp_join.sh" :-D
>>
>> ------------------- 8< -------------------
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index 33f559f491c8..d21ec9bdd453 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -4639,6 +4639,8 @@ void tcp_done(struct sock *sk)
>>          */
>>         req = rcu_dereference_protected(tcp_sk(sk)->fastopen_rsk, 1);
>>
>> +       WARN_ON_ONCE(sk->sk_state == TCP_CLOSE);
>> +
>>         if (sk->sk_state == TCP_SYN_SENT || sk->sk_state == TCP_SYN_RECV)
>>                 TCP_INC_STATS(sock_net(sk), TCP_MIB_ATTEMPTFAILS);
>>
>>
>> ------------------- 8< -------------------
> 
> Could you please share the (decoded) stack trace?

Thank you for your reply.

I was planning to look at but I didn't manage to before. I just filled
in a new ticket with the patch, the stack trace and a first quick analysis:

  https://github.com/multipath-tcp/mptcp_net-next/issues/362

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net