* [PATCH v5 0/4] Fix some mptcp syncookie process bugs
@ 2021-06-16 10:49 wujianguo106
2021-06-16 10:49 ` [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
` (4 more replies)
0 siblings, 5 replies; 24+ messages in thread
From: wujianguo106 @ 2021-06-16 10:49 UTC (permalink / raw)
To: mptcp; +Cc: pabeni, fw
From: Jianguo Wu <wujianguo@chinatelecom.cn>
v4->v5:
patch1: no changes
patch2: no changes
patch3: no changes
patch4: add comment
v3->v4:
patch1: using seq and sport/dport for hashing, and ignore network headers altogether,
as suggest by Florian
patch2: no changes
patch3: no changes
patch4: no changes
v2->v3:
patch1: directly use inet6_ehashfn() for IPv6
patch2: no changes
patch3: no changes
patch4: add Fixes tag.
v1->v2:
patch1: handle ipv6 sockets/addresses,
always use 4-tuple drived hash and never look at skb->hash
patch2: no changes
patch3: split into 2 patches.
patch4: new added.
Jianguo Wu (4):
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
net/mptcp/options.c | 6 ++++++
net/mptcp/subflow.c | 11 +++--------
net/mptcp/syncookies.c | 16 +++++++++++++++-
3 files changed, 24 insertions(+), 9 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join 2021-06-16 10:49 [PATCH v5 0/4] Fix some mptcp syncookie process bugs wujianguo106 @ 2021-06-16 10:49 ` wujianguo106 2021-06-18 22:40 ` Mat Martineau 2021-06-16 10:49 ` [PATCH v5 2/4] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106 ` (3 subsequent siblings) 4 siblings, 1 reply; 24+ messages in thread From: wujianguo106 @ 2021-06-16 10:49 UTC (permalink / raw) To: mptcp; +Cc: pabeni, fw From: Jianguo Wu <wujianguo@chinatelecom.cn> I got the following warning message while doing the test: [ 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 abe0fd099746..37127781aee9 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] 24+ messages in thread
* Re: [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join 2021-06-16 10:49 ` [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106 @ 2021-06-18 22:40 ` Mat Martineau 2021-06-21 6:14 ` Jianguo Wu 0 siblings, 1 reply; 24+ messages in thread From: Mat Martineau @ 2021-06-18 22:40 UTC (permalink / raw) To: wujianguo106; +Cc: mptcp, pabeni, fw On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > From: Jianguo Wu <wujianguo@chinatelecom.cn> > > I got the following warning message while doing the test: Hi Jianguo, thanks for your patch set and revisions. Could you explain some more about which test produced the following result? Was it one of the self tests? If this is triggered by test code that is not upstream yet, it would help to add a selftest that shows if this bug is present or fixed. A packetdrill test is also an option, if that is a better way to reproduce the error. Code below looks fine, but would like to understand the test scenario better! -Mat > > [ 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 abe0fd099746..37127781aee9 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 > > > -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join 2021-06-18 22:40 ` Mat Martineau @ 2021-06-21 6:14 ` Jianguo Wu 2021-06-21 10:09 ` Jianguo Wu 0 siblings, 1 reply; 24+ messages in thread From: Jianguo Wu @ 2021-06-21 6:14 UTC (permalink / raw) To: Mat Martineau; +Cc: mptcp, pabeni, fw Hi Mat, On 2021/6/19 6:40, Mat Martineau wrote: > > On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > >> From: Jianguo Wu <wujianguo@chinatelecom.cn> >> >> I got the following warning message while doing the test: > > Hi Jianguo, thanks for your patch set and revisions. > > Could you explain some more about which test produced the following result? Was it one of the self tests? > > If this is triggered by test code that is not upstream yet, it would help to add a selftest that shows if this bug is present or fixed. A packetdrill test is also an option, if that is a better way to reproduce the error. > I tested 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/ I will try self tests. > Code below looks fine, but would like to understand the test scenario better! > > -Mat > >> >> [ 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 abe0fd099746..37127781aee9 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 >> >> >> > > -- > Mat Martineau > Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join 2021-06-21 6:14 ` Jianguo Wu @ 2021-06-21 10:09 ` Jianguo Wu 2021-06-22 23:38 ` Mat Martineau 0 siblings, 1 reply; 24+ messages in thread From: Jianguo Wu @ 2021-06-21 10:09 UTC (permalink / raw) To: Mat Martineau; +Cc: mptcp, pabeni, fw On 2021/6/21 14:14, Jianguo Wu wrote: > Hi Mat, > > On 2021/6/19 6:40, Mat Martineau wrote: >> >> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >> >>> From: Jianguo Wu <wujianguo@chinatelecom.cn> >>> >>> I got the following warning message while doing the test: >> >> Hi Jianguo, thanks for your patch set and revisions. >> >> Could you explain some more about which test produced the following result? Was it one of the self tests? >> >> If this is triggered by test code that is not upstream yet, it would help to add a selftest that shows if this bug is present or fixed. A packetdrill test is also an option, if that is a better way to reproduce the error. >> > > I tested 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/ > > I will try self tests. > Hi Mat, It can not be reproduced with selftests(./mptcp_join.sh -k), because skb->l4_hash is 1 in this scenario, and direct using skb->hash, so __skb_flow_dissect() isn't called. Thanks, Jianguo >> Code below looks fine, but would like to understand the test scenario better! >> >> -Mat >> >>> >>> [ 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 abe0fd099746..37127781aee9 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 >>> >>> >>> >> >> -- >> Mat Martineau >> Intel > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join 2021-06-21 10:09 ` Jianguo Wu @ 2021-06-22 23:38 ` Mat Martineau 0 siblings, 0 replies; 24+ messages in thread From: Mat Martineau @ 2021-06-22 23:38 UTC (permalink / raw) To: Jianguo Wu; +Cc: mptcp, pabeni, fw On Mon, 21 Jun 2021, Jianguo Wu wrote: > > > On 2021/6/21 14:14, Jianguo Wu wrote: >> Hi Mat, >> >> On 2021/6/19 6:40, Mat Martineau wrote: >>> >>> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >>> >>>> From: Jianguo Wu <wujianguo@chinatelecom.cn> >>>> >>>> I got the following warning message while doing the test: >>> >>> Hi Jianguo, thanks for your patch set and revisions. >>> >>> Could you explain some more about which test produced the following result? Was it one of the self tests? >>> >>> If this is triggered by test code that is not upstream yet, it would help to add a selftest that shows if this bug is present or fixed. A packetdrill test is also an option, if that is a better way to reproduce the error. >>> >> >> I tested 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/ >> >> I will try self tests. >> > > Hi Mat, > > It can not be reproduced with selftests(./mptcp_join.sh -k), because skb->l4_hash is 1 in this scenario, > and direct using skb->hash, so __skb_flow_dissect() isn't called. > Thanks for the information! We hadn't seen the failures with the self tests so it's good to know that wrk and webfsd were useful for stress testing. -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 2/4] mptcp: remove redundant req destruct in subflow_check_req() 2021-06-16 10:49 [PATCH v5 0/4] Fix some mptcp syncookie process bugs wujianguo106 2021-06-16 10:49 ` [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106 @ 2021-06-16 10:49 ` wujianguo106 2021-06-16 10:49 ` [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106 ` (2 subsequent siblings) 4 siblings, 0 replies; 24+ messages in thread From: wujianguo106 @ 2021-06-16 10:49 UTC (permalink / raw) To: mptcp; +Cc: pabeni, fw 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 6b1cd4257edf..75ed530706c0 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -213,11 +213,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] 24+ messages in thread
* [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow 2021-06-16 10:49 [PATCH v5 0/4] Fix some mptcp syncookie process bugs wujianguo106 2021-06-16 10:49 ` [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106 2021-06-16 10:49 ` [PATCH v5 2/4] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106 @ 2021-06-16 10:49 ` wujianguo106 2021-06-18 23:19 ` Mat Martineau 2021-06-16 10:49 ` [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset wujianguo106 2021-06-19 0:26 ` [PATCH v5 0/4] Fix some mptcp syncookie process bugs Mat Martineau 4 siblings, 1 reply; 24+ messages in thread From: wujianguo106 @ 2021-06-16 10:49 UTC (permalink / raw) To: mptcp; +Cc: pabeni, fw From: Jianguo Wu <wujianguo@chinatelecom.cn> Lots of "TCP: tcp_fin: Impossible, sk->sk_state=7" in client side when doing stress testing. 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 75ed530706c0..6d98e19a20aa 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -224,6 +224,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, @@ -263,9 +265,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] 24+ messages in thread
* Re: [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow 2021-06-16 10:49 ` [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106 @ 2021-06-18 23:19 ` Mat Martineau 2021-06-21 6:24 ` Jianguo Wu 0 siblings, 1 reply; 24+ messages in thread From: Mat Martineau @ 2021-06-18 23:19 UTC (permalink / raw) To: wujianguo106; +Cc: mptcp, pabeni, fw On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > From: Jianguo Wu <wujianguo@chinatelecom.cn> > > Lots of "TCP: tcp_fin: Impossible, sk->sk_state=7" in client side > when doing stress testing. Hi Jianguo - Like patch 1, do the existing syncookie selftests trigger this sometimes? Or are there selftests that could be added to this series, or packetdrill tests, to do such syncookie stress tests? Thanks, Mat > > 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 75ed530706c0..6d98e19a20aa 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -224,6 +224,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, > @@ -263,9 +265,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 > > > -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow 2021-06-18 23:19 ` Mat Martineau @ 2021-06-21 6:24 ` Jianguo Wu 2021-06-24 2:08 ` Jianguo Wu 0 siblings, 1 reply; 24+ messages in thread From: Jianguo Wu @ 2021-06-21 6:24 UTC (permalink / raw) To: Mat Martineau; +Cc: mptcp, pabeni, fw Hi Mat, On 2021/6/19 7:19, Mat Martineau wrote: > On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > >> From: Jianguo Wu <wujianguo@chinatelecom.cn> >> >> Lots of "TCP: tcp_fin: Impossible, sk->sk_state=7" in client side >> when doing stress testing. > > Hi Jianguo - > > Like patch 1, do the existing syncookie selftests trigger this sometimes? > > Or are there selftests that could be added to this series, or packetdrill tests, to do such syncookie stress tests? > This is triggered by the same test in patch 1, I will try selftests too, or add selftest if necessary. > Thanks, > > Mat > > >> >> 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 75ed530706c0..6d98e19a20aa 100644 >> --- a/net/mptcp/subflow.c >> +++ b/net/mptcp/subflow.c >> @@ -224,6 +224,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, >> @@ -263,9 +265,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 >> >> >> > > -- > Mat Martineau > Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow 2021-06-21 6:24 ` Jianguo Wu @ 2021-06-24 2:08 ` Jianguo Wu 2021-06-24 22:36 ` Mat Martineau 0 siblings, 1 reply; 24+ messages in thread From: Jianguo Wu @ 2021-06-24 2:08 UTC (permalink / raw) To: Mat Martineau; +Cc: mptcp, pabeni, fw On 2021/6/21 14:24, Jianguo Wu wrote: > > Hi Mat, > > On 2021/6/19 7:19, Mat Martineau wrote: >> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >> >>> From: Jianguo Wu <wujianguo@chinatelecom.cn> >>> >>> Lots of "TCP: tcp_fin: Impossible, sk->sk_state=7" in client side >>> when doing stress testing. >> >> Hi Jianguo - >> >> Like patch 1, do the existing syncookie selftests trigger this sometimes? >> >> Or are there selftests that could be added to this series, or packetdrill tests, to do such syncookie stress tests? >> > > This is triggered by the same test in patch 1, > I will try selftests too, or add selftest if necessary. > >> Thanks, >> >> Mat >> >> >>> >>> 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. >>> Hi Mat, As MP_JOIN SYN is dropped, no SYN/ACK will be replied, So test case "subflows limited by server w cookies" in mptcp_join.sh should be updated, like this: diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh index 523c7797f30a..37b7da3cd5ca 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh @@ -1398,7 +1398,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 Is this OK? Jianguo, Thanks >>> 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 75ed530706c0..6d98e19a20aa 100644 >>> --- a/net/mptcp/subflow.c >>> +++ b/net/mptcp/subflow.c >>> @@ -224,6 +224,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, >>> @@ -263,9 +265,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 >>> >>> >>> >> >> -- >> Mat Martineau >> Intel > ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow 2021-06-24 2:08 ` Jianguo Wu @ 2021-06-24 22:36 ` Mat Martineau 0 siblings, 0 replies; 24+ messages in thread From: Mat Martineau @ 2021-06-24 22:36 UTC (permalink / raw) To: Jianguo Wu; +Cc: mptcp, pabeni, fw [-- Attachment #1: Type: text/plain, Size: 5003 bytes --] On Thu, 24 Jun 2021, Jianguo Wu wrote: > > > On 2021/6/21 14:24, Jianguo Wu wrote: >> >> Hi Mat, >> >> On 2021/6/19 7:19, Mat Martineau wrote: >>> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >>> >>>> From: Jianguo Wu <wujianguo@chinatelecom.cn> >>>> >>>> Lots of "TCP: tcp_fin: Impossible, sk->sk_state=7" in client side >>>> when doing stress testing. >>> >>> Hi Jianguo - >>> >>> Like patch 1, do the existing syncookie selftests trigger this sometimes? >>> >>> Or are there selftests that could be added to this series, or packetdrill tests, to do such syncookie stress tests? >>> >> >> This is triggered by the same test in patch 1, >> I will try selftests too, or add selftest if necessary. >> >>> Thanks, >>> >>> Mat >>> >>> >>>> >>>> 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. >>>> > > Hi Mat, > > As MP_JOIN SYN is dropped, no SYN/ACK will be replied, So test case "subflows limited by server w cookies" in mptcp_join.sh should be updated, > like this: > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh > index 523c7797f30a..37b7da3cd5ca 100755 > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh > @@ -1398,7 +1398,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 > > Is this OK? > Hi Jianguo - Yes, you should include a selftest patch in the series to make the change, since the expected behavior is changing. -Mat > >>>> 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 75ed530706c0..6d98e19a20aa 100644 >>>> --- a/net/mptcp/subflow.c >>>> +++ b/net/mptcp/subflow.c >>>> @@ -224,6 +224,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, >>>> @@ -263,9 +265,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; >>>> } >>>> >>>> -- -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset 2021-06-16 10:49 [PATCH v5 0/4] Fix some mptcp syncookie process bugs wujianguo106 ` (2 preceding siblings ...) 2021-06-16 10:49 ` [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106 @ 2021-06-16 10:49 ` wujianguo106 2021-06-19 0:19 ` Mat Martineau 2021-06-19 0:26 ` [PATCH v5 0/4] Fix some mptcp syncookie process bugs Mat Martineau 4 siblings, 1 reply; 24+ messages in thread From: wujianguo106 @ 2021-06-16 10:49 UTC (permalink / raw) To: mptcp; +Cc: pabeni, fw 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(). setting: TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; so that the following check will drop the pkt in tcp_data_queue(): if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { __kfree_skb(skb); return; } Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> --- net/mptcp/options.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/net/mptcp/options.c b/net/mptcp/options.c index 1aec01686c1a..be435c5421cd 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, return true; reset: + /* If a subflow is reset, the packet should not continue to be + * processed in tcp_data_queue(), so setting: end_seq = seq, + * then tcp_data_queue() will drop the packet. + */ + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; + mptcp_subflow_reset(ssk); return false; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset 2021-06-16 10:49 ` [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset wujianguo106 @ 2021-06-19 0:19 ` Mat Martineau 2021-06-21 6:35 ` Jianguo Wu 0 siblings, 1 reply; 24+ messages in thread From: Mat Martineau @ 2021-06-19 0:19 UTC (permalink / raw) To: wujianguo106; +Cc: mptcp, pabeni, fw On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > 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(). > > setting: > TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; > > so that the following check will drop the pkt in > tcp_data_queue(): > if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { > __kfree_skb(skb); > return; > } > > Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") > Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> > --- > net/mptcp/options.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index 1aec01686c1a..be435c5421cd 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, > return true; > > reset: > + /* If a subflow is reset, the packet should not continue to be > + * processed in tcp_data_queue(), so setting: end_seq = seq, > + * then tcp_data_queue() will drop the packet. > + */ > + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; > + This does have the desired effect when mptcp_incoming_options() is called from tcp_data_queue(), but mptcp_incoming_options() is also called from tcp_reset() and tcp_rcv_state_process(). The other callers appear to tolerate the sequence number modification. I think it would be clearer to either add a return value or output parameter to mptcp_incoming_options() to explicitly tell the caller that a reset has been sent and tcp_done() called. Then it would be clearer in tcp_data_queue() that the packet is being discarded due to mptcp header content. (It also looks like it unexpected behavior may be possible if we get a strange TCP_RST + MP_JOIN packet when not fully established, but that's unrelated to this patch. Maybe I'll try to create a packetdrill test to see what happens) > mptcp_subflow_reset(ssk); > return false; > } > -- > 1.8.3.1 -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset 2021-06-19 0:19 ` Mat Martineau @ 2021-06-21 6:35 ` Jianguo Wu 2021-06-23 0:00 ` Mat Martineau 0 siblings, 1 reply; 24+ messages in thread From: Jianguo Wu @ 2021-06-21 6:35 UTC (permalink / raw) To: Mat Martineau; +Cc: mptcp, pabeni, fw Hi Mat, On 2021/6/19 8:19, Mat Martineau wrote: > On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > >> 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(). >> >> setting: >> TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >> >> so that the following check will drop the pkt in >> tcp_data_queue(): >> if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { >> __kfree_skb(skb); >> return; >> } >> >> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") >> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> >> --- >> net/mptcp/options.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >> index 1aec01686c1a..be435c5421cd 100644 >> --- a/net/mptcp/options.c >> +++ b/net/mptcp/options.c >> @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, >> return true; >> >> reset: >> + /* If a subflow is reset, the packet should not continue to be >> + * processed in tcp_data_queue(), so setting: end_seq = seq, >> + * then tcp_data_queue() will drop the packet. >> + */ >> + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >> + > > This does have the desired effect when mptcp_incoming_options() is called from tcp_data_queue(), but mptcp_incoming_options() is also called from tcp_reset() and tcp_rcv_state_process(). The other callers appear to tolerate the sequence number modification. > > I think it would be clearer to either add a return value or output parameter to mptcp_incoming_options() to explicitly tell the caller that a reset has been sent and tcp_done() called. Then it would be clearer in tcp_data_queue() that the packet is being discarded due to mptcp header content. > If a reset has been sent and tcp_done() called in check_fully_established(), the sk_state will be TCP_CLOSE, how about just do (sk_state == TCP_CLOSE) check in tcp_data_queue() as it did in the V1 of this patch? > (It also looks like it unexpected behavior may be possible if we get a strange TCP_RST + MP_JOIN packet when not fully established, but that's unrelated to this patch. Maybe I'll try to create a packetdrill test to see what happens) > >> mptcp_subflow_reset(ssk); >> return false; >> } >> -- >> 1.8.3.1 > > -- > Mat Martineau > Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset 2021-06-21 6:35 ` Jianguo Wu @ 2021-06-23 0:00 ` Mat Martineau 2021-06-23 9:48 ` Paolo Abeni 0 siblings, 1 reply; 24+ messages in thread From: Mat Martineau @ 2021-06-23 0:00 UTC (permalink / raw) To: Jianguo Wu; +Cc: mptcp, pabeni, fw [-- Attachment #1: Type: text/plain, Size: 3185 bytes --] On Mon, 21 Jun 2021, Jianguo Wu wrote: > Hi Mat, > > On 2021/6/19 8:19, Mat Martineau wrote: >> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >> >>> 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(). >>> >>> setting: >>> TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>> >>> so that the following check will drop the pkt in >>> tcp_data_queue(): >>> if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { >>> __kfree_skb(skb); >>> return; >>> } >>> >>> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") >>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> >>> --- >>> net/mptcp/options.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>> index 1aec01686c1a..be435c5421cd 100644 >>> --- a/net/mptcp/options.c >>> +++ b/net/mptcp/options.c >>> @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, >>> return true; >>> >>> reset: >>> + /* If a subflow is reset, the packet should not continue to be >>> + * processed in tcp_data_queue(), so setting: end_seq = seq, >>> + * then tcp_data_queue() will drop the packet. >>> + */ >>> + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>> + >> >> This does have the desired effect when mptcp_incoming_options() is >> called from tcp_data_queue(), but mptcp_incoming_options() is also >> called from tcp_reset() and tcp_rcv_state_process(). The other callers >> appear to tolerate the sequence number modification. >> >> I think it would be clearer to either add a return value or output >> parameter to mptcp_incoming_options() to explicitly tell the caller >> that a reset has been sent and tcp_done() called. Then it would be >> clearer in tcp_data_queue() that the packet is being discarded due to >> mptcp header content. >> > > If a reset has been sent and tcp_done() called in > check_fully_established(), the sk_state will be TCP_CLOSE, how about > just do (sk_state == TCP_CLOSE) check in tcp_data_queue() as it did in > the V1 of this patch? Oh, I see now that Paolo suggested the the end_seq assignment in order to only modify MPTCP code. I still think it's better to make it clear that we're discarding a packet due to the mptcp headers - using the existing sequence check (intended to detect acks) in tcp_data_queue() seems sneaky to me. Something like if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { __kfree_skb(skb); return; } seems both compact and clear. Does that seem ok Paolo? The optimizer would probably share instructions with the following "end_seq == seq" condition for the kfree+return path anyway. > >> (It also looks like it unexpected behavior may be possible if we get a >> strange TCP_RST + MP_JOIN packet when not fully established, but that's >> unrelated to this patch. Maybe I'll try to create a packetdrill test to >> see what happens) >> >>> mptcp_subflow_reset(ssk); >>> return false; >>> } -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset 2021-06-23 0:00 ` Mat Martineau @ 2021-06-23 9:48 ` Paolo Abeni 2021-06-24 1:57 ` Jianguo Wu 0 siblings, 1 reply; 24+ messages in thread From: Paolo Abeni @ 2021-06-23 9:48 UTC (permalink / raw) To: Mat Martineau, Jianguo Wu; +Cc: mptcp, fw On Tue, 2021-06-22 at 17:00 -0700, Mat Martineau wrote: > On Mon, 21 Jun 2021, Jianguo Wu wrote: > > > Hi Mat, > > > > On 2021/6/19 8:19, Mat Martineau wrote: > > > On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > > > > > > > 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(). > > > > > > > > setting: > > > > TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; > > > > > > > > so that the following check will drop the pkt in > > > > tcp_data_queue(): > > > > if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { > > > > __kfree_skb(skb); > > > > return; > > > > } > > > > > > > > Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") > > > > Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> > > > > --- > > > > net/mptcp/options.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > > > index 1aec01686c1a..be435c5421cd 100644 > > > > --- a/net/mptcp/options.c > > > > +++ b/net/mptcp/options.c > > > > @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, > > > > return true; > > > > > > > > reset: > > > > + /* If a subflow is reset, the packet should not continue to be > > > > + * processed in tcp_data_queue(), so setting: end_seq = seq, > > > > + * then tcp_data_queue() will drop the packet. > > > > + */ > > > > + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; > > > > + > > > > > > This does have the desired effect when mptcp_incoming_options() is > > > called from tcp_data_queue(), but mptcp_incoming_options() is also > > > called from tcp_reset() and tcp_rcv_state_process(). The other callers > > > appear to tolerate the sequence number modification. > > > > > > I think it would be clearer to either add a return value or output > > > parameter to mptcp_incoming_options() to explicitly tell the caller > > > that a reset has been sent and tcp_done() called. Then it would be > > > clearer in tcp_data_queue() that the packet is being discarded due to > > > mptcp header content. > > > > > > > If a reset has been sent and tcp_done() called in > > check_fully_established(), the sk_state will be TCP_CLOSE, how about > > just do (sk_state == TCP_CLOSE) check in tcp_data_queue() as it did in > > the V1 of this patch? > > Oh, I see now that Paolo suggested the the end_seq assignment in order to > only modify MPTCP code. > > I still think it's better to make it clear that we're discarding a packet > due to the mptcp headers - using the existing sequence check (intended to > detect acks) in tcp_data_queue() seems sneaky to me. > > Something like > > if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { > __kfree_skb(skb); > return; > } > > seems both compact and clear. Does that seem ok Paolo? Uhmmm... we need to touch every mptcp_incoming_options() call site, and in tcp_reset() the above chunk looks a bit strange to me. Probably we could just ignore the mptcp_incoming_options() return value there. Otherwise no big objections - not sure about upstream ;) Cheers, Paolo ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset 2021-06-23 9:48 ` Paolo Abeni @ 2021-06-24 1:57 ` Jianguo Wu 2021-06-24 10:02 ` Paolo Abeni 0 siblings, 1 reply; 24+ messages in thread From: Jianguo Wu @ 2021-06-24 1:57 UTC (permalink / raw) To: Paolo Abeni, Mat Martineau; +Cc: mptcp, fw On 2021/6/23 17:48, Paolo Abeni wrote: > On Tue, 2021-06-22 at 17:00 -0700, Mat Martineau wrote: >> On Mon, 21 Jun 2021, Jianguo Wu wrote: >> >>> Hi Mat, >>> >>> On 2021/6/19 8:19, Mat Martineau wrote: >>>> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >>>> >>>>> 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(). >>>>> >>>>> setting: >>>>> TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>>>> >>>>> so that the following check will drop the pkt in >>>>> tcp_data_queue(): >>>>> if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { >>>>> __kfree_skb(skb); >>>>> return; >>>>> } >>>>> >>>>> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") >>>>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> >>>>> --- >>>>> net/mptcp/options.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>>>> index 1aec01686c1a..be435c5421cd 100644 >>>>> --- a/net/mptcp/options.c >>>>> +++ b/net/mptcp/options.c >>>>> @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, >>>>> return true; >>>>> >>>>> reset: >>>>> + /* If a subflow is reset, the packet should not continue to be >>>>> + * processed in tcp_data_queue(), so setting: end_seq = seq, >>>>> + * then tcp_data_queue() will drop the packet. >>>>> + */ >>>>> + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>>>> + >>>> >>>> This does have the desired effect when mptcp_incoming_options() is >>>> called from tcp_data_queue(), but mptcp_incoming_options() is also >>>> called from tcp_reset() and tcp_rcv_state_process(). The other callers >>>> appear to tolerate the sequence number modification. >>>> >>>> I think it would be clearer to either add a return value or output >>>> parameter to mptcp_incoming_options() to explicitly tell the caller >>>> that a reset has been sent and tcp_done() called. Then it would be >>>> clearer in tcp_data_queue() that the packet is being discarded due to >>>> mptcp header content. >>>> >>> >>> If a reset has been sent and tcp_done() called in >>> check_fully_established(), the sk_state will be TCP_CLOSE, how about >>> just do (sk_state == TCP_CLOSE) check in tcp_data_queue() as it did in >>> the V1 of this patch? >> >> Oh, I see now that Paolo suggested the the end_seq assignment in order to >> only modify MPTCP code. >> >> I still think it's better to make it clear that we're discarding a packet >> due to the mptcp headers - using the existing sequence check (intended to >> detect acks) in tcp_data_queue() seems sneaky to me. >> >> Something like >> >> if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { >> __kfree_skb(skb); >> return; >> } >> >> seems both compact and clear. Does that seem ok Paolo? > > Uhmmm... we need to touch every mptcp_incoming_options() call site, and > in tcp_reset() the above chunk looks a bit strange to me. Probably we > could just ignore the mptcp_incoming_options() return value there. > > Otherwise no big objections - not sure about upstream ;) > Hi Mat and Paolo, If you both agree, I will send a new version that mptcp_incoming_options() add a return value, and only check return value in tcp_data_queue(), ignore the return value in other call site. > Cheers, > > Paolo > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset 2021-06-24 1:57 ` Jianguo Wu @ 2021-06-24 10:02 ` Paolo Abeni 2021-06-24 22:38 ` Mat Martineau 2021-06-25 0:51 ` Jianguo Wu 0 siblings, 2 replies; 24+ messages in thread From: Paolo Abeni @ 2021-06-24 10:02 UTC (permalink / raw) To: Jianguo Wu, Mat Martineau; +Cc: mptcp, fw On Thu, 2021-06-24 at 09:57 +0800, Jianguo Wu wrote: > > On 2021/6/23 17:48, Paolo Abeni wrote: > > On Tue, 2021-06-22 at 17:00 -0700, Mat Martineau wrote: > > > On Mon, 21 Jun 2021, Jianguo Wu wrote: > > > > > > > Hi Mat, > > > > > > > > On 2021/6/19 8:19, Mat Martineau wrote: > > > > > On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > > > > > > > > > > > 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(). > > > > > > > > > > > > setting: > > > > > > TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; > > > > > > > > > > > > so that the following check will drop the pkt in > > > > > > tcp_data_queue(): > > > > > > if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { > > > > > > __kfree_skb(skb); > > > > > > return; > > > > > > } > > > > > > > > > > > > Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") > > > > > > Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> > > > > > > --- > > > > > > net/mptcp/options.c | 6 ++++++ > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > > > > > index 1aec01686c1a..be435c5421cd 100644 > > > > > > --- a/net/mptcp/options.c > > > > > > +++ b/net/mptcp/options.c > > > > > > @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, > > > > > > return true; > > > > > > > > > > > > reset: > > > > > > + /* If a subflow is reset, the packet should not continue to be > > > > > > + * processed in tcp_data_queue(), so setting: end_seq = seq, > > > > > > + * then tcp_data_queue() will drop the packet. > > > > > > + */ > > > > > > + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; > > > > > > + > > > > > > > > > > This does have the desired effect when mptcp_incoming_options() is > > > > > called from tcp_data_queue(), but mptcp_incoming_options() is also > > > > > called from tcp_reset() and tcp_rcv_state_process(). The other callers > > > > > appear to tolerate the sequence number modification. > > > > > > > > > > I think it would be clearer to either add a return value or output > > > > > parameter to mptcp_incoming_options() to explicitly tell the caller > > > > > that a reset has been sent and tcp_done() called. Then it would be > > > > > clearer in tcp_data_queue() that the packet is being discarded due to > > > > > mptcp header content. > > > > > > > > > > > > > If a reset has been sent and tcp_done() called in > > > > check_fully_established(), the sk_state will be TCP_CLOSE, how about > > > > just do (sk_state == TCP_CLOSE) check in tcp_data_queue() as it did in > > > > the V1 of this patch? > > > > > > Oh, I see now that Paolo suggested the the end_seq assignment in order to > > > only modify MPTCP code. > > > > > > I still think it's better to make it clear that we're discarding a packet > > > due to the mptcp headers - using the existing sequence check (intended to > > > detect acks) in tcp_data_queue() seems sneaky to me. > > > > > > Something like > > > > > > if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { > > > __kfree_skb(skb); > > > return; > > > } > > > > > > seems both compact and clear. Does that seem ok Paolo? > > > > Uhmmm... we need to touch every mptcp_incoming_options() call site, and > > in tcp_reset() the above chunk looks a bit strange to me. Probably we > > could just ignore the mptcp_incoming_options() return value there. > > > > Otherwise no big objections - not sure about upstream ;) > > > > Hi Mat and Paolo, > > If you both agree, I will send a new version that mptcp_incoming_options() add a return value, and only check return value in tcp_data_queue(), > ignore the return value in other call site. Even the hook in tcp_rcv_state_process() can be followed by tcp_data_queue(). I *think* it's better ignoring the return value of mptcp_incoming_options() only in tcp_reset(), adding there a comment - something alike "mptcp can't tell us to ignore reset pkts". Cheers, Paolo p.s. I'm sorry for the long, difficult and somewhat on/off review process. This change is indeed tricky. Don't despair, it looks like it's near to an happy ending! ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset 2021-06-24 10:02 ` Paolo Abeni @ 2021-06-24 22:38 ` Mat Martineau 2021-06-25 0:51 ` Jianguo Wu 1 sibling, 0 replies; 24+ messages in thread From: Mat Martineau @ 2021-06-24 22:38 UTC (permalink / raw) To: Paolo Abeni; +Cc: Jianguo Wu, mptcp, fw On Thu, 24 Jun 2021, Paolo Abeni wrote: > On Thu, 2021-06-24 at 09:57 +0800, Jianguo Wu wrote: >> >> On 2021/6/23 17:48, Paolo Abeni wrote: >>> On Tue, 2021-06-22 at 17:00 -0700, Mat Martineau wrote: >>>> On Mon, 21 Jun 2021, Jianguo Wu wrote: >>>> >>>>> Hi Mat, >>>>> >>>>> On 2021/6/19 8:19, Mat Martineau wrote: >>>>>> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >>>>>> >>>>>>> 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(). >>>>>>> >>>>>>> setting: >>>>>>> TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>>>>>> >>>>>>> so that the following check will drop the pkt in >>>>>>> tcp_data_queue(): >>>>>>> if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { >>>>>>> __kfree_skb(skb); >>>>>>> return; >>>>>>> } >>>>>>> >>>>>>> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") >>>>>>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> >>>>>>> --- >>>>>>> net/mptcp/options.c | 6 ++++++ >>>>>>> 1 file changed, 6 insertions(+) >>>>>>> >>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>>>>>> index 1aec01686c1a..be435c5421cd 100644 >>>>>>> --- a/net/mptcp/options.c >>>>>>> +++ b/net/mptcp/options.c >>>>>>> @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, >>>>>>> return true; >>>>>>> >>>>>>> reset: >>>>>>> + /* If a subflow is reset, the packet should not continue to be >>>>>>> + * processed in tcp_data_queue(), so setting: end_seq = seq, >>>>>>> + * then tcp_data_queue() will drop the packet. >>>>>>> + */ >>>>>>> + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>>>>>> + >>>>>> >>>>>> This does have the desired effect when mptcp_incoming_options() is >>>>>> called from tcp_data_queue(), but mptcp_incoming_options() is also >>>>>> called from tcp_reset() and tcp_rcv_state_process(). The other callers >>>>>> appear to tolerate the sequence number modification. >>>>>> >>>>>> I think it would be clearer to either add a return value or output >>>>>> parameter to mptcp_incoming_options() to explicitly tell the caller >>>>>> that a reset has been sent and tcp_done() called. Then it would be >>>>>> clearer in tcp_data_queue() that the packet is being discarded due to >>>>>> mptcp header content. >>>>>> >>>>> >>>>> If a reset has been sent and tcp_done() called in >>>>> check_fully_established(), the sk_state will be TCP_CLOSE, how about >>>>> just do (sk_state == TCP_CLOSE) check in tcp_data_queue() as it did in >>>>> the V1 of this patch? >>>> >>>> Oh, I see now that Paolo suggested the the end_seq assignment in order to >>>> only modify MPTCP code. >>>> >>>> I still think it's better to make it clear that we're discarding a packet >>>> due to the mptcp headers - using the existing sequence check (intended to >>>> detect acks) in tcp_data_queue() seems sneaky to me. >>>> >>>> Something like >>>> >>>> if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { >>>> __kfree_skb(skb); >>>> return; >>>> } >>>> >>>> seems both compact and clear. Does that seem ok Paolo? >>> >>> Uhmmm... we need to touch every mptcp_incoming_options() call site, and >>> in tcp_reset() the above chunk looks a bit strange to me. Probably we >>> could just ignore the mptcp_incoming_options() return value there. >>> >>> Otherwise no big objections - not sure about upstream ;) >>> >> >> Hi Mat and Paolo, >> >> If you both agree, I will send a new version that mptcp_incoming_options() add a return value, and only check return value in tcp_data_queue(), >> ignore the return value in other call site. > > Even the hook in tcp_rcv_state_process() can be followed by > tcp_data_queue(). > > I *think* it's better ignoring the return value of > mptcp_incoming_options() only in tcp_reset(), adding there a comment - > something alike "mptcp can't tell us to ignore reset pkts". > This sounds like a good approach, thanks. -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset 2021-06-24 10:02 ` Paolo Abeni 2021-06-24 22:38 ` Mat Martineau @ 2021-06-25 0:51 ` Jianguo Wu 1 sibling, 0 replies; 24+ messages in thread From: Jianguo Wu @ 2021-06-25 0:51 UTC (permalink / raw) To: Paolo Abeni, Mat Martineau; +Cc: mptcp, fw On 2021/6/24 18:02, Paolo Abeni wrote: > On Thu, 2021-06-24 at 09:57 +0800, Jianguo Wu wrote: >> >> On 2021/6/23 17:48, Paolo Abeni wrote: >>> On Tue, 2021-06-22 at 17:00 -0700, Mat Martineau wrote: >>>> On Mon, 21 Jun 2021, Jianguo Wu wrote: >>>> >>>>> Hi Mat, >>>>> >>>>> On 2021/6/19 8:19, Mat Martineau wrote: >>>>>> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >>>>>> >>>>>>> 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(). >>>>>>> >>>>>>> setting: >>>>>>> TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>>>>>> >>>>>>> so that the following check will drop the pkt in >>>>>>> tcp_data_queue(): >>>>>>> if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { >>>>>>> __kfree_skb(skb); >>>>>>> return; >>>>>>> } >>>>>>> >>>>>>> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") >>>>>>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> >>>>>>> --- >>>>>>> net/mptcp/options.c | 6 ++++++ >>>>>>> 1 file changed, 6 insertions(+) >>>>>>> >>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>>>>>> index 1aec01686c1a..be435c5421cd 100644 >>>>>>> --- a/net/mptcp/options.c >>>>>>> +++ b/net/mptcp/options.c >>>>>>> @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, >>>>>>> return true; >>>>>>> >>>>>>> reset: >>>>>>> + /* If a subflow is reset, the packet should not continue to be >>>>>>> + * processed in tcp_data_queue(), so setting: end_seq = seq, >>>>>>> + * then tcp_data_queue() will drop the packet. >>>>>>> + */ >>>>>>> + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>>>>>> + >>>>>> >>>>>> This does have the desired effect when mptcp_incoming_options() is >>>>>> called from tcp_data_queue(), but mptcp_incoming_options() is also >>>>>> called from tcp_reset() and tcp_rcv_state_process(). The other callers >>>>>> appear to tolerate the sequence number modification. >>>>>> >>>>>> I think it would be clearer to either add a return value or output >>>>>> parameter to mptcp_incoming_options() to explicitly tell the caller >>>>>> that a reset has been sent and tcp_done() called. Then it would be >>>>>> clearer in tcp_data_queue() that the packet is being discarded due to >>>>>> mptcp header content. >>>>>> >>>>> >>>>> If a reset has been sent and tcp_done() called in >>>>> check_fully_established(), the sk_state will be TCP_CLOSE, how about >>>>> just do (sk_state == TCP_CLOSE) check in tcp_data_queue() as it did in >>>>> the V1 of this patch? >>>> >>>> Oh, I see now that Paolo suggested the the end_seq assignment in order to >>>> only modify MPTCP code. >>>> >>>> I still think it's better to make it clear that we're discarding a packet >>>> due to the mptcp headers - using the existing sequence check (intended to >>>> detect acks) in tcp_data_queue() seems sneaky to me. >>>> >>>> Something like >>>> >>>> if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { >>>> __kfree_skb(skb); >>>> return; >>>> } >>>> >>>> seems both compact and clear. Does that seem ok Paolo? >>> >>> Uhmmm... we need to touch every mptcp_incoming_options() call site, and >>> in tcp_reset() the above chunk looks a bit strange to me. Probably we >>> could just ignore the mptcp_incoming_options() return value there. >>> >>> Otherwise no big objections - not sure about upstream ;) >>> >> >> Hi Mat and Paolo, >> >> If you both agree, I will send a new version that mptcp_incoming_options() add a return value, and only check return value in tcp_data_queue(), >> ignore the return value in other call site. > > Even the hook in tcp_rcv_state_process() can be followed by > tcp_data_queue(). > > I *think* it's better ignoring the return value of > mptcp_incoming_options() only in tcp_reset(), adding there a comment - > something alike "mptcp can't tell us to ignore reset pkts". > > Cheers, > > Paolo > > p.s. I'm sorry for the long, difficult and somewhat on/off review > process. This change is indeed tricky. Don't despair, it looks like > it's near to an happy ending! > Thanks for your review and suggestions! I will send a new version. > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 0/4] Fix some mptcp syncookie process bugs 2021-06-16 10:49 [PATCH v5 0/4] Fix some mptcp syncookie process bugs wujianguo106 ` (3 preceding siblings ...) 2021-06-16 10:49 ` [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset wujianguo106 @ 2021-06-19 0:26 ` Mat Martineau 2021-06-21 6:39 ` Jianguo Wu 4 siblings, 1 reply; 24+ messages in thread From: Mat Martineau @ 2021-06-19 0:26 UTC (permalink / raw) To: wujianguo106; +Cc: mptcp, pabeni, fw On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > From: Jianguo Wu <wujianguo@chinatelecom.cn> > > v4->v5: > patch1: no changes > patch2: no changes > patch3: no changes > patch4: add comment Hi Jianguo - One other thing that's helpful for reviewers on the list is to use the patch prefixes listed at https://github.com/multipath-tcp/mptcp_net-next/wiki/Patch-prefixes when emailing patches to mptcp@lists.linux.dev For example, if these patches are intended for net-next they should be sent with [PATCH mptcp-next] in the subject line, or for the net tree they should have [PATCH mptcp-net]. If you're unsure where the patches belong, please ask! Thanks, Mat > > v3->v4: > patch1: using seq and sport/dport for hashing, and ignore network headers altogether, > as suggest by Florian > patch2: no changes > patch3: no changes > patch4: no changes > > v2->v3: > patch1: directly use inet6_ehashfn() for IPv6 > patch2: no changes > patch3: no changes > patch4: add Fixes tag. > > v1->v2: > patch1: handle ipv6 sockets/addresses, > always use 4-tuple drived hash and never look at skb->hash > patch2: no changes > patch3: split into 2 patches. > patch4: new added. > > Jianguo Wu (4): > 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 > > net/mptcp/options.c | 6 ++++++ > net/mptcp/subflow.c | 11 +++-------- > net/mptcp/syncookies.c | 16 +++++++++++++++- > 3 files changed, 24 insertions(+), 9 deletions(-) > > -- > 1.8.3.1 > > > -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 0/4] Fix some mptcp syncookie process bugs 2021-06-19 0:26 ` [PATCH v5 0/4] Fix some mptcp syncookie process bugs Mat Martineau @ 2021-06-21 6:39 ` Jianguo Wu 2021-06-23 0:07 ` Mat Martineau 0 siblings, 1 reply; 24+ messages in thread From: Jianguo Wu @ 2021-06-21 6:39 UTC (permalink / raw) To: Mat Martineau; +Cc: mptcp, pabeni, fw Hi Mat, On 2021/6/19 8:26, Mat Martineau wrote: > On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > >> From: Jianguo Wu <wujianguo@chinatelecom.cn> >> >> v4->v5: >> patch1: no changes >> patch2: no changes >> patch3: no changes >> patch4: add comment > > Hi Jianguo - > > One other thing that's helpful for reviewers on the list is to use the patch prefixes listed at https://github.com/multipath-tcp/mptcp_net-next/wiki/Patch-prefixes > when emailing patches to mptcp@lists.linux.dev > Thanks for your reminding! > For example, if these patches are intended for net-next they should be sent with [PATCH mptcp-next] in the subject line, or for the net tree they should have [PATCH mptcp-net]. If you're unsure where the patches belong, please ask! > and this patchset should use [PATCH mptcp-next]? > Thanks, > > Mat > > >> >> v3->v4: >> patch1: using seq and sport/dport for hashing, and ignore network headers altogether, >> as suggest by Florian >> patch2: no changes >> patch3: no changes >> patch4: no changes >> >> v2->v3: >> patch1: directly use inet6_ehashfn() for IPv6 >> patch2: no changes >> patch3: no changes >> patch4: add Fixes tag. >> >> v1->v2: >> patch1: handle ipv6 sockets/addresses, >> always use 4-tuple drived hash and never look at skb->hash >> patch2: no changes >> patch3: split into 2 patches. >> patch4: new added. >> >> Jianguo Wu (4): >> 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 >> >> net/mptcp/options.c | 6 ++++++ >> net/mptcp/subflow.c | 11 +++-------- >> net/mptcp/syncookies.c | 16 +++++++++++++++- >> 3 files changed, 24 insertions(+), 9 deletions(-) >> >> -- >> 1.8.3.1 >> >> >> > > -- > Mat Martineau > Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 0/4] Fix some mptcp syncookie process bugs 2021-06-21 6:39 ` Jianguo Wu @ 2021-06-23 0:07 ` Mat Martineau 0 siblings, 0 replies; 24+ messages in thread From: Mat Martineau @ 2021-06-23 0:07 UTC (permalink / raw) To: Jianguo Wu; +Cc: mptcp, pabeni, fw [-- Attachment #1: Type: text/plain, Size: 2285 bytes --] On Mon, 21 Jun 2021, Jianguo Wu wrote: > Hi Mat, > > On 2021/6/19 8:26, Mat Martineau wrote: >> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >> >>> From: Jianguo Wu <wujianguo@chinatelecom.cn> >>> >>> v4->v5: >>> patch1: no changes >>> patch2: no changes >>> patch3: no changes >>> patch4: add comment >> >> Hi Jianguo - >> >> One other thing that's helpful for reviewers on the list is to use the patch prefixes listed at https://github.com/multipath-tcp/mptcp_net-next/wiki/Patch-prefixes >> when emailing patches to mptcp@lists.linux.dev >> > > Thanks for your reminding! > >> For example, if these patches are intended for net-next they should be sent with [PATCH mptcp-next] in the subject line, or for the net tree they should have [PATCH mptcp-net]. If you're unsure where the patches belong, please ask! >> > > and this patchset should use [PATCH mptcp-next]? I think these look like mptcp-net patches, especially considering the risk of double-free in patch 2. For patch 4 I'm not sure if that would be best for net-next. Is it an optimization or fixing a crash / error / warning? -Mat > >> Thanks, >> >> Mat >> >> >>> >>> v3->v4: >>> patch1: using seq and sport/dport for hashing, and ignore network headers altogether, >>> as suggest by Florian >>> patch2: no changes >>> patch3: no changes >>> patch4: no changes >>> >>> v2->v3: >>> patch1: directly use inet6_ehashfn() for IPv6 >>> patch2: no changes >>> patch3: no changes >>> patch4: add Fixes tag. >>> >>> v1->v2: >>> patch1: handle ipv6 sockets/addresses, >>> always use 4-tuple drived hash and never look at skb->hash >>> patch2: no changes >>> patch3: split into 2 patches. >>> patch4: new added. >>> >>> Jianguo Wu (4): >>> 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 >>> >>> net/mptcp/options.c | 6 ++++++ >>> net/mptcp/subflow.c | 11 +++-------- >>> net/mptcp/syncookies.c | 16 +++++++++++++++- >>> 3 files changed, 24 insertions(+), 9 deletions(-) >>> >>> -- ??>> 1.8.3.1 >>> >>> >>> >> >> -- >> Mat Martineau >> Intel > > -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-06-25 1:07 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-16 10:49 [PATCH v5 0/4] Fix some mptcp syncookie process bugs wujianguo106 2021-06-16 10:49 ` [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106 2021-06-18 22:40 ` Mat Martineau 2021-06-21 6:14 ` Jianguo Wu 2021-06-21 10:09 ` Jianguo Wu 2021-06-22 23:38 ` Mat Martineau 2021-06-16 10:49 ` [PATCH v5 2/4] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106 2021-06-16 10:49 ` [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106 2021-06-18 23:19 ` Mat Martineau 2021-06-21 6:24 ` Jianguo Wu 2021-06-24 2:08 ` Jianguo Wu 2021-06-24 22:36 ` Mat Martineau 2021-06-16 10:49 ` [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset wujianguo106 2021-06-19 0:19 ` Mat Martineau 2021-06-21 6:35 ` Jianguo Wu 2021-06-23 0:00 ` Mat Martineau 2021-06-23 9:48 ` Paolo Abeni 2021-06-24 1:57 ` Jianguo Wu 2021-06-24 10:02 ` Paolo Abeni 2021-06-24 22:38 ` Mat Martineau 2021-06-25 0:51 ` Jianguo Wu 2021-06-19 0:26 ` [PATCH v5 0/4] Fix some mptcp syncookie process bugs Mat Martineau 2021-06-21 6:39 ` Jianguo Wu 2021-06-23 0:07 ` Mat Martineau
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.