* [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling @ 2020-05-13 15:31 Paolo Abeni 2020-05-13 15:31 ` [PATCH net-next 1/3] mptcp: add new sock flag to deal with join subflows Paolo Abeni ` (4 more replies) 0 siblings, 5 replies; 9+ messages in thread From: Paolo Abeni @ 2020-05-13 15:31 UTC (permalink / raw) To: netdev; +Cc: Eric Dumazet, David S . Miller, Christoph Paasch Currently if we hit an MP_JOIN failure on the third ack, the child socket is closed with reset, but the request socket is not deleted, causing weird behaviors. The main problem is that MPTCP's MP_JOIN code needs to plug it's own 'valid 3rd ack' checks and the current TCP callbacks do not allow that. This series tries to address the above shortcoming introducing a new MPTCP specific bit in a 'struct tcp_request_sock' hole, and leveraging that to allow tcp_check_req releasing the request socket when needed. The above allows cleaning-up a bit current MPTCP hooking in tcp_check_req(). An alternative solution, possibly cleaner but more invasive, would be changing the 'bool *own_req' syn_recv_sock() argument into 'int *req_status' and let MPTCP set it to 'REQ_DROP'. RFC -> v1: - move the drop_req bit inside tcp_request_sock (Eric) Paolo Abeni (3): mptcp: add new sock flag to deal with join subflows inet_connection_sock: factor out destroy helper. mptcp: cope better with MP_JOIN failure include/linux/tcp.h | 3 +++ include/net/inet_connection_sock.h | 8 ++++++++ include/net/mptcp.h | 17 ++++++++++------- net/ipv4/inet_connection_sock.c | 6 +----- net/ipv4/tcp_minisocks.c | 2 +- net/mptcp/protocol.c | 7 ------- net/mptcp/subflow.c | 17 +++++++++++------ 7 files changed, 34 insertions(+), 26 deletions(-) -- 2.21.3 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next 1/3] mptcp: add new sock flag to deal with join subflows 2020-05-13 15:31 [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling Paolo Abeni @ 2020-05-13 15:31 ` Paolo Abeni 2020-05-14 20:13 ` Mat Martineau 2020-05-13 15:31 ` [PATCH net-next 2/3] inet_connection_sock: factor out destroy helper Paolo Abeni ` (3 subsequent siblings) 4 siblings, 1 reply; 9+ messages in thread From: Paolo Abeni @ 2020-05-13 15:31 UTC (permalink / raw) To: netdev; +Cc: Eric Dumazet, David S . Miller, Christoph Paasch MP_JOIN subflows must not land into the accept queue. Currently tcp_check_req() calls an mptcp specific helper to detect such scenario. Such helper leverages the subflow context to check for MP_JOIN subflows. We need to deal also with MP JOIN failures, even when the subflow context is not available due allocation failure. A possible solution would be changing the syn_recv_sock() signature to allow returning a more descriptive action/ error code and deal with that in tcp_check_req(). Since the above need is MPTCP specific, this patch instead uses a TCP request socket hole to add a MPTCP specific flag. Such flag is used by the MPTCP syn_recv_sock() to tell tcp_check_req() how to deal with the request socket. This change is a no-op for !MPTCP build, and makes the MPTCP code simpler. It allows also the next patch to deal correctly with MP JOIN failure. RFC -> v1: - move the drop_req bit inside tcp_request_sock (Eric) Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- include/linux/tcp.h | 3 +++ include/net/mptcp.h | 17 ++++++++++------- net/ipv4/tcp_minisocks.c | 2 +- net/mptcp/protocol.c | 7 ------- net/mptcp/subflow.c | 2 ++ 5 files changed, 16 insertions(+), 15 deletions(-) diff --git a/include/linux/tcp.h b/include/linux/tcp.h index e60db06ec28d..bf44e85d709d 100644 --- a/include/linux/tcp.h +++ b/include/linux/tcp.h @@ -120,6 +120,9 @@ struct tcp_request_sock { u64 snt_synack; /* first SYNACK sent time */ bool tfo_listener; bool is_mptcp; +#if IS_ENABLED(CONFIG_MPTCP) + bool drop_req; +#endif u32 txhash; u32 rcv_isn; u32 snt_isn; diff --git a/include/net/mptcp.h b/include/net/mptcp.h index e60275659de6..c4a6ef4ba35b 100644 --- a/include/net/mptcp.h +++ b/include/net/mptcp.h @@ -68,6 +68,11 @@ static inline bool rsk_is_mptcp(const struct request_sock *req) return tcp_rsk(req)->is_mptcp; } +static inline bool rsk_drop_req(const struct request_sock *req) +{ + return tcp_rsk(req)->is_mptcp && tcp_rsk(req)->drop_req; +} + void mptcp_space(const struct sock *ssk, int *space, int *full_space); bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb, unsigned int *size, struct mptcp_out_options *opts); @@ -121,8 +126,6 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to, skb_ext_find(from, SKB_EXT_MPTCP)); } -bool mptcp_sk_is_subflow(const struct sock *sk); - void mptcp_seq_show(struct seq_file *seq); #else @@ -140,6 +143,11 @@ static inline bool rsk_is_mptcp(const struct request_sock *req) return false; } +static inline bool rsk_drop_req(const struct request_sock *req) +{ + return false; +} + static inline void mptcp_parse_option(const struct sk_buff *skb, const unsigned char *ptr, int opsize, struct tcp_options_received *opt_rx) @@ -190,11 +198,6 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to, return true; } -static inline bool mptcp_sk_is_subflow(const struct sock *sk) -{ - return false; -} - static inline void mptcp_space(const struct sock *ssk, int *s, int *fs) { } static inline void mptcp_seq_show(struct seq_file *seq) { } #endif /* CONFIG_MPTCP */ diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c index 7e40322cc5ec..495dda2449fe 100644 --- a/net/ipv4/tcp_minisocks.c +++ b/net/ipv4/tcp_minisocks.c @@ -774,7 +774,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb, if (!child) goto listen_overflow; - if (own_req && sk_is_mptcp(child) && mptcp_sk_is_subflow(child)) { + if (own_req && rsk_drop_req(req)) { reqsk_queue_removed(&inet_csk(sk)->icsk_accept_queue, req); inet_csk_reqsk_queue_drop_and_put(sk, req); return child; diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 6a812dd8b6b6..b974898eb6b5 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1687,13 +1687,6 @@ bool mptcp_finish_join(struct sock *sk) return ret; } -bool mptcp_sk_is_subflow(const struct sock *sk) -{ - struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); - - return subflow->mp_join == 1; -} - static bool mptcp_memory_free(const struct sock *sk, int wake) { struct mptcp_sock *msk = mptcp_sk(sk); diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 009d5c478062..42a8a650ff20 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -500,6 +500,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, ctx->remote_key = mp_opt.sndr_key; ctx->fully_established = mp_opt.mp_capable; ctx->can_ack = mp_opt.mp_capable; + tcp_rsk(req)->drop_req = false; } else if (ctx->mp_join) { struct mptcp_sock *owner; @@ -512,6 +513,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, goto close_child; SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX); + tcp_rsk(req)->drop_req = true; } } -- 2.21.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] mptcp: add new sock flag to deal with join subflows 2020-05-13 15:31 ` [PATCH net-next 1/3] mptcp: add new sock flag to deal with join subflows Paolo Abeni @ 2020-05-14 20:13 ` Mat Martineau 2020-05-15 9:15 ` Paolo Abeni 2020-05-15 15:52 ` Paolo Abeni 0 siblings, 2 replies; 9+ messages in thread From: Mat Martineau @ 2020-05-14 20:13 UTC (permalink / raw) To: Paolo Abeni; +Cc: netdev, Eric Dumazet, David S . Miller, Christoph Paasch Paolo - On Wed, 13 May 2020, Paolo Abeni wrote: > MP_JOIN subflows must not land into the accept queue. > Currently tcp_check_req() calls an mptcp specific helper > to detect such scenario. > > Such helper leverages the subflow context to check for > MP_JOIN subflows. We need to deal also with MP JOIN > failures, even when the subflow context is not available > due allocation failure. > > A possible solution would be changing the syn_recv_sock() > signature to allow returning a more descriptive action/ > error code and deal with that in tcp_check_req(). > > Since the above need is MPTCP specific, this patch instead > uses a TCP request socket hole to add a MPTCP specific flag. > Such flag is used by the MPTCP syn_recv_sock() to tell > tcp_check_req() how to deal with the request socket. > > This change is a no-op for !MPTCP build, and makes the > MPTCP code simpler. It allows also the next patch to deal > correctly with MP JOIN failure. > > RFC -> v1: > - move the drop_req bit inside tcp_request_sock (Eric) > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > include/linux/tcp.h | 3 +++ > include/net/mptcp.h | 17 ++++++++++------- > net/ipv4/tcp_minisocks.c | 2 +- > net/mptcp/protocol.c | 7 ------- > net/mptcp/subflow.c | 2 ++ > 5 files changed, 16 insertions(+), 15 deletions(-) > > diff --git a/include/linux/tcp.h b/include/linux/tcp.h > index e60db06ec28d..bf44e85d709d 100644 > --- a/include/linux/tcp.h > +++ b/include/linux/tcp.h > @@ -120,6 +120,9 @@ struct tcp_request_sock { > u64 snt_synack; /* first SYNACK sent time */ > bool tfo_listener; > bool is_mptcp; > +#if IS_ENABLED(CONFIG_MPTCP) > + bool drop_req; > +#endif > u32 txhash; > u32 rcv_isn; > u32 snt_isn; > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > index e60275659de6..c4a6ef4ba35b 100644 > --- a/include/net/mptcp.h > +++ b/include/net/mptcp.h > @@ -68,6 +68,11 @@ static inline bool rsk_is_mptcp(const struct request_sock *req) > return tcp_rsk(req)->is_mptcp; > } > > +static inline bool rsk_drop_req(const struct request_sock *req) > +{ > + return tcp_rsk(req)->is_mptcp && tcp_rsk(req)->drop_req; > +} > + > void mptcp_space(const struct sock *ssk, int *space, int *full_space); > bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb, > unsigned int *size, struct mptcp_out_options *opts); > @@ -121,8 +126,6 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to, > skb_ext_find(from, SKB_EXT_MPTCP)); > } > > -bool mptcp_sk_is_subflow(const struct sock *sk); > - > void mptcp_seq_show(struct seq_file *seq); > #else > > @@ -140,6 +143,11 @@ static inline bool rsk_is_mptcp(const struct request_sock *req) > return false; > } > > +static inline bool rsk_drop_req(const struct request_sock *req) > +{ > + return false; > +} > + > static inline void mptcp_parse_option(const struct sk_buff *skb, > const unsigned char *ptr, int opsize, > struct tcp_options_received *opt_rx) > @@ -190,11 +198,6 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to, > return true; > } > > -static inline bool mptcp_sk_is_subflow(const struct sock *sk) > -{ > - return false; > -} > - > static inline void mptcp_space(const struct sock *ssk, int *s, int *fs) { } > static inline void mptcp_seq_show(struct seq_file *seq) { } > #endif /* CONFIG_MPTCP */ > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c > index 7e40322cc5ec..495dda2449fe 100644 > --- a/net/ipv4/tcp_minisocks.c > +++ b/net/ipv4/tcp_minisocks.c > @@ -774,7 +774,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb, > if (!child) > goto listen_overflow; > > - if (own_req && sk_is_mptcp(child) && mptcp_sk_is_subflow(child)) { > + if (own_req && rsk_drop_req(req)) { > reqsk_queue_removed(&inet_csk(sk)->icsk_accept_queue, req); > inet_csk_reqsk_queue_drop_and_put(sk, req); > return child; > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 6a812dd8b6b6..b974898eb6b5 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -1687,13 +1687,6 @@ bool mptcp_finish_join(struct sock *sk) > return ret; > } > > -bool mptcp_sk_is_subflow(const struct sock *sk) > -{ > - struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); > - > - return subflow->mp_join == 1; > -} > - > static bool mptcp_memory_free(const struct sock *sk, int wake) > { > struct mptcp_sock *msk = mptcp_sk(sk); > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 009d5c478062..42a8a650ff20 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -500,6 +500,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, > ctx->remote_key = mp_opt.sndr_key; > ctx->fully_established = mp_opt.mp_capable; > ctx->can_ack = mp_opt.mp_capable; > + tcp_rsk(req)->drop_req = false; > } else if (ctx->mp_join) { > struct mptcp_sock *owner; > > @@ -512,6 +513,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, > goto close_child; > > SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX); > + tcp_rsk(req)->drop_req = true; > } It looks like tcp_rsk(req)->drop_req gets initialized when the ctx->mp_capable and ctx->mp_join values are set as expected, but it's hard to tell if it's guaranteed to be initialized in every corner case (any allocation or token failures, unexpected input, etc.). Patch 3 will set drop_req for some cases, but does it makes sense to set tcp_rsk(req)->drop_req in subflow_v{4,6}_init_req() here as an additional 'else' clause? > } > > -- > 2.21.3 > > -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] mptcp: add new sock flag to deal with join subflows 2020-05-14 20:13 ` Mat Martineau @ 2020-05-15 9:15 ` Paolo Abeni 2020-05-15 15:52 ` Paolo Abeni 1 sibling, 0 replies; 9+ messages in thread From: Paolo Abeni @ 2020-05-15 9:15 UTC (permalink / raw) To: Mat Martineau; +Cc: netdev, Eric Dumazet, David S . Miller, Christoph Paasch On Thu, 2020-05-14 at 13:13 -0700, Mat Martineau wrote: > On Wed, 13 May 2020, Paolo Abeni wrote: > > > MP_JOIN subflows must not land into the accept queue. > > Currently tcp_check_req() calls an mptcp specific helper > > to detect such scenario. > > > > Such helper leverages the subflow context to check for > > MP_JOIN subflows. We need to deal also with MP JOIN > > failures, even when the subflow context is not available > > due allocation failure. > > > > A possible solution would be changing the syn_recv_sock() > > signature to allow returning a more descriptive action/ > > error code and deal with that in tcp_check_req(). > > > > Since the above need is MPTCP specific, this patch instead > > uses a TCP request socket hole to add a MPTCP specific flag. > > Such flag is used by the MPTCP syn_recv_sock() to tell > > tcp_check_req() how to deal with the request socket. > > > > This change is a no-op for !MPTCP build, and makes the > > MPTCP code simpler. It allows also the next patch to deal > > correctly with MP JOIN failure. > > > > RFC -> v1: > > - move the drop_req bit inside tcp_request_sock (Eric) > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > include/linux/tcp.h | 3 +++ > > include/net/mptcp.h | 17 ++++++++++------- > > net/ipv4/tcp_minisocks.c | 2 +- > > net/mptcp/protocol.c | 7 ------- > > net/mptcp/subflow.c | 2 ++ > > 5 files changed, 16 insertions(+), 15 deletions(-) > > > > diff --git a/include/linux/tcp.h b/include/linux/tcp.h > > index e60db06ec28d..bf44e85d709d 100644 > > --- a/include/linux/tcp.h > > +++ b/include/linux/tcp.h > > @@ -120,6 +120,9 @@ struct tcp_request_sock { > > u64 snt_synack; /* first SYNACK sent time */ > > bool tfo_listener; > > bool is_mptcp; > > +#if IS_ENABLED(CONFIG_MPTCP) > > + bool drop_req; > > +#endif > > u32 txhash; > > u32 rcv_isn; > > u32 snt_isn; > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > > index e60275659de6..c4a6ef4ba35b 100644 > > --- a/include/net/mptcp.h > > +++ b/include/net/mptcp.h > > @@ -68,6 +68,11 @@ static inline bool rsk_is_mptcp(const struct request_sock *req) > > return tcp_rsk(req)->is_mptcp; > > } > > > > +static inline bool rsk_drop_req(const struct request_sock *req) > > +{ > > + return tcp_rsk(req)->is_mptcp && tcp_rsk(req)->drop_req; > > +} > > + > > void mptcp_space(const struct sock *ssk, int *space, int *full_space); > > bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb, > > unsigned int *size, struct mptcp_out_options *opts); > > @@ -121,8 +126,6 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to, > > skb_ext_find(from, SKB_EXT_MPTCP)); > > } > > > > -bool mptcp_sk_is_subflow(const struct sock *sk); > > - > > void mptcp_seq_show(struct seq_file *seq); > > #else > > > > @@ -140,6 +143,11 @@ static inline bool rsk_is_mptcp(const struct request_sock *req) > > return false; > > } > > > > +static inline bool rsk_drop_req(const struct request_sock *req) > > +{ > > + return false; > > +} > > + > > static inline void mptcp_parse_option(const struct sk_buff *skb, > > const unsigned char *ptr, int opsize, > > struct tcp_options_received *opt_rx) > > @@ -190,11 +198,6 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to, > > return true; > > } > > > > -static inline bool mptcp_sk_is_subflow(const struct sock *sk) > > -{ > > - return false; > > -} > > - > > static inline void mptcp_space(const struct sock *ssk, int *s, int *fs) { } > > static inline void mptcp_seq_show(struct seq_file *seq) { } > > #endif /* CONFIG_MPTCP */ > > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c > > index 7e40322cc5ec..495dda2449fe 100644 > > --- a/net/ipv4/tcp_minisocks.c > > +++ b/net/ipv4/tcp_minisocks.c > > @@ -774,7 +774,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb, > > if (!child) > > goto listen_overflow; > > > > - if (own_req && sk_is_mptcp(child) && mptcp_sk_is_subflow(child)) { > > + if (own_req && rsk_drop_req(req)) { > > reqsk_queue_removed(&inet_csk(sk)->icsk_accept_queue, req); > > inet_csk_reqsk_queue_drop_and_put(sk, req); > > return child; > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > index 6a812dd8b6b6..b974898eb6b5 100644 > > --- a/net/mptcp/protocol.c > > +++ b/net/mptcp/protocol.c > > @@ -1687,13 +1687,6 @@ bool mptcp_finish_join(struct sock *sk) > > return ret; > > } > > > > -bool mptcp_sk_is_subflow(const struct sock *sk) > > -{ > > - struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); > > - > > - return subflow->mp_join == 1; > > -} > > - > > static bool mptcp_memory_free(const struct sock *sk, int wake) > > { > > struct mptcp_sock *msk = mptcp_sk(sk); > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > > index 009d5c478062..42a8a650ff20 100644 > > --- a/net/mptcp/subflow.c > > +++ b/net/mptcp/subflow.c > > @@ -500,6 +500,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, > > ctx->remote_key = mp_opt.sndr_key; > > ctx->fully_established = mp_opt.mp_capable; > > ctx->can_ack = mp_opt.mp_capable; > > + tcp_rsk(req)->drop_req = false; > > } else if (ctx->mp_join) { > > struct mptcp_sock *owner; > > > > @@ -512,6 +513,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, > > goto close_child; > > > > SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX); > > + tcp_rsk(req)->drop_req = true; > > } > > It looks like tcp_rsk(req)->drop_req gets initialized when the > ctx->mp_capable and ctx->mp_join values are set as expected, but it's hard > to tell if it's guaranteed to be initialized in every corner case (any > allocation or token failures, unexpected input, etc.). > > Patch 3 will set drop_req for some cases, but does it makes sense to > set tcp_rsk(req)->drop_req in subflow_v{4,6}_init_req() here as an > additional 'else' clause? We already have such check in place in subflow_ulp_clone(): if both subflow_req->mp_join and subflow_req->mp_capable are cleared we fallback, elsewhere we set either subflow->mp_capable or subflow- >mp_join. Later in subflow_ulp_clone() we have similar scenario, where do: } else if (subflow_req->mp_join) { while we could simply use a plain 'else'. To make things clearer, I can replace both 'else if'... with plain else (possibly in a separate patch), WDYT? Thanks, Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] mptcp: add new sock flag to deal with join subflows 2020-05-14 20:13 ` Mat Martineau 2020-05-15 9:15 ` Paolo Abeni @ 2020-05-15 15:52 ` Paolo Abeni 1 sibling, 0 replies; 9+ messages in thread From: Paolo Abeni @ 2020-05-15 15:52 UTC (permalink / raw) To: Mat Martineau; +Cc: netdev, Eric Dumazet, David S . Miller, Christoph Paasch On Thu, 2020-05-14 at 13:13 -0700, Mat Martineau wrote: > Paolo - > > On Wed, 13 May 2020, Paolo Abeni wrote: > > > MP_JOIN subflows must not land into the accept queue. > > Currently tcp_check_req() calls an mptcp specific helper > > to detect such scenario. > > > > Such helper leverages the subflow context to check for > > MP_JOIN subflows. We need to deal also with MP JOIN > > failures, even when the subflow context is not available > > due allocation failure. > > > > A possible solution would be changing the syn_recv_sock() > > signature to allow returning a more descriptive action/ > > error code and deal with that in tcp_check_req(). > > > > Since the above need is MPTCP specific, this patch instead > > uses a TCP request socket hole to add a MPTCP specific flag. > > Such flag is used by the MPTCP syn_recv_sock() to tell > > tcp_check_req() how to deal with the request socket. > > > > This change is a no-op for !MPTCP build, and makes the > > MPTCP code simpler. It allows also the next patch to deal > > correctly with MP JOIN failure. > > > > RFC -> v1: > > - move the drop_req bit inside tcp_request_sock (Eric) > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > include/linux/tcp.h | 3 +++ > > include/net/mptcp.h | 17 ++++++++++------- > > net/ipv4/tcp_minisocks.c | 2 +- > > net/mptcp/protocol.c | 7 ------- > > net/mptcp/subflow.c | 2 ++ > > 5 files changed, 16 insertions(+), 15 deletions(-) > > > > diff --git a/include/linux/tcp.h b/include/linux/tcp.h > > index e60db06ec28d..bf44e85d709d 100644 > > --- a/include/linux/tcp.h > > +++ b/include/linux/tcp.h > > @@ -120,6 +120,9 @@ struct tcp_request_sock { > > u64 snt_synack; /* first SYNACK sent time */ > > bool tfo_listener; > > bool is_mptcp; > > +#if IS_ENABLED(CONFIG_MPTCP) > > + bool drop_req; > > +#endif > > u32 txhash; > > u32 rcv_isn; > > u32 snt_isn; > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > > index e60275659de6..c4a6ef4ba35b 100644 > > --- a/include/net/mptcp.h > > +++ b/include/net/mptcp.h > > @@ -68,6 +68,11 @@ static inline bool rsk_is_mptcp(const struct request_sock *req) > > return tcp_rsk(req)->is_mptcp; > > } > > > > +static inline bool rsk_drop_req(const struct request_sock *req) > > +{ > > + return tcp_rsk(req)->is_mptcp && tcp_rsk(req)->drop_req; > > +} > > + > > void mptcp_space(const struct sock *ssk, int *space, int *full_space); > > bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb, > > unsigned int *size, struct mptcp_out_options *opts); > > @@ -121,8 +126,6 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to, > > skb_ext_find(from, SKB_EXT_MPTCP)); > > } > > > > -bool mptcp_sk_is_subflow(const struct sock *sk); > > - > > void mptcp_seq_show(struct seq_file *seq); > > #else > > > > @@ -140,6 +143,11 @@ static inline bool rsk_is_mptcp(const struct request_sock *req) > > return false; > > } > > > > +static inline bool rsk_drop_req(const struct request_sock *req) > > +{ > > + return false; > > +} > > + > > static inline void mptcp_parse_option(const struct sk_buff *skb, > > const unsigned char *ptr, int opsize, > > struct tcp_options_received *opt_rx) > > @@ -190,11 +198,6 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to, > > return true; > > } > > > > -static inline bool mptcp_sk_is_subflow(const struct sock *sk) > > -{ > > - return false; > > -} > > - > > static inline void mptcp_space(const struct sock *ssk, int *s, int *fs) { } > > static inline void mptcp_seq_show(struct seq_file *seq) { } > > #endif /* CONFIG_MPTCP */ > > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c > > index 7e40322cc5ec..495dda2449fe 100644 > > --- a/net/ipv4/tcp_minisocks.c > > +++ b/net/ipv4/tcp_minisocks.c > > @@ -774,7 +774,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb, > > if (!child) > > goto listen_overflow; > > > > - if (own_req && sk_is_mptcp(child) && mptcp_sk_is_subflow(child)) { > > + if (own_req && rsk_drop_req(req)) { > > reqsk_queue_removed(&inet_csk(sk)->icsk_accept_queue, req); > > inet_csk_reqsk_queue_drop_and_put(sk, req); > > return child; > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > index 6a812dd8b6b6..b974898eb6b5 100644 > > --- a/net/mptcp/protocol.c > > +++ b/net/mptcp/protocol.c > > @@ -1687,13 +1687,6 @@ bool mptcp_finish_join(struct sock *sk) > > return ret; > > } > > > > -bool mptcp_sk_is_subflow(const struct sock *sk) > > -{ > > - struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); > > - > > - return subflow->mp_join == 1; > > -} > > - > > static bool mptcp_memory_free(const struct sock *sk, int wake) > > { > > struct mptcp_sock *msk = mptcp_sk(sk); > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > > index 009d5c478062..42a8a650ff20 100644 > > --- a/net/mptcp/subflow.c > > +++ b/net/mptcp/subflow.c > > @@ -500,6 +500,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, > > ctx->remote_key = mp_opt.sndr_key; > > ctx->fully_established = mp_opt.mp_capable; > > ctx->can_ack = mp_opt.mp_capable; > > + tcp_rsk(req)->drop_req = false; > > } else if (ctx->mp_join) { > > struct mptcp_sock *owner; > > > > @@ -512,6 +513,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, > > goto close_child; > > > > SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX); > > + tcp_rsk(req)->drop_req = true; > > } > > It looks like tcp_rsk(req)->drop_req gets initialized when the > ctx->mp_capable and ctx->mp_join values are set as expected, but it's hard > to tell if it's guaranteed to be initialized in every corner case (any > allocation or token failures, unexpected input, etc.). > > Patch 3 will set drop_req for some cases, but does it makes sense to > set tcp_rsk(req)->drop_req in subflow_v{4,6}_init_req() here as an > additional 'else' clause? More testing confirmed I really missed a code-path for 'drop_req' initialization. So I'll move 'tcp_rsk(req)->drop_req = false;' at the top of 'own_req' conditional, just to stay safe. I'll post v2 after more testing. Thanks! Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next 2/3] inet_connection_sock: factor out destroy helper. 2020-05-13 15:31 [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling Paolo Abeni 2020-05-13 15:31 ` [PATCH net-next 1/3] mptcp: add new sock flag to deal with join subflows Paolo Abeni @ 2020-05-13 15:31 ` Paolo Abeni 2020-05-13 15:31 ` [PATCH net-next 3/3] mptcp: cope better with MP_JOIN failure Paolo Abeni ` (2 subsequent siblings) 4 siblings, 0 replies; 9+ messages in thread From: Paolo Abeni @ 2020-05-13 15:31 UTC (permalink / raw) To: netdev; +Cc: Eric Dumazet, David S . Miller, Christoph Paasch Move the steps to prepare an inet_connection_sock for forced disposal inside a separate helper. No functional changes inteded, this will just simplify the next patch. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- include/net/inet_connection_sock.h | 8 ++++++++ net/ipv4/inet_connection_sock.c | 6 +----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h index a3f076befa4f..2f1f8c3efb26 100644 --- a/include/net/inet_connection_sock.h +++ b/include/net/inet_connection_sock.h @@ -287,6 +287,14 @@ static inline int inet_csk_reqsk_queue_is_full(const struct sock *sk) void inet_csk_reqsk_queue_drop(struct sock *sk, struct request_sock *req); void inet_csk_reqsk_queue_drop_and_put(struct sock *sk, struct request_sock *req); +static inline void inet_csk_prepare_for_destroy_sock(struct sock *sk) +{ + /* The below has to be done to allow calling inet_csk_destroy_sock */ + sock_set_flag(sk, SOCK_DEAD); + percpu_counter_inc(sk->sk_prot->orphan_count); + inet_sk(sk)->inet_num = 0; +} + void inet_csk_destroy_sock(struct sock *sk); void inet_csk_prepare_forced_close(struct sock *sk); diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index 5f34eb951627..d6faf3702824 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -896,11 +896,7 @@ void inet_csk_prepare_forced_close(struct sock *sk) /* sk_clone_lock locked the socket and set refcnt to 2 */ bh_unlock_sock(sk); sock_put(sk); - - /* The below has to be done to allow calling inet_csk_destroy_sock */ - sock_set_flag(sk, SOCK_DEAD); - percpu_counter_inc(sk->sk_prot->orphan_count); - inet_sk(sk)->inet_num = 0; + inet_csk_prepare_for_destroy_sock(sk); } EXPORT_SYMBOL(inet_csk_prepare_forced_close); -- 2.21.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 3/3] mptcp: cope better with MP_JOIN failure 2020-05-13 15:31 [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling Paolo Abeni 2020-05-13 15:31 ` [PATCH net-next 1/3] mptcp: add new sock flag to deal with join subflows Paolo Abeni 2020-05-13 15:31 ` [PATCH net-next 2/3] inet_connection_sock: factor out destroy helper Paolo Abeni @ 2020-05-13 15:31 ` Paolo Abeni 2020-05-13 17:07 ` [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling Christoph Paasch 2020-05-14 22:19 ` David Miller 4 siblings, 0 replies; 9+ messages in thread From: Paolo Abeni @ 2020-05-13 15:31 UTC (permalink / raw) To: netdev; +Cc: Eric Dumazet, David S . Miller, Christoph Paasch Currently, on MP_JOIN failure we reset the child socket, but leave the request socket untouched. tcp_check_req will deal with it according to the 'tcp_abort_on_overflow' sysctl value - by default the req socket will stay alive. The above leads to inconsistent behavior on MP JOIN failure, and bad listener overflow accounting. This patch addresses the issue leveraging the infrastructure just introduced to ask the TCP stack to drop the req on failure. The child socket is not freed anymore by subflow_syn_recv_sock(), instead it's moved to a dead state and will be disposed by the next sock_put done by the TCP stack, so that listener overflow accounting is not affected by MP JOIN failure. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/subflow.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 42a8a650ff20..b57c07168468 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -476,7 +476,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, */ if (!ctx || fallback) { if (fallback_is_fatal) - goto close_child; + goto dispose_child; if (ctx) { subflow_ulp_fallback(child, ctx); @@ -506,11 +506,11 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, owner = mptcp_token_get_sock(ctx->token); if (!owner) - goto close_child; + goto dispose_child; ctx->conn = (struct sock *)owner; if (!mptcp_finish_join(child)) - goto close_child; + goto dispose_child; SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX); tcp_rsk(req)->drop_req = true; @@ -530,11 +530,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, !mptcp_subflow_ctx(child)->conn)); return child; -close_child: +dispose_child: + tcp_rsk(req)->drop_req = true; tcp_send_active_reset(child, GFP_ATOMIC); - inet_csk_prepare_forced_close(child); + inet_csk_prepare_for_destroy_sock(child); tcp_done(child); - return NULL; + + /* The last child reference will be released by the caller */ + return child; } static struct inet_connection_sock_af_ops subflow_specific; -- 2.21.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling 2020-05-13 15:31 [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling Paolo Abeni ` (2 preceding siblings ...) 2020-05-13 15:31 ` [PATCH net-next 3/3] mptcp: cope better with MP_JOIN failure Paolo Abeni @ 2020-05-13 17:07 ` Christoph Paasch 2020-05-14 22:19 ` David Miller 4 siblings, 0 replies; 9+ messages in thread From: Christoph Paasch @ 2020-05-13 17:07 UTC (permalink / raw) To: Paolo Abeni; +Cc: netdev, Eric Dumazet, David S . Miller On 13/05/20 - 17:31:01, Paolo Abeni wrote: > Currently if we hit an MP_JOIN failure on the third ack, the child socket is > closed with reset, but the request socket is not deleted, causing weird > behaviors. > > The main problem is that MPTCP's MP_JOIN code needs to plug it's own > 'valid 3rd ack' checks and the current TCP callbacks do not allow that. > > This series tries to address the above shortcoming introducing a new MPTCP > specific bit in a 'struct tcp_request_sock' hole, and leveraging that to allow > tcp_check_req releasing the request socket when needed. > > The above allows cleaning-up a bit current MPTCP hooking in tcp_check_req(). > > An alternative solution, possibly cleaner but more invasive, would be > changing the 'bool *own_req' syn_recv_sock() argument into 'int *req_status' > and let MPTCP set it to 'REQ_DROP'. > > RFC -> v1: > - move the drop_req bit inside tcp_request_sock (Eric) > > Paolo Abeni (3): > mptcp: add new sock flag to deal with join subflows > inet_connection_sock: factor out destroy helper. > mptcp: cope better with MP_JOIN failure > > include/linux/tcp.h | 3 +++ > include/net/inet_connection_sock.h | 8 ++++++++ > include/net/mptcp.h | 17 ++++++++++------- > net/ipv4/inet_connection_sock.c | 6 +----- > net/ipv4/tcp_minisocks.c | 2 +- > net/mptcp/protocol.c | 7 ------- > net/mptcp/subflow.c | 17 +++++++++++------ > 7 files changed, 34 insertions(+), 26 deletions(-) Reviewed-by: Christoph Paasch <cpaasch@apple.com> Christoph ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling 2020-05-13 15:31 [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling Paolo Abeni ` (3 preceding siblings ...) 2020-05-13 17:07 ` [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling Christoph Paasch @ 2020-05-14 22:19 ` David Miller 4 siblings, 0 replies; 9+ messages in thread From: David Miller @ 2020-05-14 22:19 UTC (permalink / raw) To: pabeni; +Cc: netdev, edumazet, cpaasch Paolo, please respond to the feedback you received for patch #1. Thank you. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-05-15 15:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-13 15:31 [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling Paolo Abeni 2020-05-13 15:31 ` [PATCH net-next 1/3] mptcp: add new sock flag to deal with join subflows Paolo Abeni 2020-05-14 20:13 ` Mat Martineau 2020-05-15 9:15 ` Paolo Abeni 2020-05-15 15:52 ` Paolo Abeni 2020-05-13 15:31 ` [PATCH net-next 2/3] inet_connection_sock: factor out destroy helper Paolo Abeni 2020-05-13 15:31 ` [PATCH net-next 3/3] mptcp: cope better with MP_JOIN failure Paolo Abeni 2020-05-13 17:07 ` [PATCH net-next 0/3] mptcp: fix MP_JOIN failure handling Christoph Paasch 2020-05-14 22:19 ` David Miller
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.