mptcp: always provide MPTCP skb ext for tx skb

[PATCH mptcp-net] mptcp: always provide MPTCP skb ext for tx skb

Posted by Paolo Abeni 4 years ago

The mptcp code will uses whatever skb is present into
ssk->sk_tx_skb_cache. If the skb tx cache is enabled, the
recycle operation will clean the MPTCP extension from the
cached skb, and the MPTCP would hit a later WARN.

Fix the issue always invoking the mptcp skb allocation
helper, which will safely check the cached skb for the extension
presence.

Fixes: 1094c6fe7280 ("mptcp: fix possible divide by zero")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 2602f1386160..f0673541a764 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1325,7 +1325,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	}
 
 alloc_skb:
-	if (!must_collapse && !ssk->sk_tx_skb_cache &&
+	if (!must_collapse &&
 	    !mptcp_alloc_tx_skb(sk, ssk, info->data_lock_held))
 		return 0;
 
-- 
2.26.3

Re: [PATCH mptcp-net] mptcp: always provide MPTCP skb ext for tx skb

Posted by Mat Martineau 4 years ago

On Thu, 16 Sep 2021, Paolo Abeni wrote:

> The mptcp code will uses whatever skb is present into
> ssk->sk_tx_skb_cache. If the skb tx cache is enabled, the
> recycle operation will clean the MPTCP extension from the
> cached skb, and the MPTCP would hit a later WARN.
>
> Fix the issue always invoking the mptcp skb allocation
> helper, which will safely check the cached skb for the extension
> presence.
>
> Fixes: 1094c6fe7280 ("mptcp: fix possible divide by zero")

Should add this tag too:

Reported-by: syzbot+263a248eec3e875baa7b@syzkaller.appspotmail.com

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2602f1386160..f0673541a764 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1325,7 +1325,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
> 	}
>
> alloc_skb:
> -	if (!must_collapse && !ssk->sk_tx_skb_cache &&
> +	if (!must_collapse &&
> 	    !mptcp_alloc_tx_skb(sk, ssk, info->data_lock_held))
> 		return 0;

must_collapse is only true if the tail skb extension has already been 
checked, so skipping the mptcp_alloc_tx_skb() will be ok in that case.

Looks good:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net] mptcp: always provide MPTCP skb ext for tx skb

Posted by Matthieu Baerts 3 years, 12 months ago

Hi Paolo, Mat,

On 16/09/2021 18:06, Paolo Abeni wrote:
> The mptcp code will uses whatever skb is present into
> ssk->sk_tx_skb_cache. If the skb tx cache is enabled, the
> recycle operation will clean the MPTCP extension from the
> cached skb, and the MPTCP would hit a later WARN.
> 
> Fix the issue always invoking the mptcp skb allocation
> helper, which will safely check the cached skb for the extension
> presence.

Thank you for the patch and the review!

This patch is now in our tree (fixes for net) with the Reported-by tag
and Mat's RvB tag.

Please note that as expected, I had a conflict with "mptcp: stop relying
on tcp_tx_skb_cache". As a result, 'export' is seen as unmodified
because the modification here is no longer needed thanks to "mptcp: stop
relying on tcp_tx_skb_cache".

- cc90251fe40f: mptcp: always provide MPTCP skb ext for tx skb
- 1027c0b1c29c: conflict in t/mptcp-stop-relying-on-tcp_tx_skb_cache
- Results: 1c6ca6c7576e..82229863a0d7 (diff is empty as expected)

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210920T154149
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210920T154149

(but the tests are validating the same code as before as "export" still
has the same code at the end)

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

Re: [PATCH mptcp-net] mptcp: always provide MPTCP skb ext for tx skb

Posted by Mat Martineau 3 years, 12 months ago

On Mon, 20 Sep 2021, Matthieu Baerts wrote:

> Hi Paolo, Mat,
>
> On 16/09/2021 18:06, Paolo Abeni wrote:
>> The mptcp code will uses whatever skb is present into
>> ssk->sk_tx_skb_cache. If the skb tx cache is enabled, the
>> recycle operation will clean the MPTCP extension from the
>> cached skb, and the MPTCP would hit a later WARN.
>>
>> Fix the issue always invoking the mptcp skb allocation
>> helper, which will safely check the cached skb for the extension
>> presence.
>
> Thank you for the patch and the review!
>
> This patch is now in our tree (fixes for net) with the Reported-by tag
> and Mat's RvB tag.
>
> Please note that as expected, I had a conflict with "mptcp: stop relying
> on tcp_tx_skb_cache". As a result, 'export' is seen as unmodified
> because the modification here is no longer needed thanks to "mptcp: stop
> relying on tcp_tx_skb_cache".
>
> - cc90251fe40f: mptcp: always provide MPTCP skb ext for tx skb
> - 1027c0b1c29c: conflict in t/mptcp-stop-relying-on-tcp_tx_skb_cache
> - Results: 1c6ca6c7576e..82229863a0d7 (diff is empty as expected)
>
> Builds and tests are now in progress:
>
> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210920T154149
> https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210920T154149
>
> (but the tests are validating the same code as before as "export" still
> has the same code at the end)
>

I'll give the syzbot reproducer a try with this patch today.

--
Mat Martineau
Intel

Re: [PATCH mptcp-net] mptcp: always provide MPTCP skb ext for tx skb

Posted by Mat Martineau 3 years, 12 months ago

On Mon, 20 Sep 2021, Mat Martineau wrote:

> On Mon, 20 Sep 2021, Matthieu Baerts wrote:
>
>> Hi Paolo, Mat,
>> 
>> On 16/09/2021 18:06, Paolo Abeni wrote:
>>> The mptcp code will uses whatever skb is present into
>>> ssk->sk_tx_skb_cache. If the skb tx cache is enabled, the
>>> recycle operation will clean the MPTCP extension from the
>>> cached skb, and the MPTCP would hit a later WARN.
>>> 
>>> Fix the issue always invoking the mptcp skb allocation
>>> helper, which will safely check the cached skb for the extension
>>> presence.
>> 
>> Thank you for the patch and the review!
>> 
>> This patch is now in our tree (fixes for net) with the Reported-by tag
>> and Mat's RvB tag.
>> 
>> Please note that as expected, I had a conflict with "mptcp: stop relying
>> on tcp_tx_skb_cache". As a result, 'export' is seen as unmodified
>> because the modification here is no longer needed thanks to "mptcp: stop
>> relying on tcp_tx_skb_cache".
>> 
>> - cc90251fe40f: mptcp: always provide MPTCP skb ext for tx skb
>> - 1027c0b1c29c: conflict in t/mptcp-stop-relying-on-tcp_tx_skb_cache
>> - Results: 1c6ca6c7576e..82229863a0d7 (diff is empty as expected)
>> 
>> Builds and tests are now in progress:
>> 
>> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210920T154149
>> https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210920T154149
>> 
>> (but the tests are validating the same code as before as "export" still
>> has the same code at the end)
>> 
>
> I'll give the syzbot reproducer a try with this patch today.
>

Paolo's proposed fix didn't resolve the issue for syzbot. Unfortunately 
I didn't get good information from my local syzkaller setup today trying 
to reproduce the issue (without the fix) on net-next. Will have to try a 
different approach to reproducing it tomorrow.

--
Mat Martineau
Intel