* [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs @ 2021-06-26 9:16 wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106 ` (4 more replies) 0 siblings, 5 replies; 10+ messages in thread From: wujianguo106 @ 2021-06-26 9:16 UTC (permalink / raw) To: mptcp; +Cc: pabeni, mathew.j.martineau From: wujianguo <wujianguo@chinatelecom.cn> v7->v8: - patch4: using bool for return value, and keep check_fully_established() untouched. v6->v7: - patch4: corret the declaration of mptcp_incoming_options() when CONFIG_MPTCP is not set - patch5: add tag Reported-by: kernel test robot <oliver.sang@intel.com> v5->v6: - patch1: describe the testing tools in the commit log - patch4: add return value to mptcp_incoming_options(), return 0 if a subflow has been reset, else return 1, and drop the packet in tcp_data_queue()/tcp_rcv_state_process() if the return value is 0 - patch5: update selftest case "multiple subflows limited by server", since the expected behavior is changed by patch4 v4->v5: - patch4: add comment v3->v4: - patch1: using seq and sport/dport for hashing, and ignore network headers altogether, as suggest by Florian v2->v3: - patch1: directly use inet6_ehashfn() for IPv6 - patch4: add Fixes tag. v1->v2: - patch1: handle ipv6 sockets/addresses, always use 4-tuple drived hash and never look at skb->hash - patch3: split into 2 patches. - patch4: new added. Jianguo Wu (5): mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join mptcp: remove redundant req destruct in subflow_check_req() mptcp: fix syncookie process if mptcp can not_accept new subflow mptcp: avoid processing packet if a subflow reset selftests: mptcp: fix case multiple subflows limited by server include/net/mptcp.h | 5 +++-- net/ipv4/tcp_input.c | 19 +++++++++++++++---- net/mptcp/options.c | 19 +++++++++++++------ net/mptcp/subflow.c | 11 +++-------- net/mptcp/syncookies.c | 16 +++++++++++++++- tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +- 6 files changed, 50 insertions(+), 22 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join 2021-06-26 9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106 @ 2021-06-26 9:16 ` wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 2/5] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106 ` (3 subsequent siblings) 4 siblings, 0 replies; 10+ messages in thread From: wujianguo106 @ 2021-06-26 9:16 UTC (permalink / raw) To: mptcp; +Cc: pabeni, mathew.j.martineau From: Jianguo Wu <wujianguo@chinatelecom.cn> I did stress test with wrk(https://github.com/wg/wrk) and webfsd(https://github.com/ourway/webfsd) with the assistance of mptcp-tools(https://github.com/pabeni/mptcp-tools): Server side: ./use_mptcp.sh webfsd -4 -R /tmp/ -p 8099 Client side: ./use_mptcp.sh wrk -c 200 -d 30 -t 4 http://192.168.174.129:8099/ and got the following warning message: [ 55.552626] TCP: request_sock_subflow: Possible SYN flooding on port 8099. Sending cookies. Check SNMP counters. [ 55.553024] ------------[ cut here ]------------ [ 55.553027] WARNING: CPU: 0 PID: 10 at net/core/flow_dissector.c:984 __skb_flow_dissect+0x280/0x1650 ... [ 55.553117] CPU: 0 PID: 10 Comm: ksoftirqd/0 Not tainted 5.12.0+ #18 [ 55.553121] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020 [ 55.553124] RIP: 0010:__skb_flow_dissect+0x280/0x1650 ... [ 55.553133] RSP: 0018:ffffb79580087770 EFLAGS: 00010246 [ 55.553137] RAX: 0000000000000000 RBX: ffffffff8ddb58e0 RCX: ffffb79580087888 [ 55.553139] RDX: ffffffff8ddb58e0 RSI: ffff8f7e4652b600 RDI: 0000000000000000 [ 55.553141] RBP: ffffb79580087858 R08: 0000000000000000 R09: 0000000000000008 [ 55.553143] R10: 000000008c622965 R11: 00000000d3313a5b R12: ffff8f7e4652b600 [ 55.553146] R13: ffff8f7e465c9062 R14: 0000000000000000 R15: ffffb79580087888 [ 55.553149] FS: 0000000000000000(0000) GS:ffff8f7f75e00000(0000) knlGS:0000000000000000 [ 55.553152] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 55.553154] CR2: 00007f73d1d19000 CR3: 0000000135e10004 CR4: 00000000003706f0 [ 55.553160] Call Trace: [ 55.553166] ? __sha256_final+0x67/0xd0 [ 55.553173] ? sha256+0x7e/0xa0 [ 55.553177] __skb_get_hash+0x57/0x210 [ 55.553182] subflow_init_req_cookie_join_save+0xac/0xc0 [ 55.553189] subflow_check_req+0x474/0x550 [ 55.553195] ? ip_route_output_key_hash+0x67/0x90 [ 55.553200] ? xfrm_lookup_route+0x1d/0xa0 [ 55.553207] subflow_v4_route_req+0x8e/0xd0 [ 55.553212] tcp_conn_request+0x31e/0xab0 [ 55.553218] ? selinux_socket_sock_rcv_skb+0x116/0x210 [ 55.553224] ? tcp_rcv_state_process+0x179/0x6d0 [ 55.553229] tcp_rcv_state_process+0x179/0x6d0 [ 55.553235] tcp_v4_do_rcv+0xaf/0x220 [ 55.553239] tcp_v4_rcv+0xce4/0xd80 [ 55.553243] ? ip_route_input_rcu+0x246/0x260 [ 55.553248] ip_protocol_deliver_rcu+0x35/0x1b0 [ 55.553253] ip_local_deliver_finish+0x44/0x50 [ 55.553258] ip_local_deliver+0x6c/0x110 [ 55.553262] ? ip_rcv_finish_core.isra.19+0x5a/0x400 [ 55.553267] ip_rcv+0xd1/0xe0 ... After debugging, I found in __skb_flow_dissect(), skb->dev and skb->sk are both NULL, then net is NULL, and trigger WARN_ON_ONCE(!net), actually net is always NULL in this code path, as skb->dev is set to NULL in tcp_v4_rcv(), and skb->sk is never set. Code snippet in __skb_flow_dissect() that trigger warning: 975 if (skb) { 976 if (!net) { 977 if (skb->dev) 978 net = dev_net(skb->dev); 979 else if (skb->sk) 980 net = sock_net(skb->sk); 981 } 982 } 983 984 WARN_ON_ONCE(!net); So, using seq and transport header derived hash. Fixes: 9466a1ccebbe("mptcp: enable JOIN requests even if cookies are in use"). Suggested-by: Paolo Abeni <pabeni@redhat.com> Suggested-by: Florian Westphal <fw@strlen.de> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> --- net/mptcp/syncookies.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/net/mptcp/syncookies.c b/net/mptcp/syncookies.c index abe0fd0..3712778 100644 --- a/net/mptcp/syncookies.c +++ b/net/mptcp/syncookies.c @@ -37,7 +37,21 @@ struct join_entry { static u32 mptcp_join_entry_hash(struct sk_buff *skb, struct net *net) { - u32 i = skb_get_hash(skb) ^ net_hash_mix(net); + static u32 mptcp_join_hash_secret __read_mostly; + struct tcphdr *th = tcp_hdr(skb); + u32 seq, i; + + net_get_random_once(&mptcp_join_hash_secret, + sizeof(mptcp_join_hash_secret)); + + if (th->syn) + seq = TCP_SKB_CB(skb)->seq; + else + seq = TCP_SKB_CB(skb)->seq - 1; + + i = jhash_3words(seq, net_hash_mix(net), + (__force __u32)th->source << 16 | (__force __u32)th->dest, + mptcp_join_hash_secret); return i % ARRAY_SIZE(join_entries); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH mptcp-net v8 2/5] mptcp: remove redundant req destruct in subflow_check_req() 2021-06-26 9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106 @ 2021-06-26 9:16 ` wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106 ` (2 subsequent siblings) 4 siblings, 0 replies; 10+ messages in thread From: wujianguo106 @ 2021-06-26 9:16 UTC (permalink / raw) To: mptcp; +Cc: pabeni, mathew.j.martineau From: Jianguo Wu <wujianguo@chinatelecom.cn> In subflow_check_req(), if subflow sport is mismatch, will put msk, destroy token, and destruct req, then return -EPERM, which can be done by subflow_req_destructor() via: tcp_conn_request() |--__reqsk_free() |--subflow_req_destructor() So we should remove these redundant code, otherwise will call tcp_v4_reqsk_destructor() twice, and may double free inet_rsk(req)->ireq_opt. Fixes: 5bc56388c74f ("mptcp: add port number check for MP_JOIN") Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> --- net/mptcp/subflow.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 0b5d4a3..3a849fb 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -214,11 +214,6 @@ static int subflow_check_req(struct request_sock *req, ntohs(inet_sk(sk_listener)->inet_sport), ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport)); if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) { - sock_put((struct sock *)subflow_req->msk); - mptcp_token_destroy_request(req); - tcp_request_sock_ops.destructor(req); - subflow_req->msk = NULL; - subflow_req->mp_join = 0; SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX); return -EPERM; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH mptcp-net v8 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow 2021-06-26 9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 2/5] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106 @ 2021-06-26 9:16 ` wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 5/5] selftests: mptcp: fix case multiple subflows limited by server wujianguo106 4 siblings, 0 replies; 10+ messages in thread From: wujianguo106 @ 2021-06-26 9:16 UTC (permalink / raw) To: mptcp; +Cc: pabeni, mathew.j.martineau From: Jianguo Wu <wujianguo@chinatelecom.cn> Lots of "TCP: tcp_fin: Impossible, sk->sk_state=7" in client side when doing stress testing using wrk and webfsd. There are at least two cases may trigger this warning: 1.mptcp is in syncookie, and server recv MP_JOIN SYN request, in subflow_check_req(), the mptcp_can_accept_new_subflow() return false, so subflow_init_req_cookie_join_save() isn't called, i.e. not store the data present in the MP_JOIN syn request and the random nonce in hash table - join_entries[], but still send synack. When recv 3rd-ack, mptcp_token_join_cookie_init_state() will return false, and 3rd-ack is dropped, then if mptcp conn is closed by client, client will send a DATA_FIN and a MPTCP FIN, the DATA_FIN doesn't have MP_CAPABLE or MP_JOIN, so mptcp_subflow_init_cookie_req() will return 0, and pass the cookie check, MP_JOIN request is fallback to normal TCP. Server will send a TCP FIN if closed, in client side, when process TCP FIN, it will do reset, the code path is: tcp_data_queue()->mptcp_incoming_options() ->check_fully_established()->mptcp_subflow_reset(). mptcp_subflow_reset() will set sock state to TCP_CLOSE, so tcp_fin will hit TCP_CLOSE, and print the warning. 2.mptcp is in syncookie, and server recv 3rd-ack, in mptcp_subflow_init_cookie_req(), mptcp_can_accept_new_subflow() return false, and subflow_req->mp_join is not set to 1, so in subflow_syn_recv_sock() will not reset the MP_JOIN subflow, but fallback to normal TCP, and then the same thing happens when server will send a TCP FIN if closed. For case1, subflow_check_req() return -EPERM, then tcp_conn_request() will drop MP_JOIN SYN. For case2, let subflow_syn_recv_sock() call mptcp_can_accept_new_subflow(), and do fatal fallback, send reset. Fixes: 9466a1ccebbe("mptcp: enable JOIN requests even if cookies are in use") Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> --- net/mptcp/subflow.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 3a849fb..f3f1358 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -225,6 +225,8 @@ static int subflow_check_req(struct request_sock *req, if (unlikely(req->syncookie)) { if (mptcp_can_accept_new_subflow(subflow_req->msk)) subflow_init_req_cookie_join_save(subflow_req, skb); + else + return -EPERM; } pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token, @@ -264,9 +266,7 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req, if (!mptcp_token_join_cookie_init_state(subflow_req, skb)) return -EINVAL; - if (mptcp_can_accept_new_subflow(subflow_req->msk)) - subflow_req->mp_join = 1; - + subflow_req->mp_join = 1; subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq - 1; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset 2021-06-26 9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106 ` (2 preceding siblings ...) 2021-06-26 9:16 ` [PATCH mptcp-net v8 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106 @ 2021-06-26 9:16 ` wujianguo106 2021-06-28 8:35 ` Geliang Tang 2021-06-26 9:16 ` [PATCH mptcp-net v8 5/5] selftests: mptcp: fix case multiple subflows limited by server wujianguo106 4 siblings, 1 reply; 10+ messages in thread From: wujianguo106 @ 2021-06-26 9:16 UTC (permalink / raw) To: mptcp; +Cc: pabeni, mathew.j.martineau From: Jianguo Wu <wujianguo@chinatelecom.cn> If check_fully_established() causes a subflow reset, it should not continue to process the packet in tcp_data_queue(). Add a return value to mptcp_incoming_options(), and return false if a subflow has been reset, else return true. Then drop the packet in tcp_data_queue()/tcp_rcv_state_process() if mptcp_incoming_options() return false. Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> --- include/net/mptcp.h | 5 +++-- net/ipv4/tcp_input.c | 19 +++++++++++++++---- net/mptcp/options.c | 19 +++++++++++++------ 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/include/net/mptcp.h b/include/net/mptcp.h index cb580b0..8b5af68 100644 --- a/include/net/mptcp.h +++ b/include/net/mptcp.h @@ -105,7 +105,7 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size, bool mptcp_established_options(struct sock *sk, struct sk_buff *skb, unsigned int *size, unsigned int remaining, struct mptcp_out_options *opts); -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb); +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb); void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, struct mptcp_out_options *opts); @@ -227,9 +227,10 @@ static inline bool mptcp_established_options(struct sock *sk, return false; } -static inline void mptcp_incoming_options(struct sock *sk, +static inline bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) { + return true; } static inline void mptcp_skb_ext_move(struct sk_buff *to, diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 7d5e59f..4bacd7d 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -4247,6 +4247,9 @@ void tcp_reset(struct sock *sk, struct sk_buff *skb) { trace_tcp_receive_reset(sk); + /* mptcp can't tell us to ignore reset pkts, + * so just ignore the return value of mptcp_incoming_options(). + */ if (sk_is_mptcp(sk)) mptcp_incoming_options(sk, skb); @@ -4941,8 +4944,13 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb) bool fragstolen; int eaten; - if (sk_is_mptcp(sk)) - mptcp_incoming_options(sk, skb); + /* If a subflow has been reset, the packet should not continue + * to be processed, drop the packet. + */ + if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { + __kfree_skb(skb); + return; + } if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { __kfree_skb(skb); @@ -6523,8 +6531,11 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) case TCP_CLOSING: case TCP_LAST_ACK: if (!before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) { - if (sk_is_mptcp(sk)) - mptcp_incoming_options(sk, skb); + /* If a subflow has been reset, the packet should not + * continue to be processed, drop the packet. + */ + if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) + goto discard; break; } fallthrough; diff --git a/net/mptcp/options.c b/net/mptcp/options.c index b5850af..4452455 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -1035,7 +1035,8 @@ static bool add_addr_hmac_valid(struct mptcp_sock *msk, return hmac == mp_opt->ahmac; } -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) +/* Return false if a subflow has been reset, else return true */ +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) { struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); struct mptcp_sock *msk = mptcp_sk(subflow->conn); @@ -1053,12 +1054,16 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) __mptcp_check_push(subflow->conn, sk); __mptcp_data_acked(subflow->conn); mptcp_data_unlock(subflow->conn); - return; + return true; } mptcp_get_options(sk, skb, &mp_opt); + + /* The subflow can be in close state only if check_fully_established() + * just sent a reset. If so, tell the caller to ignore the current packet. + */ if (!check_fully_established(msk, sk, subflow, skb, &mp_opt)) - return; + return sk->sk_state != TCP_CLOSE; if (mp_opt.fastclose && msk->local_key == mp_opt.rcvr_key) { @@ -1100,7 +1105,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) } if (!mp_opt.dss) - return; + return true; /* we can't wait for recvmsg() to update the ack_seq, otherwise * monodirectional flows will stuck @@ -1119,12 +1124,12 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) schedule_work(&msk->work)) sock_hold(subflow->conn); - return; + return true; } mpext = skb_ext_add(skb, SKB_EXT_MPTCP); if (!mpext) - return; + return true; memset(mpext, 0, sizeof(*mpext)); @@ -1153,6 +1158,8 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) if (mpext->csum_reqd) mpext->csum = mp_opt.csum; } + + return true; } static void mptcp_set_rwin(const struct tcp_sock *tp) -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset 2021-06-26 9:16 ` [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106 @ 2021-06-28 8:35 ` Geliang Tang 2021-06-28 16:45 ` Matthieu Baerts 2021-06-29 2:55 ` Jianguo Wu 0 siblings, 2 replies; 10+ messages in thread From: Geliang Tang @ 2021-06-28 8:35 UTC (permalink / raw) To: Jianguo Wu; +Cc: mptcp, Paolo Abeni, Mat Martineau Hi Jianguo, Thank you for this new patch set! There are three checkpatch.pl warnings in this patch set, please fix them. And a minor comment below. --------------------------------------------------------------- 0001-mptcp-fix-warning-in-__skb_flow_dissect-when-do-syn-.patch --------------------------------------------------------------- WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #7: I did stress test with wrk(https://github.com/wg/wrk) and webfsd(https://github.com/ourway/webfsd) total: 0 errors, 1 warnings, 0 checks, 22 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. 0001-mptcp-fix-warning-in-__skb_flow_dissect-when-do-syn-.patch has style problems, please review. --------------------------------------------------------------- 0002-mptcp-remove-redundant-req-destruct-in-subflow_check.patch --------------------------------------------------------------- WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #14: tcp_v4_reqsk_destructor() twice, and may double free inet_rsk(req)->ireq_opt. total: 0 errors, 1 warnings, 0 checks, 11 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. 0002-mptcp-remove-redundant-req-destruct-in-subflow_check.patch has style problems, please review. --------------------------------------------------------------- 0005-selftests-mptcp-fix-case-multiple-subflows-limited-b.patch --------------------------------------------------------------- WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #7: After patch "mptcp: fix syncookie process if mptcp can not_accept new subflow", total: 0 errors, 1 warnings, 8 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. 0005-selftests-mptcp-fix-case-multiple-subflows-limited-b.patch has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. <wujianguo106@163.com> 于2021年6月26日周六 下午5:17写道: > > From: Jianguo Wu <wujianguo@chinatelecom.cn> > > If check_fully_established() causes a subflow reset, it should not > continue to process the packet in tcp_data_queue(). > Add a return value to mptcp_incoming_options(), and return false if a > subflow has been reset, else return true. Then drop the packet in > tcp_data_queue()/tcp_rcv_state_process() if mptcp_incoming_options() > return false. > > Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") > Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> > --- > include/net/mptcp.h | 5 +++-- > net/ipv4/tcp_input.c | 19 +++++++++++++++---- > net/mptcp/options.c | 19 +++++++++++++------ > 3 files changed, 31 insertions(+), 12 deletions(-) > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > index cb580b0..8b5af68 100644 > --- a/include/net/mptcp.h > +++ b/include/net/mptcp.h > @@ -105,7 +105,7 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size, > bool mptcp_established_options(struct sock *sk, struct sk_buff *skb, > unsigned int *size, unsigned int remaining, > struct mptcp_out_options *opts); > -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb); > +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb); > > void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, > struct mptcp_out_options *opts); > @@ -227,9 +227,10 @@ static inline bool mptcp_established_options(struct sock *sk, > return false; > } > > -static inline void mptcp_incoming_options(struct sock *sk, > +static inline bool mptcp_incoming_options(struct sock *sk, > struct sk_buff *skb) > { > + return true; > } > > static inline void mptcp_skb_ext_move(struct sk_buff *to, > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 7d5e59f..4bacd7d 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -4247,6 +4247,9 @@ void tcp_reset(struct sock *sk, struct sk_buff *skb) > { > trace_tcp_receive_reset(sk); > > + /* mptcp can't tell us to ignore reset pkts, > + * so just ignore the return value of mptcp_incoming_options(). > + */ > if (sk_is_mptcp(sk)) > mptcp_incoming_options(sk, skb); > > @@ -4941,8 +4944,13 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb) > bool fragstolen; > int eaten; > > - if (sk_is_mptcp(sk)) > - mptcp_incoming_options(sk, skb); > + /* If a subflow has been reset, the packet should not continue > + * to be processed, drop the packet. > + */ > + if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { > + __kfree_skb(skb); > + return; > + } > > if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { > __kfree_skb(skb); > @@ -6523,8 +6531,11 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) > case TCP_CLOSING: > case TCP_LAST_ACK: > if (!before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) { > - if (sk_is_mptcp(sk)) > - mptcp_incoming_options(sk, skb); > + /* If a subflow has been reset, the packet should not > + * continue to be processed, drop the packet. > + */ > + if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) > + goto discard; > break; > } > fallthrough; > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index b5850af..4452455 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -1035,7 +1035,8 @@ static bool add_addr_hmac_valid(struct mptcp_sock *msk, > return hmac == mp_opt->ahmac; > } > > -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) > +/* Return false if a subflow has been reset, else return true */ > +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) > { > struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); > struct mptcp_sock *msk = mptcp_sk(subflow->conn); > @@ -1053,12 +1054,16 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) > __mptcp_check_push(subflow->conn, sk); > __mptcp_data_acked(subflow->conn); > mptcp_data_unlock(subflow->conn); > - return; > + return true; > } > > mptcp_get_options(sk, skb, &mp_opt); > + > + /* The subflow can be in close state only if check_fully_established() > + * just sent a reset. If so, tell the caller to ignore the current packet. > + */ > if (!check_fully_established(msk, sk, subflow, skb, &mp_opt)) > - return; > + return sk->sk_state != TCP_CLOSE; > > if (mp_opt.fastclose && > msk->local_key == mp_opt.rcvr_key) { > @@ -1100,7 +1105,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) > } > > if (!mp_opt.dss) > - return; > + return true; > > /* we can't wait for recvmsg() to update the ack_seq, otherwise > * monodirectional flows will stuck > @@ -1119,12 +1124,12 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) > schedule_work(&msk->work)) > sock_hold(subflow->conn); > > - return; > + return true; > } > > mpext = skb_ext_add(skb, SKB_EXT_MPTCP); > if (!mpext) > - return; > + return true; > > memset(mpext, 0, sizeof(*mpext)); > > @@ -1153,6 +1158,8 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) > if (mpext->csum_reqd) > mpext->csum = mp_opt.csum; > } > + How about adding a label 'out:' here, and all above 'return true' can use 'goto out' instead. This is more like the kernel code style. WDYT? -Geliang > + return true; > } > > static void mptcp_set_rwin(const struct tcp_sock *tp) > -- > 1.8.3.1 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset 2021-06-28 8:35 ` Geliang Tang @ 2021-06-28 16:45 ` Matthieu Baerts 2021-06-29 2:54 ` Jianguo Wu 2021-06-29 2:55 ` Jianguo Wu 1 sibling, 1 reply; 10+ messages in thread From: Matthieu Baerts @ 2021-06-28 16:45 UTC (permalink / raw) To: Geliang Tang, Jianguo Wu; +Cc: mptcp, Paolo Abeni, Mat Martineau Hi Jianguo, Geliang, On 28/06/2021 10:35, Geliang Tang wrote: > Thank you for this new patch set! > > There are three checkpatch.pl warnings in this patch set, please fix them. > And a minor comment below. As pointed out by Paolo on IRC, these modifications are required but they are minor. Because it is already your v8 and your first major series, we did these small modifications for you ;) Note that we didn't change the "goto" just to return "true". Also, I didn't add any Acked-by or RvB tags because none were given but I can easily add them later if I get any but I guess Mat will send these patches later to netdev with his SoB. - 6bb69e9b2123: mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join - ee4bcc0a7a64: mptcp: remove redundant req destruct in subflow_check_req() - f0056198a479: mptcp: fix syncookie process if mptcp can not_accept new subflow - b65e3f889d44: mptcp: avoid processing packet if a subflow reset - fc66a740c7c7: selftests: mptcp: fix case multiple subflows limited by server - Results: 31aa314277b1..60851d13158d Builds and tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210628T164431 https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210628T164431 Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset 2021-06-28 16:45 ` Matthieu Baerts @ 2021-06-29 2:54 ` Jianguo Wu 0 siblings, 0 replies; 10+ messages in thread From: Jianguo Wu @ 2021-06-29 2:54 UTC (permalink / raw) To: Matthieu Baerts, Geliang Tang; +Cc: mptcp, Paolo Abeni, Mat Martineau On 2021/6/29 0:45, Matthieu Baerts wrote: > Hi Jianguo, Geliang, > > On 28/06/2021 10:35, Geliang Tang wrote: >> Thank you for this new patch set! >> >> There are three checkpatch.pl warnings in this patch set, please fix them. >> And a minor comment below. > > As pointed out by Paolo on IRC, these modifications are required but > they are minor. Because it is already your v8 and your first major > series, we did these small modifications for you ;) > > Note that we didn't change the "goto" just to return "true". > Also, I didn't add any Acked-by or RvB tags because none were given but > I can easily add them later if I get any but I guess Mat will send these > patches later to netdev with his SoB. > Hi Matt, Thanks for all your work :) > - 6bb69e9b2123: mptcp: fix warning in __skb_flow_dissect() when do syn > cookie for subflow join > > - ee4bcc0a7a64: mptcp: remove redundant req destruct in subflow_check_req() > > - f0056198a479: mptcp: fix syncookie process if mptcp can not_accept new > subflow > > - b65e3f889d44: mptcp: avoid processing packet if a subflow reset > > - fc66a740c7c7: selftests: mptcp: fix case multiple subflows limited by > server > > - Results: 31aa314277b1..60851d13158d > > Builds and tests are now in progress: > > https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210628T164431 > https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210628T164431 > > Cheers, > Matt > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset 2021-06-28 8:35 ` Geliang Tang 2021-06-28 16:45 ` Matthieu Baerts @ 2021-06-29 2:55 ` Jianguo Wu 1 sibling, 0 replies; 10+ messages in thread From: Jianguo Wu @ 2021-06-29 2:55 UTC (permalink / raw) To: Geliang Tang; +Cc: mptcp, Paolo Abeni, Mat Martineau Hi Geliang, On 2021/6/28 16:35, Geliang Tang wrote: > Hi Jianguo, > > Thank you for this new patch set! > > There are three checkpatch.pl warnings in this patch set, please fix them. > And a minor comment below. > Thanks for your checking and comment, I should do more checking next time. > --------------------------------------------------------------- > 0001-mptcp-fix-warning-in-__skb_flow_dissect-when-do-syn-.patch > --------------------------------------------------------------- > WARNING: Possible unwrapped commit description (prefer a maximum 75 > chars per line) > #7: > I did stress test with wrk(https://github.com/wg/wrk) and > webfsd(https://github.com/ourway/webfsd) > > total: 0 errors, 1 warnings, 0 checks, 22 lines checked > > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or --fix-inplace. > > 0001-mptcp-fix-warning-in-__skb_flow_dissect-when-do-syn-.patch has > style problems, please review. > --------------------------------------------------------------- > 0002-mptcp-remove-redundant-req-destruct-in-subflow_check.patch > --------------------------------------------------------------- > WARNING: Possible unwrapped commit description (prefer a maximum 75 > chars per line) > #14: > tcp_v4_reqsk_destructor() twice, and may double free inet_rsk(req)->ireq_opt. > > total: 0 errors, 1 warnings, 0 checks, 11 lines checked > > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or --fix-inplace. > > 0002-mptcp-remove-redundant-req-destruct-in-subflow_check.patch has > style problems, please review. > --------------------------------------------------------------- > 0005-selftests-mptcp-fix-case-multiple-subflows-limited-b.patch > --------------------------------------------------------------- > WARNING: Possible unwrapped commit description (prefer a maximum 75 > chars per line) > #7: > After patch "mptcp: fix syncookie process if mptcp can not_accept new subflow", > > total: 0 errors, 1 warnings, 8 lines checked > > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or --fix-inplace. > > 0005-selftests-mptcp-fix-case-multiple-subflows-limited-b.patch has > style problems, please review. > > NOTE: If any of the errors are false positives, please report > them to the maintainer, see CHECKPATCH in MAINTAINERS. > > > <wujianguo106@163.com> 于2021年6月26日周六 下午5:17写道: >> >> From: Jianguo Wu <wujianguo@chinatelecom.cn> >> >> If check_fully_established() causes a subflow reset, it should not >> continue to process the packet in tcp_data_queue(). >> Add a return value to mptcp_incoming_options(), and return false if a >> subflow has been reset, else return true. Then drop the packet in >> tcp_data_queue()/tcp_rcv_state_process() if mptcp_incoming_options() >> return false. >> >> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") >> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> >> --- >> include/net/mptcp.h | 5 +++-- >> net/ipv4/tcp_input.c | 19 +++++++++++++++---- >> net/mptcp/options.c | 19 +++++++++++++------ >> 3 files changed, 31 insertions(+), 12 deletions(-) >> >> diff --git a/include/net/mptcp.h b/include/net/mptcp.h >> index cb580b0..8b5af68 100644 >> --- a/include/net/mptcp.h >> +++ b/include/net/mptcp.h >> @@ -105,7 +105,7 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size, >> bool mptcp_established_options(struct sock *sk, struct sk_buff *skb, >> unsigned int *size, unsigned int remaining, >> struct mptcp_out_options *opts); >> -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb); >> +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb); >> >> void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp, >> struct mptcp_out_options *opts); >> @@ -227,9 +227,10 @@ static inline bool mptcp_established_options(struct sock *sk, >> return false; >> } >> >> -static inline void mptcp_incoming_options(struct sock *sk, >> +static inline bool mptcp_incoming_options(struct sock *sk, >> struct sk_buff *skb) >> { >> + return true; >> } >> >> static inline void mptcp_skb_ext_move(struct sk_buff *to, >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c >> index 7d5e59f..4bacd7d 100644 >> --- a/net/ipv4/tcp_input.c >> +++ b/net/ipv4/tcp_input.c >> @@ -4247,6 +4247,9 @@ void tcp_reset(struct sock *sk, struct sk_buff *skb) >> { >> trace_tcp_receive_reset(sk); >> >> + /* mptcp can't tell us to ignore reset pkts, >> + * so just ignore the return value of mptcp_incoming_options(). >> + */ >> if (sk_is_mptcp(sk)) >> mptcp_incoming_options(sk, skb); >> >> @@ -4941,8 +4944,13 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb) >> bool fragstolen; >> int eaten; >> >> - if (sk_is_mptcp(sk)) >> - mptcp_incoming_options(sk, skb); >> + /* If a subflow has been reset, the packet should not continue >> + * to be processed, drop the packet. >> + */ >> + if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { >> + __kfree_skb(skb); >> + return; >> + } >> >> if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { >> __kfree_skb(skb); >> @@ -6523,8 +6531,11 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) >> case TCP_CLOSING: >> case TCP_LAST_ACK: >> if (!before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) { >> - if (sk_is_mptcp(sk)) >> - mptcp_incoming_options(sk, skb); >> + /* If a subflow has been reset, the packet should not >> + * continue to be processed, drop the packet. >> + */ >> + if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) >> + goto discard; >> break; >> } >> fallthrough; >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >> index b5850af..4452455 100644 >> --- a/net/mptcp/options.c >> +++ b/net/mptcp/options.c >> @@ -1035,7 +1035,8 @@ static bool add_addr_hmac_valid(struct mptcp_sock *msk, >> return hmac == mp_opt->ahmac; >> } >> >> -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) >> +/* Return false if a subflow has been reset, else return true */ >> +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) >> { >> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); >> struct mptcp_sock *msk = mptcp_sk(subflow->conn); >> @@ -1053,12 +1054,16 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) >> __mptcp_check_push(subflow->conn, sk); >> __mptcp_data_acked(subflow->conn); >> mptcp_data_unlock(subflow->conn); >> - return; >> + return true; >> } >> >> mptcp_get_options(sk, skb, &mp_opt); >> + >> + /* The subflow can be in close state only if check_fully_established() >> + * just sent a reset. If so, tell the caller to ignore the current packet. >> + */ >> if (!check_fully_established(msk, sk, subflow, skb, &mp_opt)) >> - return; >> + return sk->sk_state != TCP_CLOSE; >> >> if (mp_opt.fastclose && >> msk->local_key == mp_opt.rcvr_key) { >> @@ -1100,7 +1105,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) >> } >> >> if (!mp_opt.dss) >> - return; >> + return true; >> >> /* we can't wait for recvmsg() to update the ack_seq, otherwise >> * monodirectional flows will stuck >> @@ -1119,12 +1124,12 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) >> schedule_work(&msk->work)) >> sock_hold(subflow->conn); >> >> - return; >> + return true; >> } >> >> mpext = skb_ext_add(skb, SKB_EXT_MPTCP); >> if (!mpext) >> - return; >> + return true; >> >> memset(mpext, 0, sizeof(*mpext)); >> >> @@ -1153,6 +1158,8 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) >> if (mpext->csum_reqd) >> mpext->csum = mp_opt.csum; >> } >> + > > How about adding a label 'out:' here, and all above 'return true' can use > 'goto out' instead. This is more like the kernel code style. WDYT? > > -Geliang > >> + return true; >> } >> >> static void mptcp_set_rwin(const struct tcp_sock *tp) >> -- >> 1.8.3.1 >> >> >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH mptcp-net v8 5/5] selftests: mptcp: fix case multiple subflows limited by server 2021-06-26 9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106 ` (3 preceding siblings ...) 2021-06-26 9:16 ` [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106 @ 2021-06-26 9:16 ` wujianguo106 4 siblings, 0 replies; 10+ messages in thread From: wujianguo106 @ 2021-06-26 9:16 UTC (permalink / raw) To: mptcp; +Cc: pabeni, mathew.j.martineau From: Jianguo Wu <wujianguo@chinatelecom.cn> After patch "mptcp: fix syncookie process if mptcp can not_accept new subflow", If subflow is limited, MP_JOIN SYN is dropped, and no SYN/ACK will be replied. So in case "multiple subflows limited by server", the expected SYN/ACK number should be 1. Reported-by: kernel test robot <oliver.sang@intel.com> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> --- tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh index 9a191c1..f02f4de 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh @@ -1409,7 +1409,7 @@ syncookies_tests() ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 flags subflow run_tests $ns1 $ns2 10.0.1.1 - chk_join_nr "subflows limited by server w cookies" 2 2 1 + chk_join_nr "subflows limited by server w cookies" 2 1 1 # test signal address with cookies reset_with_cookies -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-06-29 3:14 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-26 9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 2/5] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106 2021-06-26 9:16 ` [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106 2021-06-28 8:35 ` Geliang Tang 2021-06-28 16:45 ` Matthieu Baerts 2021-06-29 2:54 ` Jianguo Wu 2021-06-29 2:55 ` Jianguo Wu 2021-06-26 9:16 ` [PATCH mptcp-net v8 5/5] selftests: mptcp: fix case multiple subflows limited by server wujianguo106
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).