[PATCH v5 0/4] Fix some mptcp syncookie process bugs

[PATCH v5 0/4] Fix some mptcp syncookie process bugs

[wujianguo106]

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

[PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join

[wujianguo106]

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

[PATCH v5 2/4] mptcp: remove redundant req destruct in subflow_check_req()

[wujianguo106]

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

[PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow

[wujianguo106]

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

[PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset

[wujianguo106]

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

Re: [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join

[Mat Martineau]

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

Re: [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow

[Mat Martineau]

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

Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset

[Mat Martineau]

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

Re: [PATCH v5 0/4] Fix some mptcp syncookie process bugs

[Mat Martineau]

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

Re: [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join

[Jianguo Wu]

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

Re: [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow

[Jianguo Wu]

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

Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset

[Jianguo Wu]

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

Re: [PATCH v5 0/4] Fix some mptcp syncookie process bugs

[Jianguo Wu]

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

Re: [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join

[Jianguo Wu]

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
> 

Re: [PATCH v5 1/4] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join

[Mat Martineau]

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

Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset

[Mat Martineau]

[-- 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

Re: [PATCH v5 0/4] Fix some mptcp syncookie process bugs

[Mat Martineau]

[-- 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

Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset

[Paolo Abeni]

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

Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset

[Jianguo Wu]

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
> 

Re: [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow

[Jianguo Wu]

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
> 

Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset

[Paolo Abeni]

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!

Re: [PATCH v5 3/4] mptcp: fix syncookie process if mptcp can not_accept new subflow

[Mat Martineau]

[-- 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

Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset

[Mat Martineau]

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

Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset

[Jianguo Wu]

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.

>