* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-21 3:51 Mat Martineau
0 siblings, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2020-10-21 3:51 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-23 16:03 Matthieu Baerts
0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2020-10-23 16:03 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-21 4:30 Christoph Paasch
0 siblings, 0 replies; 10+ messages in thread
From: Christoph Paasch @ 2020-10-21 4:30 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-17 7:56 Matthieu Baerts
0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2020-10-17 7:56 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-17 7:56 Matthieu Baerts
0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2020-10-17 7:56 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-16 20:24 Christoph Paasch
0 siblings, 0 replies; 10+ messages in thread
From: Christoph Paasch @ 2020-10-16 20:24 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-15 23:54 Mat Martineau
0 siblings, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2020-10-15 23:54 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-15 14:51 Paolo Abeni
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Abeni @ 2020-10-15 14:51 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-06 12:28 Matthieu Baerts
0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2020-10-06 12:28 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close"
@ 2020-10-06 0:21 Mat Martineau
0 siblings, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2020-10-06 0:21 UTC (permalink / raw)
To: mptcp
[-- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-23 16:03 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-21 3:51 [MPTCP] Re: [PATCH net-next] Squash-to: "mptcp: refactor shutdown and close" Mat Martineau
-- strict thread matches above, loose matches on Subject: below --
2020-10-23 16:03 Matthieu Baerts
2020-10-21 4:30 Christoph Paasch
2020-10-17 7:56 Matthieu Baerts
2020-10-17 7:56 Matthieu Baerts
2020-10-16 20:24 Christoph Paasch
2020-10-15 23:54 Mat Martineau
2020-10-15 14:51 Paolo Abeni
2020-10-06 12:28 Matthieu Baerts
2020-10-06 0:21 Mat Martineau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.