[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Matthieu Baerts]

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

Hi Paolo, Mat,

On 20/10/2020 18:25, Paolo Abeni wrote:
> we must release the ssk socket only after orphaning it,
> or we may hit UaF in the receive path.
> 
> Beyond the error critical code path reported by syzkaller,
> there is also the scenario of a subflow closed by the PM.

Thank you for this patch and the review!

- 51c3efe449ec: "squashed" in "mptcp: refactor shutdown and close"
- Results: da50c705808b..62d3c2d391fa

Tests + export are going to be started soon!

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

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Christoph Paasch]

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

On 10/20/20 - 20:51, Mat Martineau wrote:
> On Tue, 20 Oct 2020, Paolo Abeni wrote:
> 
> > we must release the ssk socket only after orphaning it,
> > or we may hit UaF in the receive path.
> > 
> > Beyond the error critical code path reported by syzkaller,
> > there is also the scenario of a subflow closed by the PM.
> > 
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > ---
> > Should fix 101 by I actually tested this only vs self-tests
> 
> Looks fine in terms of code review. I will work on verifying it with
> syzkaller tomorrow unless I hear something from Christoph.

I'm having some problems with my syzkaller, so I can't test right now.


Christoph

> 
> 
> Mat
> 
> 
> > ---
> > net/mptcp/protocol.c | 34 ++++++++++++++++++++++++----------
> > 1 file changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index a32c27fe525c..2240fd370014 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1746,16 +1746,22 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk)
> > void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> > 		       struct mptcp_subflow_context *subflow)
> > {
> > -	struct socket *sock = READ_ONCE(ssk->sk_socket);
> > +	bool dispose_socket = false;
> > +	struct socket *sock;
> > 
> > 	list_del(&subflow->node);
> > 
> > -	/* outgoing subflow */
> > -	if (sock && sock != sk->sk_socket)
> > -		iput(SOCK_INODE(sock));
> > -
> > 	lock_sock(ssk);
> > 
> > +	/* if we are invoked by the msk cleanup code, the subflow is
> > +	 * already orphaned
> > +	 */
> > +	sock = ssk->sk_socket;
> > +	if (sock) {
> > +		dispose_socket = sock != sk->sk_socket;
> > +		sock_orphan(ssk);
> > +	}
> > +
> > 	/* if ssk hit tcp_done(), tcp_cleanup_ulp() cleared the related ops
> > 	 * the ssk has been already destroyed, we just need to release the
> > 	 * reference owned by msk;
> > @@ -1771,6 +1777,9 @@ void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> > 		__sock_put(ssk);
> > 	}
> > 	release_sock(ssk);
> > +	if (dispose_socket)
> > +		iput(SOCK_INODE(sock));
> > +
> > 	sock_put(ssk);
> > }
> > 
> > @@ -2165,15 +2174,20 @@ static void mptcp_close(struct sock *sk, long timeout)
> > 	inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32;
> > 	list_for_each_entry(subflow, &mptcp_sk(sk)->conn_list, node) {
> > 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > -		struct socket *sock = READ_ONCE(ssk->sk_socket);
> > -		bool slow;
> > -
> > -		if (sock && sock != sk->sk_socket)
> > -			iput(SOCK_INODE(sock));
> > +		bool slow, dispose_socket;
> > +		struct socket *sock;
> > 
> > 		slow = lock_sock_fast(ssk);
> > +		sock = ssk->sk_socket;
> > +		dispose_socket = sock && sock != sk->sk_socket;
> > 		sock_orphan(ssk);
> > 		unlock_sock_fast(ssk, slow);
> > +
> > +		/* for the outgoing subflows we additionally need to free
> > +		 * the associated socket
> > +		 */
> > +		if (dispose_socket)
> > +			iput(SOCK_INODE(sock));
> > 	}
> > 	sock_orphan(sk);
> > 
> > -- 
> > 2.26.2
> 
> --
> Mat Martineau
> Intel

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Mat Martineau]

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

On Tue, 20 Oct 2020, Paolo Abeni wrote:

> we must release the ssk socket only after orphaning it,
> or we may hit UaF in the receive path.
>
> Beyond the error critical code path reported by syzkaller,
> there is also the scenario of a subflow closed by the PM.
>
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> Should fix 101 by I actually tested this only vs self-tests

Looks fine in terms of code review. I will work on verifying it with 
syzkaller tomorrow unless I hear something from Christoph.


Mat


> ---
> net/mptcp/protocol.c | 34 ++++++++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index a32c27fe525c..2240fd370014 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1746,16 +1746,22 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk)
> void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> 		       struct mptcp_subflow_context *subflow)
> {
> -	struct socket *sock = READ_ONCE(ssk->sk_socket);
> +	bool dispose_socket = false;
> +	struct socket *sock;
>
> 	list_del(&subflow->node);
>
> -	/* outgoing subflow */
> -	if (sock && sock != sk->sk_socket)
> -		iput(SOCK_INODE(sock));
> -
> 	lock_sock(ssk);
>
> +	/* if we are invoked by the msk cleanup code, the subflow is
> +	 * already orphaned
> +	 */
> +	sock = ssk->sk_socket;
> +	if (sock) {
> +		dispose_socket = sock != sk->sk_socket;
> +		sock_orphan(ssk);
> +	}
> +
> 	/* if ssk hit tcp_done(), tcp_cleanup_ulp() cleared the related ops
> 	 * the ssk has been already destroyed, we just need to release the
> 	 * reference owned by msk;
> @@ -1771,6 +1777,9 @@ void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> 		__sock_put(ssk);
> 	}
> 	release_sock(ssk);
> +	if (dispose_socket)
> +		iput(SOCK_INODE(sock));
> +
> 	sock_put(ssk);
> }
>
> @@ -2165,15 +2174,20 @@ static void mptcp_close(struct sock *sk, long timeout)
> 	inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32;
> 	list_for_each_entry(subflow, &mptcp_sk(sk)->conn_list, node) {
> 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> -		struct socket *sock = READ_ONCE(ssk->sk_socket);
> -		bool slow;
> -
> -		if (sock && sock != sk->sk_socket)
> -			iput(SOCK_INODE(sock));
> +		bool slow, dispose_socket;
> +		struct socket *sock;
>
> 		slow = lock_sock_fast(ssk);
> +		sock = ssk->sk_socket;
> +		dispose_socket = sock && sock != sk->sk_socket;
> 		sock_orphan(ssk);
> 		unlock_sock_fast(ssk, slow);
> +
> +		/* for the outgoing subflows we additionally need to free
> +		 * the associated socket
> +		 */
> +		if (dispose_socket)
> +			iput(SOCK_INODE(sock));
> 	}
> 	sock_orphan(sk);
>
> -- 
> 2.26.2

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Matthieu Baerts]

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

Hi Paolo, Mat, Christoph,

On 15/10/2020 23:28, Paolo Abeni wrote:
> After the refactor __mptcp_close_ssk() can be invoked on
> orphaned TCP socket, so we can't wait a timeout, or
> sk_stream_wait_close() will oops due to NULL waitqueue.
> 
> We can simply always use a 0 timeout (and clean-up the
> related signature accordingly): the timeout, if any,
> is used before by mptcp_close().

Thank you for the patch, review and validation!

- e3514ae223bd: "squashed" (with conflicts) in "mptcp: refactor shutdown 
and close"
- a563fd15f543: conflict in 
t/mptcp-move-page-frag-allocation-in-mptcp_sendmsg
- Results: b160e00bdf28..a26726f4d29b

Tests + export are going to be started soon!

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

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Matthieu Baerts]

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

Hi Davide, Paolo,

On 15/10/2020 16:51, Paolo Abeni wrote:
> On Thu, 2020-10-15 at 16:30 +0200, Davide Caratti wrote:
>> when the data-fin is acked on all subflows, the socket goes in
>> FIN_WAIT_2 state. Call __mptcp_destroy_sock() to transmit a TCP FIN.
> 
> Note for the reviewers: the above happens only for orphaned msk, and
> follows quite closely what plain TCP does.

Thank you for having destroyed orphans!

>>
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/98
>> Signed-off-by: Davide Caratti <dcaratti(a)redhat.com>
>> ---
>>   net/mptcp/protocol.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 5b32741f2af3..95566f120e72 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -1859,7 +1859,8 @@ static void mptcp_worker(struct work_struct *work)
>>   	 */
>>   	if (sock_flag(sk, SOCK_DEAD) &&
>>   	    (mptcp_check_close_timeout(sk) ||
>> -	    (state != sk->sk_state && sk->sk_state == TCP_CLOSE))) {
>> +	    (state != sk->sk_state &&
>> +	    ((1 << inet_sk_state_load(sk)) & (TCPF_CLOSE | TCPF_FIN_WAIT2))))) {
>>   		inet_sk_state_store(sk, TCP_CLOSE);
>>   		__mptcp_destroy_sock(sk, 0);
>>   		goto unlock;
> 
> LGTM! Thanks Davide!

Thank you for the patch and the review!

- 323d37ef423c: "squashed" in "mptcp: refactor shutdown and close"
- Results: e41dc6548e3b..b160e00bdf28

Tests + export are going to be started soon!

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

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Christoph Paasch]

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

On 10/15/20 - 23:28, Paolo Abeni wrote:
> After the refactor __mptcp_close_ssk() can be invoked on
> orphaned TCP socket, so we can't wait a timeout, or
> sk_stream_wait_close() will oops due to NULL waitqueue.
> 
> We can simply always use a 0 timeout (and clean-up the
> related signature accordingly): the timeout, if any,
> is used before by mptcp_close().
> 
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> This should fix issue 100, but I only build test it ;)

Yes, it does! :-)


Christoph

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Mat Martineau]

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


On Thu, 15 Oct 2020, Paolo Abeni wrote:

> After the refactor __mptcp_close_ssk() can be invoked on
> orphaned TCP socket, so we can't wait a timeout, or
> sk_stream_wait_close() will oops due to NULL waitqueue.
>
> We can simply always use a 0 timeout (and clean-up the
> related signature accordingly): the timeout, if any,
> is used before by mptcp_close().
>
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> This should fix issue 100, but I only build test it ;)
> ---
> net/mptcp/pm_netlink.c |  6 ++----
> net/mptcp/protocol.c   | 19 +++++++++----------
> net/mptcp/protocol.h   |  3 +--
> 3 files changed, 12 insertions(+), 16 deletions(-)

Looks good to me from a code review perspective. I haven't confirmed that 
it fixes #100.


--
Mat Martineau
Intel

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Paolo Abeni]

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

On Thu, 2020-10-15 at 16:30 +0200, Davide Caratti wrote:
> when the data-fin is acked on all subflows, the socket goes in
> FIN_WAIT_2 state. Call __mptcp_destroy_sock() to transmit a TCP FIN.

Note for the reviewers: the above happens only for orphaned msk, and
follows quite closely what plain TCP does.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/98
> Signed-off-by: Davide Caratti <dcaratti(a)redhat.com>
> ---
>  net/mptcp/protocol.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 5b32741f2af3..95566f120e72 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1859,7 +1859,8 @@ static void mptcp_worker(struct work_struct *work)
>  	 */
>  	if (sock_flag(sk, SOCK_DEAD) &&
>  	    (mptcp_check_close_timeout(sk) ||
> -	    (state != sk->sk_state && sk->sk_state == TCP_CLOSE))) {
> +	    (state != sk->sk_state &&
> +	    ((1 << inet_sk_state_load(sk)) & (TCPF_CLOSE | TCPF_FIN_WAIT2))))) {
>  		inet_sk_state_store(sk, TCP_CLOSE);
>  		__mptcp_destroy_sock(sk, 0);
>  		goto unlock;

LGTM! Thanks Davide!

/P

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Matthieu Baerts]

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

Hi Paolo, Mat,

On 06/10/2020 02:21, Mat Martineau wrote:
> On Mon, 5 Oct 2020, Paolo Abeni wrote:
> 
>> With the additional SM handling, fallback socket will
>> go through an unneeded timeout, because they never
>> get the DATA_FIN from the peer.
>>
>> In case of fallback, we can just close the msk and
>> invoke directly tcp_close on the subflow.
>>
>> This fixes diag.sh self-tests failure in current export
>> branch
> 
> Looks good to me. Thanks Paolo.

Thank you for the patch and the review!

- a98dba029718: "squashed" in "mptcp: refactor shutdown and close"
- Results: 10c53117392d..b74dfa0c4018

Tests + export are in progress!

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

[MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"

[Mat Martineau]

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

On Mon, 5 Oct 2020, Paolo Abeni wrote:

> With the additional SM handling, fallback socket will
> go through an unneeded timeout, because they never
> get the DATA_FIN from the peer.
>
> In case of fallback, we can just close the msk and
> invoke directly tcp_close on the subflow.
>
> This fixes diag.sh self-tests failure in current export
> branch
>
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 29be694c1d94..7d5905968fac 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2102,7 +2102,8 @@ static void mptcp_close(struct sock *sk, long timeout)
> 	lock_sock(sk);
> 	sk->sk_shutdown = SHUTDOWN_MASK;
>
> -	if (sk->sk_state == TCP_LISTEN) {
> +	if (sk->sk_state == TCP_LISTEN ||
> +	    __mptcp_check_fallback((struct mptcp_sock *)sk)) {
> 		inet_sk_state_store(sk, TCP_CLOSE);
> 		goto cleanup;
> 	} else if (sk->sk_state == TCP_CLOSE) {
> -- 
> 2.26.2

Looks good to me. Thanks Paolo.

--
Mat Martineau
Intel