* [PATCH v4 0/4] Fix some mptcp syncookie process bugs
@ 2021-06-15 3:02 wujianguo106
2021-06-15 3:02 ` [PATCH v4 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: wujianguo106 @ 2021-06-15 3:02 UTC (permalink / raw)
To: mptcp; +Cc: pabeni, fw
From: Jianguo Wu <wujianguo@chinatelecom.cn>
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 | 2 ++
net/mptcp/subflow.c | 11 +++--------
net/mptcp/syncookies.c | 16 +++++++++++++++-
3 files changed, 20 insertions(+), 9 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join
2021-06-15 3:02 [PATCH v4 0/4] Fix some mptcp syncookie process bugs wujianguo106
@ 2021-06-15 3:02 ` wujianguo106
2021-06-15 3:02 ` [PATCH v4 2/4] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: wujianguo106 @ 2021-06-15 3:02 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] 7+ messages in thread
* [PATCH v4 2/4] mptcp: remove redundant req destruct in subflow_check_req()
2021-06-15 3:02 [PATCH v4 0/4] Fix some mptcp syncookie process bugs wujianguo106
2021-06-15 3:02 ` [PATCH v4 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
@ 2021-06-15 3:02 ` wujianguo106
2021-06-15 3:02 ` [PATCH v4 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106
2021-06-15 3:02 ` [PATCH v4 4/4] mptcp: avoid processing packet if a subflow reset wujianguo106
3 siblings, 0 replies; 7+ messages in thread
From: wujianguo106 @ 2021-06-15 3:02 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] 7+ messages in thread
* [PATCH v4 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow
2021-06-15 3:02 [PATCH v4 0/4] Fix some mptcp syncookie process bugs wujianguo106
2021-06-15 3:02 ` [PATCH v4 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
2021-06-15 3:02 ` [PATCH v4 2/4] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106
@ 2021-06-15 3:02 ` wujianguo106
2021-06-15 3:02 ` [PATCH v4 4/4] mptcp: avoid processing packet if a subflow reset wujianguo106
3 siblings, 0 replies; 7+ messages in thread
From: wujianguo106 @ 2021-06-15 3:02 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] 7+ messages in thread
* [PATCH v4 4/4] mptcp: avoid processing packet if a subflow reset
2021-06-15 3:02 [PATCH v4 0/4] Fix some mptcp syncookie process bugs wujianguo106
` (2 preceding siblings ...)
2021-06-15 3:02 ` [PATCH v4 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106
@ 2021-06-15 3:02 ` wujianguo106
2021-06-15 15:19 ` Florian Westphal
3 siblings, 1 reply; 7+ messages in thread
From: wujianguo106 @ 2021-06-15 3:02 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 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 1aec01686c1a..01a1bf60b187 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -926,6 +926,8 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
return true;
reset:
+ 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] 7+ messages in thread
* Re: [PATCH v4 4/4] mptcp: avoid processing packet if a subflow reset
2021-06-15 3:02 ` [PATCH v4 4/4] mptcp: avoid processing packet if a subflow reset wujianguo106
@ 2021-06-15 15:19 ` Florian Westphal
2021-06-16 7:02 ` Jianguo Wu
0 siblings, 1 reply; 7+ messages in thread
From: Florian Westphal @ 2021-06-15 15:19 UTC (permalink / raw)
To: wujianguo106; +Cc: mptcp, pabeni, fw
wujianguo106@163.com <wujianguo106@163.com> wrote:
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1aec01686c1a..01a1bf60b187 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -926,6 +926,8 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
> return true;
>
> reset:
> + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq;
Magic. Please at least add a comment here.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 4/4] mptcp: avoid processing packet if a subflow reset
2021-06-15 15:19 ` Florian Westphal
@ 2021-06-16 7:02 ` Jianguo Wu
0 siblings, 0 replies; 7+ messages in thread
From: Jianguo Wu @ 2021-06-16 7:02 UTC (permalink / raw)
To: Florian Westphal; +Cc: mptcp, pabeni
Hi Florian,
On 2021/6/15 23:19, Florian Westphal wrote:
> wujianguo106@163.com <wujianguo106@163.com> wrote:
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 1aec01686c1a..01a1bf60b187 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -926,6 +926,8 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>> return true;
>>
>> reset:
>> + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq;
>
> Magic. Please at least add a comment here.
>
Ok, will add a comment, thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-16 7:34 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15 3:02 [PATCH v4 0/4] Fix some mptcp syncookie process bugs wujianguo106
2021-06-15 3:02 ` [PATCH v4 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
2021-06-15 3:02 ` [PATCH v4 2/4] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106
2021-06-15 3:02 ` [PATCH v4 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106
2021-06-15 3:02 ` [PATCH v4 4/4] mptcp: avoid processing packet if a subflow reset wujianguo106
2021-06-15 15:19 ` Florian Westphal
2021-06-16 7:02 ` Jianguo Wu
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).