* [PATCH net 1/3] mptcp: handle mptcp listener destruction via rcu
2020-04-20 14:25 [PATCH net 0/3] mptcp: fix races on accept() Paolo Abeni
@ 2020-04-20 14:25 ` Paolo Abeni
2020-04-20 14:25 ` [PATCH net 2/3] mptcp: avoid flipping mp_capable field in syn_recv_sock() Paolo Abeni
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Paolo Abeni @ 2020-04-20 14:25 UTC (permalink / raw)
To: netdev
Cc: Mat Martineau, Matthieu Baerts, Jakub Kicinski, Christoph Paasch,
Florian Westphal
From: Florian Westphal <fw@strlen.de>
Following splat can occur during self test:
BUG: KASAN: use-after-free in subflow_data_ready+0x156/0x160
Read of size 8 at addr ffff888100c35c28 by task mptcp_connect/4808
subflow_data_ready+0x156/0x160
tcp_child_process+0x6a3/0xb30
tcp_v4_rcv+0x2231/0x3730
ip_protocol_deliver_rcu+0x5c/0x860
ip_local_deliver_finish+0x220/0x360
ip_local_deliver+0x1c8/0x4e0
ip_rcv_finish+0x1da/0x2f0
ip_rcv+0xd0/0x3c0
__netif_receive_skb_one_core+0xf5/0x160
__netif_receive_skb+0x27/0x1c0
process_backlog+0x21e/0x780
net_rx_action+0x35f/0xe90
do_softirq+0x4c/0x50
[..]
This occurs when accessing subflow_ctx->conn.
Problem is that tcp_child_process() calls listen sockets'
sk_data_ready() notification, but it doesn't hold the listener
lock. Another cpu calling close() on the listener will then cause
transition of refcount to 0.
Fixes: 58b09919626bf ("mptcp: create msk early")
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b2d2193453bc..d275c1e827fe 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1391,6 +1391,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
msk->ack_seq = ack_seq;
}
+ sock_reset_flag(nsk, SOCK_RCU_FREE);
/* will be fully established after successful MPC subflow creation */
inet_sk_state_store(nsk, TCP_SYN_RECV);
bh_unlock_sock(nsk);
@@ -1792,6 +1793,8 @@ static int mptcp_listen(struct socket *sock, int backlog)
goto unlock;
}
+ sock_set_flag(sock->sk, SOCK_RCU_FREE);
+
err = ssock->ops->listen(ssock, backlog);
inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
if (!err)
--
2.21.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH net 2/3] mptcp: avoid flipping mp_capable field in syn_recv_sock()
2020-04-20 14:25 [PATCH net 0/3] mptcp: fix races on accept() Paolo Abeni
2020-04-20 14:25 ` [PATCH net 1/3] mptcp: handle mptcp listener destruction via rcu Paolo Abeni
@ 2020-04-20 14:25 ` Paolo Abeni
2020-04-20 14:25 ` [PATCH net 3/3] mptcp: drop req socket remote_key* fields Paolo Abeni
2020-04-20 20:02 ` [PATCH net 0/3] mptcp: fix races on accept() David Miller
3 siblings, 0 replies; 8+ messages in thread
From: Paolo Abeni @ 2020-04-20 14:25 UTC (permalink / raw)
To: netdev
Cc: Mat Martineau, Matthieu Baerts, Jakub Kicinski, Christoph Paasch,
Florian Westphal
If multiple CPUs races on the same req_sock in syn_recv_sock(),
flipping such field can cause inconsistent child socket status.
When racing, the CPU losing the req ownership may still change
the mptcp request socket mp_capable flag while the CPU owning
the request is cloning the socket, leaving the child socket with
'is_mptcp' set but no 'mp_capable' flag.
Such socket will stay with 'conn' field cleared, heading to oops
in later mptcp callback.
Address the issue tracking the fallback status in a local variable.
Fixes: 58b09919626b ("mptcp: create msk early")
Co-developed-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/subflow.c | 46 +++++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 3a94f859347a..10090ca3d3e0 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -376,6 +376,17 @@ static void mptcp_force_close(struct sock *sk)
sk_common_release(sk);
}
+static void subflow_ulp_fallback(struct sock *sk,
+ struct mptcp_subflow_context *old_ctx)
+{
+ struct inet_connection_sock *icsk = inet_csk(sk);
+
+ mptcp_subflow_tcp_fallback(sk, old_ctx);
+ icsk->icsk_ulp_ops = NULL;
+ rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
+ tcp_sk(sk)->is_mptcp = 0;
+}
+
static struct sock *subflow_syn_recv_sock(const struct sock *sk,
struct sk_buff *skb,
struct request_sock *req,
@@ -388,6 +399,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
struct tcp_options_received opt_rx;
bool fallback_is_fatal = false;
struct sock *new_msk = NULL;
+ bool fallback = false;
struct sock *child;
pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
@@ -412,14 +424,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
subflow_req->remote_key = opt_rx.mptcp.sndr_key;
subflow_req->remote_key_valid = 1;
} else {
- subflow_req->mp_capable = 0;
+ fallback = true;
goto create_child;
}
create_msk:
new_msk = mptcp_sk_clone(listener->conn, req);
if (!new_msk)
- subflow_req->mp_capable = 0;
+ fallback = true;
} else if (subflow_req->mp_join) {
fallback_is_fatal = true;
opt_rx.mptcp.mp_join = 0;
@@ -438,12 +450,18 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
if (child && *own_req) {
struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(child);
- /* we have null ctx on TCP fallback, which is fatal on
- * MPJ handshake
+ /* we need to fallback on ctx allocation failure and on pre-reqs
+ * checking above. In the latter scenario we additionally need
+ * to reset the context to non MPTCP status.
*/
- if (!ctx) {
+ if (!ctx || fallback) {
if (fallback_is_fatal)
goto close_child;
+
+ if (ctx) {
+ subflow_ulp_fallback(child, ctx);
+ kfree_rcu(ctx, rcu);
+ }
goto out;
}
@@ -474,6 +492,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
/* dispose of the left over mptcp master, if any */
if (unlikely(new_msk))
mptcp_force_close(new_msk);
+
+ /* check for expected invariant - should never trigger, just help
+ * catching eariler subtle bugs
+ */
+ WARN_ON_ONCE(*own_req && child && tcp_sk(child)->is_mptcp &&
+ (!mptcp_subflow_ctx(child) ||
+ !mptcp_subflow_ctx(child)->conn));
return child;
close_child:
@@ -1094,17 +1119,6 @@ static void subflow_ulp_release(struct sock *sk)
kfree_rcu(ctx, rcu);
}
-static void subflow_ulp_fallback(struct sock *sk,
- struct mptcp_subflow_context *old_ctx)
-{
- struct inet_connection_sock *icsk = inet_csk(sk);
-
- mptcp_subflow_tcp_fallback(sk, old_ctx);
- icsk->icsk_ulp_ops = NULL;
- rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
- tcp_sk(sk)->is_mptcp = 0;
-}
-
static void subflow_ulp_clone(const struct request_sock *req,
struct sock *newsk,
const gfp_t priority)
--
2.21.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH net 3/3] mptcp: drop req socket remote_key* fields
2020-04-20 14:25 [PATCH net 0/3] mptcp: fix races on accept() Paolo Abeni
2020-04-20 14:25 ` [PATCH net 1/3] mptcp: handle mptcp listener destruction via rcu Paolo Abeni
2020-04-20 14:25 ` [PATCH net 2/3] mptcp: avoid flipping mp_capable field in syn_recv_sock() Paolo Abeni
@ 2020-04-20 14:25 ` Paolo Abeni
2020-04-20 20:05 ` Mat Martineau
2020-04-20 20:02 ` [PATCH net 0/3] mptcp: fix races on accept() David Miller
3 siblings, 1 reply; 8+ messages in thread
From: Paolo Abeni @ 2020-04-20 14:25 UTC (permalink / raw)
To: netdev
Cc: Mat Martineau, Matthieu Baerts, Jakub Kicinski, Christoph Paasch,
Florian Westphal
We don't need them, as we can use the current ingress opt
data instead. Setting them in syn_recv_sock() may causes
inconsistent mptcp socket status, as per previous commit.
Fixes: cc7972ea1932 ("mptcp: parse and emit MP_CAPABLE option according to v1 spec")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 8 +++++---
net/mptcp/protocol.h | 8 ++++----
net/mptcp/subflow.c | 20 ++++++++++----------
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d275c1e827fe..58ad03fc1bbc 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1345,7 +1345,9 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk)
}
#endif
-struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
+struct sock *mptcp_sk_clone(const struct sock *sk,
+ const struct tcp_options_received *opt_rx,
+ struct request_sock *req)
{
struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);
@@ -1383,9 +1385,9 @@ struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
msk->write_seq = subflow_req->idsn + 1;
atomic64_set(&msk->snd_una, msk->write_seq);
- if (subflow_req->remote_key_valid) {
+ if (opt_rx->mptcp.mp_capable) {
msk->can_ack = true;
- msk->remote_key = subflow_req->remote_key;
+ msk->remote_key = opt_rx->mptcp.sndr_key;
mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
ack_seq++;
msk->ack_seq = ack_seq;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 67448002a2d7..a2b3048037d0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -206,12 +206,10 @@ struct mptcp_subflow_request_sock {
struct tcp_request_sock sk;
u16 mp_capable : 1,
mp_join : 1,
- backup : 1,
- remote_key_valid : 1;
+ backup : 1;
u8 local_id;
u8 remote_id;
u64 local_key;
- u64 remote_key;
u64 idsn;
u32 token;
u32 ssn_offset;
@@ -332,7 +330,9 @@ void mptcp_proto_init(void);
int mptcp_proto_v6_init(void);
#endif
-struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req);
+struct sock *mptcp_sk_clone(const struct sock *sk,
+ const struct tcp_options_received *opt_rx,
+ struct request_sock *req);
void mptcp_get_options(const struct sk_buff *skb,
struct tcp_options_received *opt_rx);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 10090ca3d3e0..87c094702d63 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -133,7 +133,6 @@ static void subflow_init_req(struct request_sock *req,
subflow_req->mp_capable = 0;
subflow_req->mp_join = 0;
- subflow_req->remote_key_valid = 0;
#ifdef CONFIG_TCP_MD5SIG
/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
@@ -404,6 +403,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
+ opt_rx.mptcp.mp_capable = 0;
if (tcp_rsk(req)->is_mptcp == 0)
goto create_child;
@@ -418,18 +418,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
goto create_msk;
}
- opt_rx.mptcp.mp_capable = 0;
mptcp_get_options(skb, &opt_rx);
- if (opt_rx.mptcp.mp_capable) {
- subflow_req->remote_key = opt_rx.mptcp.sndr_key;
- subflow_req->remote_key_valid = 1;
- } else {
+ if (!opt_rx.mptcp.mp_capable) {
fallback = true;
goto create_child;
}
create_msk:
- new_msk = mptcp_sk_clone(listener->conn, req);
+ new_msk = mptcp_sk_clone(listener->conn, &opt_rx, req);
if (!new_msk)
fallback = true;
} else if (subflow_req->mp_join) {
@@ -473,6 +469,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
mptcp_pm_new_connection(mptcp_sk(new_msk), 1);
ctx->conn = new_msk;
new_msk = NULL;
+
+ /* with OoO packets we can reach here without ingress
+ * mpc option
+ */
+ ctx->remote_key = opt_rx.mptcp.sndr_key;
+ ctx->fully_established = opt_rx.mptcp.mp_capable;
+ ctx->can_ack = opt_rx.mptcp.mp_capable;
} else if (ctx->mp_join) {
struct mptcp_sock *owner;
@@ -1152,9 +1155,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
* is fully established only after we receive the remote key
*/
new_ctx->mp_capable = 1;
- new_ctx->fully_established = subflow_req->remote_key_valid;
- new_ctx->can_ack = subflow_req->remote_key_valid;
- new_ctx->remote_key = subflow_req->remote_key;
new_ctx->local_key = subflow_req->local_key;
new_ctx->token = subflow_req->token;
new_ctx->ssn_offset = subflow_req->ssn_offset;
--
2.21.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net 3/3] mptcp: drop req socket remote_key* fields
2020-04-20 14:25 ` [PATCH net 3/3] mptcp: drop req socket remote_key* fields Paolo Abeni
@ 2020-04-20 20:05 ` Mat Martineau
2020-04-21 8:24 ` Paolo Abeni
0 siblings, 1 reply; 8+ messages in thread
From: Mat Martineau @ 2020-04-20 20:05 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, Matthieu Baerts, Jakub Kicinski, Christoph Paasch,
Florian Westphal
On Mon, 20 Apr 2020, Paolo Abeni wrote:
> We don't need them, as we can use the current ingress opt
> data instead. Setting them in syn_recv_sock() may causes
> inconsistent mptcp socket status, as per previous commit.
>
> Fixes: cc7972ea1932 ("mptcp: parse and emit MP_CAPABLE option according to v1 spec")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 8 +++++---
> net/mptcp/protocol.h | 8 ++++----
> net/mptcp/subflow.c | 20 ++++++++++----------
> 3 files changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index d275c1e827fe..58ad03fc1bbc 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1345,7 +1345,9 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk)
> }
> #endif
>
> -struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
> +struct sock *mptcp_sk_clone(const struct sock *sk,
> + const struct tcp_options_received *opt_rx,
> + struct request_sock *req)
> {
> struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
> struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);
> @@ -1383,9 +1385,9 @@ struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
>
> msk->write_seq = subflow_req->idsn + 1;
> atomic64_set(&msk->snd_una, msk->write_seq);
> - if (subflow_req->remote_key_valid) {
> + if (opt_rx->mptcp.mp_capable) {
> msk->can_ack = true;
> - msk->remote_key = subflow_req->remote_key;
> + msk->remote_key = opt_rx->mptcp.sndr_key;
> mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
> ack_seq++;
> msk->ack_seq = ack_seq;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 67448002a2d7..a2b3048037d0 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -206,12 +206,10 @@ struct mptcp_subflow_request_sock {
> struct tcp_request_sock sk;
> u16 mp_capable : 1,
> mp_join : 1,
> - backup : 1,
> - remote_key_valid : 1;
> + backup : 1;
> u8 local_id;
> u8 remote_id;
> u64 local_key;
> - u64 remote_key;
> u64 idsn;
> u32 token;
> u32 ssn_offset;
> @@ -332,7 +330,9 @@ void mptcp_proto_init(void);
> int mptcp_proto_v6_init(void);
> #endif
>
> -struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req);
> +struct sock *mptcp_sk_clone(const struct sock *sk,
> + const struct tcp_options_received *opt_rx,
> + struct request_sock *req);
> void mptcp_get_options(const struct sk_buff *skb,
> struct tcp_options_received *opt_rx);
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 10090ca3d3e0..87c094702d63 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -133,7 +133,6 @@ static void subflow_init_req(struct request_sock *req,
>
> subflow_req->mp_capable = 0;
> subflow_req->mp_join = 0;
> - subflow_req->remote_key_valid = 0;
>
> #ifdef CONFIG_TCP_MD5SIG
> /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> @@ -404,6 +403,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>
> pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
>
> + opt_rx.mptcp.mp_capable = 0;
> if (tcp_rsk(req)->is_mptcp == 0)
> goto create_child;
>
> @@ -418,18 +418,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> goto create_msk;
> }
>
> - opt_rx.mptcp.mp_capable = 0;
> mptcp_get_options(skb, &opt_rx);
> - if (opt_rx.mptcp.mp_capable) {
> - subflow_req->remote_key = opt_rx.mptcp.sndr_key;
> - subflow_req->remote_key_valid = 1;
> - } else {
> + if (!opt_rx.mptcp.mp_capable) {
> fallback = true;
> goto create_child;
> }
>
> create_msk:
> - new_msk = mptcp_sk_clone(listener->conn, req);
> + new_msk = mptcp_sk_clone(listener->conn, &opt_rx, req);
> if (!new_msk)
> fallback = true;
> } else if (subflow_req->mp_join) {
> @@ -473,6 +469,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> mptcp_pm_new_connection(mptcp_sk(new_msk), 1);
> ctx->conn = new_msk;
> new_msk = NULL;
> +
> + /* with OoO packets we can reach here without ingress
> + * mpc option
> + */
> + ctx->remote_key = opt_rx.mptcp.sndr_key;
> + ctx->fully_established = opt_rx.mptcp.mp_capable;
> + ctx->can_ack = opt_rx.mptcp.mp_capable;
If this code is reached without an incoming mpc option, it looks like
ctx->remote_key gets junk off the stack. Maybe this instead?
+ if (opt_rx.mptcp.mp_capable) {
+ ctx->remote_key = opt_rx.mptcp.sndr_key;
+ ctx->fully_established = 1;
+ ctx->can_ack = 1;
+ }
Mat
> } else if (ctx->mp_join) {
> struct mptcp_sock *owner;
>
> @@ -1152,9 +1155,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
> * is fully established only after we receive the remote key
> */
> new_ctx->mp_capable = 1;
> - new_ctx->fully_established = subflow_req->remote_key_valid;
> - new_ctx->can_ack = subflow_req->remote_key_valid;
> - new_ctx->remote_key = subflow_req->remote_key;
> new_ctx->local_key = subflow_req->local_key;
> new_ctx->token = subflow_req->token;
> new_ctx->ssn_offset = subflow_req->ssn_offset;
> --
> 2.21.1
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net 3/3] mptcp: drop req socket remote_key* fields
2020-04-20 20:05 ` Mat Martineau
@ 2020-04-21 8:24 ` Paolo Abeni
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Abeni @ 2020-04-21 8:24 UTC (permalink / raw)
To: Mat Martineau
Cc: netdev, Matthieu Baerts, Jakub Kicinski, Christoph Paasch,
Florian Westphal
On Mon, 2020-04-20 at 13:05 -0700, Mat Martineau wrote:
> On Mon, 20 Apr 2020, Paolo Abeni wrote:
>
> > We don't need them, as we can use the current ingress opt
> > data instead. Setting them in syn_recv_sock() may causes
> > inconsistent mptcp socket status, as per previous commit.
> >
> > Fixes: cc7972ea1932 ("mptcp: parse and emit MP_CAPABLE option according to v1 spec")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/protocol.c | 8 +++++---
> > net/mptcp/protocol.h | 8 ++++----
> > net/mptcp/subflow.c | 20 ++++++++++----------
> > 3 files changed, 19 insertions(+), 17 deletions(-)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index d275c1e827fe..58ad03fc1bbc 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1345,7 +1345,9 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk)
> > }
> > #endif
> >
> > -struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
> > +struct sock *mptcp_sk_clone(const struct sock *sk,
> > + const struct tcp_options_received *opt_rx,
> > + struct request_sock *req)
> > {
> > struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
> > struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);
> > @@ -1383,9 +1385,9 @@ struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
> >
> > msk->write_seq = subflow_req->idsn + 1;
> > atomic64_set(&msk->snd_una, msk->write_seq);
> > - if (subflow_req->remote_key_valid) {
> > + if (opt_rx->mptcp.mp_capable) {
> > msk->can_ack = true;
> > - msk->remote_key = subflow_req->remote_key;
> > + msk->remote_key = opt_rx->mptcp.sndr_key;
> > mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
> > ack_seq++;
> > msk->ack_seq = ack_seq;
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 67448002a2d7..a2b3048037d0 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -206,12 +206,10 @@ struct mptcp_subflow_request_sock {
> > struct tcp_request_sock sk;
> > u16 mp_capable : 1,
> > mp_join : 1,
> > - backup : 1,
> > - remote_key_valid : 1;
> > + backup : 1;
> > u8 local_id;
> > u8 remote_id;
> > u64 local_key;
> > - u64 remote_key;
> > u64 idsn;
> > u32 token;
> > u32 ssn_offset;
> > @@ -332,7 +330,9 @@ void mptcp_proto_init(void);
> > int mptcp_proto_v6_init(void);
> > #endif
> >
> > -struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req);
> > +struct sock *mptcp_sk_clone(const struct sock *sk,
> > + const struct tcp_options_received *opt_rx,
> > + struct request_sock *req);
> > void mptcp_get_options(const struct sk_buff *skb,
> > struct tcp_options_received *opt_rx);
> >
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 10090ca3d3e0..87c094702d63 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -133,7 +133,6 @@ static void subflow_init_req(struct request_sock *req,
> >
> > subflow_req->mp_capable = 0;
> > subflow_req->mp_join = 0;
> > - subflow_req->remote_key_valid = 0;
> >
> > #ifdef CONFIG_TCP_MD5SIG
> > /* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> > @@ -404,6 +403,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> >
> > pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
> >
> > + opt_rx.mptcp.mp_capable = 0;
> > if (tcp_rsk(req)->is_mptcp == 0)
> > goto create_child;
> >
> > @@ -418,18 +418,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> > goto create_msk;
> > }
> >
> > - opt_rx.mptcp.mp_capable = 0;
> > mptcp_get_options(skb, &opt_rx);
> > - if (opt_rx.mptcp.mp_capable) {
> > - subflow_req->remote_key = opt_rx.mptcp.sndr_key;
> > - subflow_req->remote_key_valid = 1;
> > - } else {
> > + if (!opt_rx.mptcp.mp_capable) {
> > fallback = true;
> > goto create_child;
> > }
> >
> > create_msk:
> > - new_msk = mptcp_sk_clone(listener->conn, req);
> > + new_msk = mptcp_sk_clone(listener->conn, &opt_rx, req);
> > if (!new_msk)
> > fallback = true;
> > } else if (subflow_req->mp_join) {
> > @@ -473,6 +469,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> > mptcp_pm_new_connection(mptcp_sk(new_msk), 1);
> > ctx->conn = new_msk;
> > new_msk = NULL;
> > +
> > + /* with OoO packets we can reach here without ingress
> > + * mpc option
> > + */
> > + ctx->remote_key = opt_rx.mptcp.sndr_key;
> > + ctx->fully_established = opt_rx.mptcp.mp_capable;
> > + ctx->can_ack = opt_rx.mptcp.mp_capable;
>
> If this code is reached without an incoming mpc option, it looks like
> ctx->remote_key gets junk off the stack. Maybe this instead?
The idea here is to avoid additional conditional. The remote_key will
be used only after 'can_ack' becomes true, and the '!can_ack' condition
here is unlikely. Overall this should be faster and safe.
Thanks for checking,
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net 0/3] mptcp: fix races on accept()
2020-04-20 14:25 [PATCH net 0/3] mptcp: fix races on accept() Paolo Abeni
` (2 preceding siblings ...)
2020-04-20 14:25 ` [PATCH net 3/3] mptcp: drop req socket remote_key* fields Paolo Abeni
@ 2020-04-20 20:02 ` David Miller
2020-04-21 8:38 ` Paolo Abeni
3 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2020-04-20 20:02 UTC (permalink / raw)
To: pabeni; +Cc: netdev, mathew.j.martineau, matthieu.baerts, kuba, cpaasch, fw
From: Paolo Abeni <pabeni@redhat.com>
Date: Mon, 20 Apr 2020 16:25:03 +0200
> This series includes some fixes for accept() races which may cause inconsistent
> MPTCP socket status and oops. Please see the individual patches for the
> technical details.
Series applied, thanks.
It seems like patch #3 might be relevant for v5.6 -stable, what's the
story here?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net 0/3] mptcp: fix races on accept()
2020-04-20 20:02 ` [PATCH net 0/3] mptcp: fix races on accept() David Miller
@ 2020-04-21 8:38 ` Paolo Abeni
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Abeni @ 2020-04-21 8:38 UTC (permalink / raw)
To: David Miller
Cc: netdev, mathew.j.martineau, matthieu.baerts, kuba, cpaasch, fw
On Mon, 2020-04-20 at 13:02 -0700, David Miller wrote:
> From: Paolo Abeni <pabeni@redhat.com>
> Date: Mon, 20 Apr 2020 16:25:03 +0200
>
> > This series includes some fixes for accept() races which may cause inconsistent
> > MPTCP socket status and oops. Please see the individual patches for the
> > technical details.
>
> Series applied, thanks.
>
> It seems like patch #3 might be relevant for v5.6 -stable, what's the
> story here?
Yes, it addresses a race condition present since cc7972ea1932 ("mptcp:
parse and emit MP_CAPABLE option according to v1 spec"). I see now that
the changelog is probably a bit too vague, I'm sorry.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread