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

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

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

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