[MPTCP] Re: [PATCH mptcp] squashto: mptcp: protect the rx path with the msk socket spinlock

[MPTCP] Re: [PATCH mptcp] squashto: mptcp: protect the rx path with the msk socket spinlock

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 1035 bytes --]

Paolo Abeni <pabeni(a)redhat.com> wrote:
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 92236a2ccbea..e62d34034d9e 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2707,7 +2707,9 @@ void mptcp_destroy_common(struct mptcp_sock *msk)
> >  
> >  	__mptcp_clear_xmit(sk);
> >  
> > -	skb_queue_splice_tail_init(&sk->sk_receive_queue, &msk->receive_queue);
> > +	/* move to sk_receive_queue, sk_stream_kill_queues will purge it */
> > +	skb_queue_splice_tail_init(&msk->receive_queue, &sk->sk_receive_queue);
> > +
> >  	skb_rbtree_purge(&msk->out_of_order_queue);
> >  	mptcp_token_destroy(msk);
> >  	mptcp_pm_free_anno_list(msk);
> 
> 
> LGTM, thanks for catching this!
> 
> Not sure why self-tests did non complain loudly here ?!?

Probably all test cases drain the rx queue completely before exiting,
we might need something new to cover this.

That being said, packetdrill tests catch it but I suppose those do not
run as frequently as the kselftest infra.

[MPTCP] Re: [PATCH mptcp] squashto: mptcp: protect the rx path with the msk socket spinlock

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Hi Florian, Paolo,

On 20/11/2020 00:47, Florian Westphal wrote:
> Looks like the arguments are inverted.  Intention seems to move
> the msk rx queue to the sk one so it can be purged from the destroy
> function.

Thank you for the patch and the review!

- 86cb91bc831d: "squashed" (with conflicts) in "mptcp: protect the rx 
path with the msk socket spinlock"
- 35d1c3be69d3: "Signed-off-by" + "Co-developed-by"
- 6e9853431195: conflict in t/mptcp-use-mptcp-release_cb-for-delayed-tasks
- Results: 580a33433a07..b86f4581b5c3

Tests + export are in progress!

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

[MPTCP] Re: [PATCH mptcp] squashto: mptcp: protect the rx path with the msk socket spinlock

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 2117 bytes --]

On Fri, 2020-11-20 at 00:47 +0100, Florian Westphal wrote:
> Looks like the arguments are inverted.  Intention seems to move
> the msk rx queue to the sk one so it can be purged from the destroy
> function.

Indeed it was!

> This avoids:
> unreferenced object 0xffff888105cd3800 (size 512):
>   comm "packetdrill", pid 1648, jiffies 4294677515 (age 433.463s)
>   hex dump (first 32 bytes):
>     3c 33 30 3e 4e 6f 76 20 32 30 20 30 30 3a 32 33  <30>Nov 20 00:23
>     3a 34 38 20 73 79 73 74 65 6d 64 5b 31 5d 3a 20  :48 systemd[1]:
>   backtrace:
>     [<00000000b794d231>] __kmalloc_reserve.isra.0+0x2d/0x90
>     [<000000000421e158>] __alloc_skb+0x90/0x260
>     [<00000000d46c1201>] alloc_skb_with_frags+0x5e/0x250
>     [<00000000357464a5>] sock_alloc_send_pskb+0x265/0x2a0
>     [<00000000a73deb72>] tun_get_user+0x4dc/0x1840
>     [<00000000ba538b49>] tun_chr_write_iter+0x51/0x80
>     [<000000004f72fd7e>] do_iter_readv_writev+0x1c6/0x2b0
>     [<00000000c606f908>] do_iter_write+0xb1/0x230
>     [<0000000088092c0e>] vfs_writev+0xcb/0x130
>     [<00000000f9881e25>] do_writev+0x8c/0x150
>     [<00000000dc31e12e>] do_syscall_64+0x2d/0x40
>     [<0000000019b09572>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> plus WARN()s from nonzero sk_forward_alloc.
> ---
>  net/mptcp/protocol.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 92236a2ccbea..e62d34034d9e 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2707,7 +2707,9 @@ void mptcp_destroy_common(struct mptcp_sock *msk)
>  
>  	__mptcp_clear_xmit(sk);
>  
> -	skb_queue_splice_tail_init(&sk->sk_receive_queue, &msk->receive_queue);
> +	/* move to sk_receive_queue, sk_stream_kill_queues will purge it */
> +	skb_queue_splice_tail_init(&msk->receive_queue, &sk->sk_receive_queue);
> +
>  	skb_rbtree_purge(&msk->out_of_order_queue);
>  	mptcp_token_destroy(msk);
>  	mptcp_pm_free_anno_list(msk);


LGTM, thanks for catching this!

Not sure why self-tests did non complain loudly here ?!?

/P