[PATCH] mptcp: Fix crash due to tcp_tsorted_anchor was initialized before release skb

[PATCH] mptcp: Fix crash due to tcp_tsorted_anchor was initialized before release skb

[Yonglong Li]

get crash when do pressure test of mptcp:

===========================================================================
dst_release: dst:ffffa06ce6e5c058 refcnt:-1
kernel tried to execute NX-protected page - exploit attempt? (uid: 0)
BUG: unable to handle kernel paging request at ffffa06ce6e5c058
PGD 190a01067 P4D 190a01067 PUD 43fffb067 PMD 22e403063 PTE 8000000226e5c063
Oops: 0011 [#1] SMP PTI
CPU: 7 PID: 7823 Comm: kworker/7:0 Kdump: loaded Tainted: G            E
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.2.1 04/01/2014
Call Trace:
 ? skb_release_head_state+0x68/0x100
 ? skb_release_all+0xe/0x30
 ? kfree_skb+0x32/0xa0
 ? mptcp_sendmsg_frag+0x57e/0x750
 ? __mptcp_retrans+0x21b/0x3c0
 ? __switch_to_asm+0x35/0x70
 ? mptcp_worker+0x25e/0x320
 ? process_one_work+0x1a7/0x360
 ? worker_thread+0x30/0x390
 ? create_worker+0x1a0/0x1a0
 ? kthread+0x112/0x130
 ? kthread_flush_work_fn+0x10/0x10
 ? ret_from_fork+0x35/0x40
===========================================================================

in __mptcp_alloc_tx_skb skb was alloced and skb->tcp_tsorted_anchor will be
initialized,  in under memory pressure situation sk_wmem_schedule will
return false and then kfree_skb. In this case skb->_skb_refdst is not null
because_skb_refdst and tcp_tsorted_anchor are stored in the same mem, and
kfree_skb will try to release dst and casue crash.

Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/protocol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3cb9752..fbb14df 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1199,6 +1199,7 @@ static struct sk_buff *__mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk, g
 		tcp_skb_entail(ssk, skb);
 		return skb;
 	}
+	tcp_skb_tsorted_anchor_cleanup(skb);
 	kfree_skb(skb);
 	return NULL;
 }
-- 
1.8.3.1

Re: mptcp: Fix crash due to tcp_tsorted_anchor was initialized before release skb: Tests Results

[MPTCP CI]

Hi Yonglong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6222883100819456
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6222883100819456/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join - Critical: Global Timeout ❌:
  - Task: https://cirrus-ci.com/task/4815508217266176
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4815508217266176/summary/summary.txt

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

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: Fix crash due to tcp_tsorted_anchor was initialized before release skb

[Paolo Abeni]

On Wed, 2022-03-09 at 18:20 +0800, Yonglong Li wrote:
> get crash when do pressure test of mptcp:

Ouch!

> ===========================================================================
> dst_release: dst:ffffa06ce6e5c058 refcnt:-1
> kernel tried to execute NX-protected page - exploit attempt? (uid: 0)
> BUG: unable to handle kernel paging request at ffffa06ce6e5c058
> PGD 190a01067 P4D 190a01067 PUD 43fffb067 PMD 22e403063 PTE 8000000226e5c063
> Oops: 0011 [#1] SMP PTI
> CPU: 7 PID: 7823 Comm: kworker/7:0 Kdump: loaded Tainted: G            E
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.2.1 04/01/2014
> Call Trace:
>  ? skb_release_head_state+0x68/0x100
>  ? skb_release_all+0xe/0x30
>  ? kfree_skb+0x32/0xa0
>  ? mptcp_sendmsg_frag+0x57e/0x750
>  ? __mptcp_retrans+0x21b/0x3c0
>  ? __switch_to_asm+0x35/0x70
>  ? mptcp_worker+0x25e/0x320
>  ? process_one_work+0x1a7/0x360
>  ? worker_thread+0x30/0x390
>  ? create_worker+0x1a0/0x1a0
>  ? kthread+0x112/0x130
>  ? kthread_flush_work_fn+0x10/0x10
>  ? ret_from_fork+0x35/0x40
> ===========================================================================
> 
> in __mptcp_alloc_tx_skb skb was alloced and skb->tcp_tsorted_anchor will be
> initialized,  in under memory pressure situation sk_wmem_schedule will
> return false and then kfree_skb. In this case skb->_skb_refdst is not null
> because_skb_refdst and tcp_tsorted_anchor are stored in the same mem, and
> kfree_skb will try to release dst and casue crash.

Fixes: f70cad1085d1 ("mptcp: stop relying on tcp_tx_skb_cache"


> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
>  net/mptcp/protocol.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3cb9752..fbb14df 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1199,6 +1199,7 @@ static struct sk_buff *__mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk, g
>  		tcp_skb_entail(ssk, skb);
>  		return skb;
>  	}
> +	tcp_skb_tsorted_anchor_cleanup(skb);
>  	kfree_skb(skb);
>  	return NULL;
>  }

LGTM!

Reviewed-by: Paolo Abeni <pabeni@redhat.com>

Re: [PATCH] mptcp: Fix crash due to tcp_tsorted_anchor was initialized before release skb

[Mat Martineau]

On Wed, 9 Mar 2022, Paolo Abeni wrote:

> On Wed, 2022-03-09 at 18:20 +0800, Yonglong Li wrote:
>> get crash when do pressure test of mptcp:
>
> Ouch!
>
>> ===========================================================================
>> dst_release: dst:ffffa06ce6e5c058 refcnt:-1
>> kernel tried to execute NX-protected page - exploit attempt? (uid: 0)
>> BUG: unable to handle kernel paging request at ffffa06ce6e5c058
>> PGD 190a01067 P4D 190a01067 PUD 43fffb067 PMD 22e403063 PTE 8000000226e5c063
>> Oops: 0011 [#1] SMP PTI
>> CPU: 7 PID: 7823 Comm: kworker/7:0 Kdump: loaded Tainted: G            E
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.2.1 04/01/2014
>> Call Trace:
>>  ? skb_release_head_state+0x68/0x100
>>  ? skb_release_all+0xe/0x30
>>  ? kfree_skb+0x32/0xa0
>>  ? mptcp_sendmsg_frag+0x57e/0x750
>>  ? __mptcp_retrans+0x21b/0x3c0
>>  ? __switch_to_asm+0x35/0x70
>>  ? mptcp_worker+0x25e/0x320
>>  ? process_one_work+0x1a7/0x360
>>  ? worker_thread+0x30/0x390
>>  ? create_worker+0x1a0/0x1a0
>>  ? kthread+0x112/0x130
>>  ? kthread_flush_work_fn+0x10/0x10
>>  ? ret_from_fork+0x35/0x40
>> ===========================================================================
>>
>> in __mptcp_alloc_tx_skb skb was alloced and skb->tcp_tsorted_anchor will be
>> initialized,  in under memory pressure situation sk_wmem_schedule will
>> return false and then kfree_skb. In this case skb->_skb_refdst is not null
>> because_skb_refdst and tcp_tsorted_anchor are stored in the same mem, and
>> kfree_skb will try to release dst and casue crash.
>
> Fixes: f70cad1085d1 ("mptcp: stop relying on tcp_tx_skb_cache"
>
>
>> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
>> ---
>>  net/mptcp/protocol.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 3cb9752..fbb14df 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -1199,6 +1199,7 @@ static struct sk_buff *__mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk, g
>>  		tcp_skb_entail(ssk, skb);
>>  		return skb;
>>  	}
>> +	tcp_skb_tsorted_anchor_cleanup(skb);
>>  	kfree_skb(skb);
>>  	return NULL;
>>  }
>
> LGTM!
>
> Reviewed-by: Paolo Abeni <pabeni@redhat.com>

I agree, looks good for mptcp-net (with the recommended Fixes tag)

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

--
Mat Martineau
Intel

Re: [PATCH] mptcp: Fix crash due to tcp_tsorted_anchor was initialized before release skb

[Matthieu Baerts]

Hi Yonglong, Paolo, Mat,

On 09/03/2022 11:20, Yonglong Li wrote:
> get crash when do pressure test of mptcp:
> 

(...)

> 
> in __mptcp_alloc_tx_skb skb was alloced and skb->tcp_tsorted_anchor 
> will be initialized,  in under memory pressure situation 
> sk_wmem_schedule will return false and then kfree_skb. In this case 
> skb->_skb_refdst is not null because_skb_refdst and 
> tcp_tsorted_anchor are stored in the same mem, and kfree_skb will
> try to release dst and casue crash.

Thank you for the patch and the reviews!

(sorry for the delay due to the reworking of the git repo)

Now in our tree with Paolo and Mat's RvB tags (and without typos
detected by 'checkpatch.pl --codespell')

- 9f5c50f2e2b5: mptcp: Fix crash due to tcp_tsorted_anchor was
initialized before release skb
- Results (net-next): 1d99000bd70..b0d3123bdf0
- Results (net): f8a0df6cce2..153502a63ba

Builds and tests are now in progress:

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

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