[PATCH mptcp-net 0/2] mptcp: a couple of fixup

[PATCH mptcp-net 0/2] mptcp: a couple of fixup

[Paolo Abeni]

Hopefully addressing for good issues/371

There are actually several different problems that pop-up in random
order with the repro from such issue.

@Christoph: could you please have a spin in your testbed?

Paolo Abeni (2):
  Squash-to: "mptcp: fix accept vs worker race"
  Squash-to: "mptcp: stops worker on unaccepted sockets at listener
    close"

 net/mptcp/protocol.c | 13 +++++++++++--
 net/mptcp/subflow.c  |  4 ++--
 2 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.39.2

[PATCH mptcp-net 1/2] Squash-to: "mptcp: fix accept vs worker race"

[Paolo Abeni]

2 separate fixes needed for the mentioned patch

On some exceptional scenarios (e.g. listener socket disconnecting while
the subflow is created) a newly created MPC is deleted by the TCP stack
via inet_child_forget(). When that happen we leak the subflow context
and the msk.

Address the issue explicitly detecting such scenario and forcing a full
msk shutdown.

mptcp_subflow_drop_ctx() does not remove the subflow from the msk list,
racing msk lookup done by the NL PM could fetch the msk just before
deletion, and try to free again the subflow.

Just explicitly remove the subflow from said list.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 13 +++++++++++--
 net/mptcp/subflow.c  |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0e4961e87b48..76c1814c0b19 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2378,9 +2378,18 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	 * to deliver the msk to user-space.
 	 * Do nothing at the moment and take action at accept and/or listener
 	 * shutdown.
+	 * If instead such subflow has been destroyed, e.g. by inet_child_forget
+	 * do the kill
 	 */
-	if (msk->in_accept_queue && msk->first == ssk)
-		return;
+	if (msk->in_accept_queue && msk->first == ssk) {
+		if (!sock_flag(ssk, SOCK_DEAD))
+			return;
+
+		/* ensure later check in mptcp_worker will dispose the msk */
+		sock_set_flag(sk, SOCK_DEAD);
+		inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32 -
+							  TCP_TIMEWAIT_LEN -1;
+	}
 
 	dispose_it = !msk->subflow || ssk != msk->subflow->sk;
 	if (dispose_it)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 3d6ccf5067d7..32e54b7fdbbc 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -715,6 +715,7 @@ void mptcp_subflow_drop_ctx(struct sock *ssk)
 	if (!ctx)
 		return;
 
+	list_del(&mptcp_subflow_ctx(ssk)->node);
 	subflow_ulp_fallback(ssk, ctx);
 	if (ctx->conn)
 		sock_put(ctx->conn);
-- 
2.39.2

Re: [PATCH mptcp-net 1/2] Squash-to: "mptcp: fix accept vs worker race"

[Matthieu Baerts]

Hi Paolo,

Thank you for the patch!

It looks good to me but I just have some questions if you don't mind,
just to be sure I understood the modifications.

On 06/04/2023 00:47, Paolo Abeni wrote:
> 2 separate fixes needed for the mentioned patch
> 
> On some exceptional scenarios (e.g. listener socket disconnecting while
> the subflow is created) a newly created MPC is deleted by the TCP stack
> via inet_child_forget(). When that happen we leak the subflow context
> and the msk.
> 
> Address the issue explicitly detecting such scenario and forcing a full
> msk shutdown.
> 
> mptcp_subflow_drop_ctx() does not remove the subflow from the msk list,
> racing msk lookup done by the NL PM could fetch the msk just before
> deletion, and try to free again the subflow.
> 
> Just explicitly remove the subflow from said list.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/protocol.c | 13 +++++++++++--
>  net/mptcp/subflow.c  |  1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0e4961e87b48..76c1814c0b19 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2378,9 +2378,18 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	 * to deliver the msk to user-space.
>  	 * Do nothing at the moment and take action at accept and/or listener
>  	 * shutdown.
> +	 * If instead such subflow has been destroyed, e.g. by inet_child_forget
> +	 * do the kill
>  	 */
> -	if (msk->in_accept_queue && msk->first == ssk)
> -		return;
> +	if (msk->in_accept_queue && msk->first == ssk) {
> +		if (!sock_flag(ssk, SOCK_DEAD))
> +			return;
> +
> +		/* ensure later check in mptcp_worker will dispose the msk */
> +		sock_set_flag(sk, SOCK_DEAD);

Is it OK to mark the msk as dead and continue below? I guess yes because
there will not be any data to re-inject anyway?

> +		inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32 -
> +							  TCP_TIMEWAIT_LEN -1;

Changing the timestamp looks like an ugly hack :)

But I guess it is needed not to wait for nothing and there is no clearer
way? Or move this logic to the worker?

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

Re: [PATCH mptcp-net 1/2] Squash-to: "mptcp: fix accept vs worker race"

[Paolo Abeni]

On Thu, 2023-04-06 at 11:19 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> Thank you for the patch!
> 
> It looks good to me but I just have some questions if you don't mind,
> just to be sure I understood the modifications.
> 
> On 06/04/2023 00:47, Paolo Abeni wrote:
> > 2 separate fixes needed for the mentioned patch
> > 
> > On some exceptional scenarios (e.g. listener socket disconnecting while
> > the subflow is created) a newly created MPC is deleted by the TCP stack
> > via inet_child_forget(). When that happen we leak the subflow context
> > and the msk.
> > 
> > Address the issue explicitly detecting such scenario and forcing a full
> > msk shutdown.
> > 
> > mptcp_subflow_drop_ctx() does not remove the subflow from the msk list,
> > racing msk lookup done by the NL PM could fetch the msk just before
> > deletion, and try to free again the subflow.
> > 
> > Just explicitly remove the subflow from said list.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  net/mptcp/protocol.c | 13 +++++++++++--
> >  net/mptcp/subflow.c  |  1 +
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 0e4961e87b48..76c1814c0b19 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2378,9 +2378,18 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> >  	 * to deliver the msk to user-space.
> >  	 * Do nothing at the moment and take action at accept and/or listener
> >  	 * shutdown.
> > +	 * If instead such subflow has been destroyed, e.g. by inet_child_forget
> > +	 * do the kill
> >  	 */
> > -	if (msk->in_accept_queue && msk->first == ssk)
> > -		return;
> > +	if (msk->in_accept_queue && msk->first == ssk) {
> > +		if (!sock_flag(ssk, SOCK_DEAD))
> > +			return;
> > +
> > +		/* ensure later check in mptcp_worker will dispose the msk */
> > +		sock_set_flag(sk, SOCK_DEAD);
> 
> Is it OK to mark the msk as dead and continue below? I guess yes because
> there will not be any data to re-inject anyway?

We don't process any data here (the msk is unaccepted at this point).
Looks safe/sane to me. Overall this is the MPTCP equivalent of
inet_child_forget()

> > +		inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32 -
> > +							  TCP_TIMEWAIT_LEN -1;
> 
> Changing the timestamp looks like an ugly hack :)
> 
> But I guess it is needed not to wait for nothing and there is no clearer
> way? Or move this logic to the worker?

I can add another flag inside the msk. I did not do that here to keep
the patch small, but no objection on my side to clean this path - the
important thing is let syzkaller crunch this ;)

/P

Re: [PATCH mptcp-net 1/2] Squash-to: "mptcp: fix accept vs worker race"

[Matthieu Baerts]

Hi Paolo,

On 06/04/2023 15:56, Paolo Abeni wrote:
> On Thu, 2023-04-06 at 11:19 +0200, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> Thank you for the patch!
>>
>> It looks good to me but I just have some questions if you don't mind,
>> just to be sure I understood the modifications.
>>
>> On 06/04/2023 00:47, Paolo Abeni wrote:
>>> 2 separate fixes needed for the mentioned patch
>>>
>>> On some exceptional scenarios (e.g. listener socket disconnecting while
>>> the subflow is created) a newly created MPC is deleted by the TCP stack
>>> via inet_child_forget(). When that happen we leak the subflow context
>>> and the msk.
>>>
>>> Address the issue explicitly detecting such scenario and forcing a full
>>> msk shutdown.
>>>
>>> mptcp_subflow_drop_ctx() does not remove the subflow from the msk list,
>>> racing msk lookup done by the NL PM could fetch the msk just before
>>> deletion, and try to free again the subflow.
>>>
>>> Just explicitly remove the subflow from said list.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>>  net/mptcp/protocol.c | 13 +++++++++++--
>>>  net/mptcp/subflow.c  |  1 +
>>>  2 files changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 0e4961e87b48..76c1814c0b19 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2378,9 +2378,18 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>>>  	 * to deliver the msk to user-space.
>>>  	 * Do nothing at the moment and take action at accept and/or listener
>>>  	 * shutdown.
>>> +	 * If instead such subflow has been destroyed, e.g. by inet_child_forget
>>> +	 * do the kill
>>>  	 */
>>> -	if (msk->in_accept_queue && msk->first == ssk)
>>> -		return;
>>> +	if (msk->in_accept_queue && msk->first == ssk) {
>>> +		if (!sock_flag(ssk, SOCK_DEAD))
>>> +			return;
>>> +
>>> +		/* ensure later check in mptcp_worker will dispose the msk */
>>> +		sock_set_flag(sk, SOCK_DEAD);
>>
>> Is it OK to mark the msk as dead and continue below? I guess yes because
>> there will not be any data to re-inject anyway?
> 
> We don't process any data here (the msk is unaccepted at this point).
> Looks safe/sane to me. Overall this is the MPTCP equivalent of
> inet_child_forget()

Thank you for the confirmation!

>>> +		inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32 -
>>> +							  TCP_TIMEWAIT_LEN -1;
>>
>> Changing the timestamp looks like an ugly hack :)
>>
>> But I guess it is needed not to wait for nothing and there is no clearer
>> way? Or move this logic to the worker?
> 
> I can add another flag inside the msk. I did not do that here to keep
> the patch small, but no objection on my side to clean this path - the
> important thing is let syzkaller crunch this ;)

It might be cleaner yes. But this can be done later, better not to block
the validation. I'm going to apply the two patches and if needed, I can
modify the commit message/content later.

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

[PATCH mptcp-net 2/2] Squash-to: "mptcp: stops worker on unaccepted sockets at listener close"

[Paolo Abeni]

The mentioned patch carries a bad lockdep annotation. The msk socket
lock is not nested, as that is the outermost one, held only by the
user-space.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/subflow.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 32e54b7fdbbc..c6ae5ddd3bb0 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1882,8 +1882,7 @@ void mptcp_subflow_queue_clean(struct sock *listener_sk, struct sock *listener_s
 		 */
 		mutex_release(&listener_sk->sk_lock.dep_map, _RET_IP_);
 		mptcp_cancel_work(sk);
-		mutex_acquire(&listener_sk->sk_lock.dep_map,
-			      SINGLE_DEPTH_NESTING, 0, _RET_IP_);
+		mutex_acquire(&listener_sk->sk_lock.dep_map, 0, 0, _RET_IP_);
 
 		sock_put(sk);
 	}
-- 
2.39.2

Re: Squash-to: "mptcp: stops worker on unaccepted sockets at listener close": Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6146554927513600
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6146554927513600/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/5583604974092288
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5583604974092288/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4563258183516160
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4563258183516160/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6709504880934912
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6709504880934912/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/db821117c754


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-net 0/2] mptcp: a couple of fixup

[Matthieu Baerts]

Hi Paolo,

On 06/04/2023 00:47, Paolo Abeni wrote:
> Hopefully addressing for good issues/371
> 
> There are actually several different problems that pop-up in random
> order with the repro from such issue.

Thank you for the fixes! They look good to me!

New patches for t/upstream-net and t/upstream:
- 3ac6e434b228: "squashed" patch 1/2 in "mptcp: fix accept vs worker race"
- 8e2c8f661424: "squashed" patch 2/2 in "mptcp: stops worker on
unaccepted sockets at listener close"
- Results: 5b4478bd1afd..e8ce70ce4423 (export-net)
- Results: a73afdaa7080..2d58f6944092 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230406T150700
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230406T150700

Cheers,
Matt

> @Christoph: could you please have a spin in your testbed?

@Christoph: you can now use the export branch ;-)

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