mptcp.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs
@ 2021-06-26  9:16 wujianguo106
  2021-06-26  9:16 ` [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: wujianguo106 @ 2021-06-26  9:16 UTC (permalink / raw)
  To: mptcp; +Cc: pabeni, mathew.j.martineau

From: wujianguo <wujianguo@chinatelecom.cn>

v7->v8:
 - patch4: using bool for return value, and keep check_fully_established()
	   untouched.

v6->v7:
 - patch4: corret the declaration of mptcp_incoming_options()
	   when CONFIG_MPTCP is not set
 - patch5: add tag Reported-by: kernel test robot <oliver.sang@intel.com>

v5->v6:
 - patch1: describe the testing tools in the commit log
 - patch4: add return value to mptcp_incoming_options(), return 0 if
	   a subflow has been reset, else return 1, and drop the packet
	   in tcp_data_queue()/tcp_rcv_state_process() if the return
	   value is 0
 - patch5: update selftest case "multiple subflows limited by server",
	   since the expected behavior is changed by patch4

v4->v5:
 - patch4: add comment 

v3->v4:
 - patch1: using seq and sport/dport for hashing, and ignore network
	   headers altogether, as suggest by Florian

v2->v3:
 - patch1: directly use inet6_ehashfn() for IPv6
 - patch4: add Fixes tag.

v1->v2:
 - patch1: handle ipv6 sockets/addresses,
	   always use 4-tuple drived hash and never look at skb->hash
 - patch3: split into 2 patches.
 - patch4: new added.

Jianguo Wu (5):
  mptcp: fix warning in __skb_flow_dissect() when do syn cookie for
    subflow join
  mptcp: remove redundant req destruct in subflow_check_req()
  mptcp: fix syncookie process if mptcp can not_accept new subflow
  mptcp: avoid processing packet if a subflow reset
  selftests: mptcp: fix case multiple subflows limited by server

 include/net/mptcp.h                             |  5 +++--
 net/ipv4/tcp_input.c                            | 19 +++++++++++++++----
 net/mptcp/options.c                             | 19 +++++++++++++------
 net/mptcp/subflow.c                             | 11 +++--------
 net/mptcp/syncookies.c                          | 16 +++++++++++++++-
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  2 +-
 6 files changed, 50 insertions(+), 22 deletions(-)

-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join
  2021-06-26  9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106
@ 2021-06-26  9:16 ` wujianguo106
  2021-06-26  9:16 ` [PATCH mptcp-net v8 2/5] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: wujianguo106 @ 2021-06-26  9:16 UTC (permalink / raw)
  To: mptcp; +Cc: pabeni, mathew.j.martineau

From: Jianguo Wu <wujianguo@chinatelecom.cn>

I did stress test with wrk(https://github.com/wg/wrk) and webfsd(https://github.com/ourway/webfsd)
with the assistance of mptcp-tools(https://github.com/pabeni/mptcp-tools):
  Server side:
      ./use_mptcp.sh webfsd -4 -R /tmp/ -p 8099
  Client side:
      ./use_mptcp.sh wrk -c 200 -d 30 -t 4 http://192.168.174.129:8099/

and got the following warning message:

[   55.552626] TCP: request_sock_subflow: Possible SYN flooding on port 8099. Sending cookies.  Check SNMP counters.
[   55.553024] ------------[ cut here ]------------
[   55.553027] WARNING: CPU: 0 PID: 10 at net/core/flow_dissector.c:984 __skb_flow_dissect+0x280/0x1650
...
[   55.553117] CPU: 0 PID: 10 Comm: ksoftirqd/0 Not tainted 5.12.0+ #18
[   55.553121] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 02/27/2020
[   55.553124] RIP: 0010:__skb_flow_dissect+0x280/0x1650
...
[   55.553133] RSP: 0018:ffffb79580087770 EFLAGS: 00010246
[   55.553137] RAX: 0000000000000000 RBX: ffffffff8ddb58e0 RCX: ffffb79580087888
[   55.553139] RDX: ffffffff8ddb58e0 RSI: ffff8f7e4652b600 RDI: 0000000000000000
[   55.553141] RBP: ffffb79580087858 R08: 0000000000000000 R09: 0000000000000008
[   55.553143] R10: 000000008c622965 R11: 00000000d3313a5b R12: ffff8f7e4652b600
[   55.553146] R13: ffff8f7e465c9062 R14: 0000000000000000 R15: ffffb79580087888
[   55.553149] FS:  0000000000000000(0000) GS:ffff8f7f75e00000(0000) knlGS:0000000000000000
[   55.553152] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   55.553154] CR2: 00007f73d1d19000 CR3: 0000000135e10004 CR4: 00000000003706f0
[   55.553160] Call Trace:
[   55.553166]  ? __sha256_final+0x67/0xd0
[   55.553173]  ? sha256+0x7e/0xa0
[   55.553177]  __skb_get_hash+0x57/0x210
[   55.553182]  subflow_init_req_cookie_join_save+0xac/0xc0
[   55.553189]  subflow_check_req+0x474/0x550
[   55.553195]  ? ip_route_output_key_hash+0x67/0x90
[   55.553200]  ? xfrm_lookup_route+0x1d/0xa0
[   55.553207]  subflow_v4_route_req+0x8e/0xd0
[   55.553212]  tcp_conn_request+0x31e/0xab0
[   55.553218]  ? selinux_socket_sock_rcv_skb+0x116/0x210
[   55.553224]  ? tcp_rcv_state_process+0x179/0x6d0
[   55.553229]  tcp_rcv_state_process+0x179/0x6d0
[   55.553235]  tcp_v4_do_rcv+0xaf/0x220
[   55.553239]  tcp_v4_rcv+0xce4/0xd80
[   55.553243]  ? ip_route_input_rcu+0x246/0x260
[   55.553248]  ip_protocol_deliver_rcu+0x35/0x1b0
[   55.553253]  ip_local_deliver_finish+0x44/0x50
[   55.553258]  ip_local_deliver+0x6c/0x110
[   55.553262]  ? ip_rcv_finish_core.isra.19+0x5a/0x400
[   55.553267]  ip_rcv+0xd1/0xe0
...

After debugging, I found in __skb_flow_dissect(), skb->dev and skb->sk are both NULL,
then net is NULL, and trigger WARN_ON_ONCE(!net), actually net is always NULL in this
code path, as skb->dev is set to NULL in tcp_v4_rcv(), and skb->sk is never set.

Code snippet in __skb_flow_dissect() that trigger warning:
  975         if (skb) {
  976                 if (!net) {
  977                         if (skb->dev)
  978                                 net = dev_net(skb->dev);
  979                         else if (skb->sk)
  980                                 net = sock_net(skb->sk);
  981                 }
  982         }
  983
  984         WARN_ON_ONCE(!net);

So, using seq and transport header derived hash.

Fixes: 9466a1ccebbe("mptcp: enable JOIN requests even if cookies are in use").
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
---
 net/mptcp/syncookies.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/syncookies.c b/net/mptcp/syncookies.c
index abe0fd0..3712778 100644
--- a/net/mptcp/syncookies.c
+++ b/net/mptcp/syncookies.c
@@ -37,7 +37,21 @@ struct join_entry {
 
 static u32 mptcp_join_entry_hash(struct sk_buff *skb, struct net *net)
 {
-	u32 i = skb_get_hash(skb) ^ net_hash_mix(net);
+	static u32 mptcp_join_hash_secret __read_mostly;
+	struct tcphdr *th = tcp_hdr(skb);
+	u32 seq, i;
+
+	net_get_random_once(&mptcp_join_hash_secret,
+			    sizeof(mptcp_join_hash_secret));
+
+	if (th->syn)
+		seq = TCP_SKB_CB(skb)->seq;
+	else
+		seq = TCP_SKB_CB(skb)->seq - 1;
+
+	i = jhash_3words(seq, net_hash_mix(net),
+			 (__force __u32)th->source << 16 | (__force __u32)th->dest,
+			 mptcp_join_hash_secret);
 
 	return i % ARRAY_SIZE(join_entries);
 }
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH mptcp-net v8 2/5] mptcp: remove redundant req destruct in subflow_check_req()
  2021-06-26  9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106
  2021-06-26  9:16 ` [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
@ 2021-06-26  9:16 ` wujianguo106
  2021-06-26  9:16 ` [PATCH mptcp-net v8 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: wujianguo106 @ 2021-06-26  9:16 UTC (permalink / raw)
  To: mptcp; +Cc: pabeni, mathew.j.martineau

From: Jianguo Wu <wujianguo@chinatelecom.cn>

In subflow_check_req(), if subflow sport is mismatch, will put msk,
destroy token, and destruct req, then return -EPERM, which can be
done by subflow_req_destructor() via:
  tcp_conn_request()
    |--__reqsk_free()
      |--subflow_req_destructor()
So we should remove these redundant code, otherwise will call
tcp_v4_reqsk_destructor() twice, and may double free inet_rsk(req)->ireq_opt.

Fixes: 5bc56388c74f ("mptcp: add port number check for MP_JOIN")
Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
---
 net/mptcp/subflow.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 0b5d4a3..3a849fb 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -214,11 +214,6 @@ static int subflow_check_req(struct request_sock *req,
 				 ntohs(inet_sk(sk_listener)->inet_sport),
 				 ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
 			if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) {
-				sock_put((struct sock *)subflow_req->msk);
-				mptcp_token_destroy_request(req);
-				tcp_request_sock_ops.destructor(req);
-				subflow_req->msk = NULL;
-				subflow_req->mp_join = 0;
 				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX);
 				return -EPERM;
 			}
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH mptcp-net v8 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow
  2021-06-26  9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106
  2021-06-26  9:16 ` [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
  2021-06-26  9:16 ` [PATCH mptcp-net v8 2/5] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106
@ 2021-06-26  9:16 ` wujianguo106
  2021-06-26  9:16 ` [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
  2021-06-26  9:16 ` [PATCH mptcp-net v8 5/5] selftests: mptcp: fix case multiple subflows limited by server wujianguo106
  4 siblings, 0 replies; 10+ messages in thread
From: wujianguo106 @ 2021-06-26  9:16 UTC (permalink / raw)
  To: mptcp; +Cc: pabeni, mathew.j.martineau

From: Jianguo Wu <wujianguo@chinatelecom.cn>

Lots of "TCP: tcp_fin: Impossible, sk->sk_state=7" in client side
when doing stress testing using wrk and webfsd.

There are at least two cases may trigger this warning:
1.mptcp is in syncookie, and server recv MP_JOIN SYN request,
  in subflow_check_req(), the mptcp_can_accept_new_subflow()
  return false, so subflow_init_req_cookie_join_save() isn't
  called, i.e. not store the data present in the MP_JOIN syn
  request and the random nonce in hash table - join_entries[],
  but still send synack. When recv 3rd-ack,
  mptcp_token_join_cookie_init_state() will return false, and
  3rd-ack is dropped, then if mptcp conn is closed by client,
  client will send a DATA_FIN and a MPTCP FIN, the DATA_FIN
  doesn't have MP_CAPABLE or MP_JOIN,
  so mptcp_subflow_init_cookie_req() will return 0, and pass
  the cookie check, MP_JOIN request is fallback to normal TCP.
  Server will send a TCP FIN if closed, in client side,
  when process TCP FIN, it will do reset, the code path is:
    tcp_data_queue()->mptcp_incoming_options()
      ->check_fully_established()->mptcp_subflow_reset().
  mptcp_subflow_reset() will set sock state to TCP_CLOSE,
  so tcp_fin will hit TCP_CLOSE, and print the warning.
2.mptcp is in syncookie, and server recv 3rd-ack, in
  mptcp_subflow_init_cookie_req(), mptcp_can_accept_new_subflow()
  return false, and subflow_req->mp_join is not set to 1,
  so in subflow_syn_recv_sock() will not reset the MP_JOIN
  subflow, but fallback to normal TCP, and then the same thing
  happens when server will send a TCP FIN if closed.

For case1, subflow_check_req() return -EPERM,
then tcp_conn_request() will drop MP_JOIN SYN.

For case2, let subflow_syn_recv_sock() call mptcp_can_accept_new_subflow(),
and do fatal fallback, send reset.

Fixes: 9466a1ccebbe("mptcp: enable JOIN requests even if cookies are in use")
Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
---
 net/mptcp/subflow.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 3a849fb..f3f1358 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -225,6 +225,8 @@ static int subflow_check_req(struct request_sock *req,
 		if (unlikely(req->syncookie)) {
 			if (mptcp_can_accept_new_subflow(subflow_req->msk))
 				subflow_init_req_cookie_join_save(subflow_req, skb);
+			else
+				return -EPERM;
 		}
 
 		pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token,
@@ -264,9 +266,7 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req,
 		if (!mptcp_token_join_cookie_init_state(subflow_req, skb))
 			return -EINVAL;
 
-		if (mptcp_can_accept_new_subflow(subflow_req->msk))
-			subflow_req->mp_join = 1;
-
+		subflow_req->mp_join = 1;
 		subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq - 1;
 	}
 
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset
  2021-06-26  9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106
                   ` (2 preceding siblings ...)
  2021-06-26  9:16 ` [PATCH mptcp-net v8 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106
@ 2021-06-26  9:16 ` wujianguo106
  2021-06-28  8:35   ` Geliang Tang
  2021-06-26  9:16 ` [PATCH mptcp-net v8 5/5] selftests: mptcp: fix case multiple subflows limited by server wujianguo106
  4 siblings, 1 reply; 10+ messages in thread
From: wujianguo106 @ 2021-06-26  9:16 UTC (permalink / raw)
  To: mptcp; +Cc: pabeni, mathew.j.martineau

From: Jianguo Wu <wujianguo@chinatelecom.cn>

If check_fully_established() causes a subflow reset, it should not
continue to process the packet in tcp_data_queue().
Add a return value to mptcp_incoming_options(), and return false if a
subflow has been reset, else return true. Then drop the packet in
tcp_data_queue()/tcp_rcv_state_process() if mptcp_incoming_options()
return false.

Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
---
 include/net/mptcp.h  |  5 +++--
 net/ipv4/tcp_input.c | 19 +++++++++++++++----
 net/mptcp/options.c  | 19 +++++++++++++------
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index cb580b0..8b5af68 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -105,7 +105,7 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
 bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 			       unsigned int *size, unsigned int remaining,
 			       struct mptcp_out_options *opts);
-void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb);
+bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb);
 
 void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			 struct mptcp_out_options *opts);
@@ -227,9 +227,10 @@ static inline bool mptcp_established_options(struct sock *sk,
 	return false;
 }
 
-static inline void mptcp_incoming_options(struct sock *sk,
+static inline bool mptcp_incoming_options(struct sock *sk,
 					  struct sk_buff *skb)
 {
+	return true;
 }
 
 static inline void mptcp_skb_ext_move(struct sk_buff *to,
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7d5e59f..4bacd7d 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4247,6 +4247,9 @@ void tcp_reset(struct sock *sk, struct sk_buff *skb)
 {
 	trace_tcp_receive_reset(sk);
 
+	/* mptcp can't tell us to ignore reset pkts,
+	 * so just ignore the return value of mptcp_incoming_options().
+	 */
 	if (sk_is_mptcp(sk))
 		mptcp_incoming_options(sk, skb);
 
@@ -4941,8 +4944,13 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
 	bool fragstolen;
 	int eaten;
 
-	if (sk_is_mptcp(sk))
-		mptcp_incoming_options(sk, skb);
+	/* If a subflow has been reset, the packet should not continue
+	 * to be processed, drop the packet.
+	 */
+	if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) {
+		__kfree_skb(skb);
+		return;
+	}
 
 	if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) {
 		__kfree_skb(skb);
@@ -6523,8 +6531,11 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 	case TCP_CLOSING:
 	case TCP_LAST_ACK:
 		if (!before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) {
-			if (sk_is_mptcp(sk))
-				mptcp_incoming_options(sk, skb);
+			/* If a subflow has been reset, the packet should not
+			 * continue to be processed, drop the packet.
+			 */
+			if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb))
+				goto discard;
 			break;
 		}
 		fallthrough;
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index b5850af..4452455 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1035,7 +1035,8 @@ static bool add_addr_hmac_valid(struct mptcp_sock *msk,
 	return hmac == mp_opt->ahmac;
 }
 
-void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
+/* Return false if a subflow has been reset, else return true */
+bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
@@ -1053,12 +1054,16 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 			__mptcp_check_push(subflow->conn, sk);
 		__mptcp_data_acked(subflow->conn);
 		mptcp_data_unlock(subflow->conn);
-		return;
+		return true;
 	}
 
 	mptcp_get_options(sk, skb, &mp_opt);
+
+	/* The subflow can be in close state only if check_fully_established()
+	 * just sent a reset. If so, tell the caller to ignore the current packet.
+	 */
 	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
-		return;
+		return sk->sk_state != TCP_CLOSE;
 
 	if (mp_opt.fastclose &&
 	    msk->local_key == mp_opt.rcvr_key) {
@@ -1100,7 +1105,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 	}
 
 	if (!mp_opt.dss)
-		return;
+		return true;
 
 	/* we can't wait for recvmsg() to update the ack_seq, otherwise
 	 * monodirectional flows will stuck
@@ -1119,12 +1124,12 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		    schedule_work(&msk->work))
 			sock_hold(subflow->conn);
 
-		return;
+		return true;
 	}
 
 	mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
 	if (!mpext)
-		return;
+		return true;
 
 	memset(mpext, 0, sizeof(*mpext));
 
@@ -1153,6 +1158,8 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		if (mpext->csum_reqd)
 			mpext->csum = mp_opt.csum;
 	}
+
+	return true;
 }
 
 static void mptcp_set_rwin(const struct tcp_sock *tp)
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH mptcp-net v8 5/5] selftests: mptcp: fix case multiple subflows limited by server
  2021-06-26  9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106
                   ` (3 preceding siblings ...)
  2021-06-26  9:16 ` [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
@ 2021-06-26  9:16 ` wujianguo106
  4 siblings, 0 replies; 10+ messages in thread
From: wujianguo106 @ 2021-06-26  9:16 UTC (permalink / raw)
  To: mptcp; +Cc: pabeni, mathew.j.martineau

From: Jianguo Wu <wujianguo@chinatelecom.cn>

After patch "mptcp: fix syncookie process if mptcp can not_accept new subflow",
If subflow is limited, MP_JOIN SYN is dropped, and no SYN/ACK will be replied.
So in case "multiple subflows limited by server", the expected SYN/ACK number
should be 1.

Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 9a191c1..f02f4de 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1409,7 +1409,7 @@ syncookies_tests()
 	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
 	ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 flags subflow
 	run_tests $ns1 $ns2 10.0.1.1
-	chk_join_nr "subflows limited by server w cookies" 2 2 1
+	chk_join_nr "subflows limited by server w cookies" 2 1 1
 
 	# test signal address with cookies
 	reset_with_cookies
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset
  2021-06-26  9:16 ` [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
@ 2021-06-28  8:35   ` Geliang Tang
  2021-06-28 16:45     ` Matthieu Baerts
  2021-06-29  2:55     ` Jianguo Wu
  0 siblings, 2 replies; 10+ messages in thread
From: Geliang Tang @ 2021-06-28  8:35 UTC (permalink / raw)
  To: Jianguo Wu; +Cc: mptcp, Paolo Abeni, Mat Martineau

Hi Jianguo,

Thank you for this new patch set!

There are three checkpatch.pl warnings in this patch set, please fix them.
And a minor comment below.

---------------------------------------------------------------
0001-mptcp-fix-warning-in-__skb_flow_dissect-when-do-syn-.patch
---------------------------------------------------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#7:
I did stress test with wrk(https://github.com/wg/wrk) and
webfsd(https://github.com/ourway/webfsd)

total: 0 errors, 1 warnings, 0 checks, 22 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0001-mptcp-fix-warning-in-__skb_flow_dissect-when-do-syn-.patch has
style problems, please review.
---------------------------------------------------------------
0002-mptcp-remove-redundant-req-destruct-in-subflow_check.patch
---------------------------------------------------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#14:
tcp_v4_reqsk_destructor() twice, and may double free inet_rsk(req)->ireq_opt.

total: 0 errors, 1 warnings, 0 checks, 11 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0002-mptcp-remove-redundant-req-destruct-in-subflow_check.patch has
style problems, please review.
---------------------------------------------------------------
0005-selftests-mptcp-fix-case-multiple-subflows-limited-b.patch
---------------------------------------------------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#7:
After patch "mptcp: fix syncookie process if mptcp can not_accept new subflow",

total: 0 errors, 1 warnings, 8 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0005-selftests-mptcp-fix-case-multiple-subflows-limited-b.patch has
style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


<wujianguo106@163.com> 于2021年6月26日周六 下午5:17写道:
>
> From: Jianguo Wu <wujianguo@chinatelecom.cn>
>
> If check_fully_established() causes a subflow reset, it should not
> continue to process the packet in tcp_data_queue().
> Add a return value to mptcp_incoming_options(), and return false if a
> subflow has been reset, else return true. Then drop the packet in
> tcp_data_queue()/tcp_rcv_state_process() if mptcp_incoming_options()
> return false.
>
> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
> ---
>  include/net/mptcp.h  |  5 +++--
>  net/ipv4/tcp_input.c | 19 +++++++++++++++----
>  net/mptcp/options.c  | 19 +++++++++++++------
>  3 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index cb580b0..8b5af68 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -105,7 +105,7 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
>  bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>                                unsigned int *size, unsigned int remaining,
>                                struct mptcp_out_options *opts);
> -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb);
> +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb);
>
>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>                          struct mptcp_out_options *opts);
> @@ -227,9 +227,10 @@ static inline bool mptcp_established_options(struct sock *sk,
>         return false;
>  }
>
> -static inline void mptcp_incoming_options(struct sock *sk,
> +static inline bool mptcp_incoming_options(struct sock *sk,
>                                           struct sk_buff *skb)
>  {
> +       return true;
>  }
>
>  static inline void mptcp_skb_ext_move(struct sk_buff *to,
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 7d5e59f..4bacd7d 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4247,6 +4247,9 @@ void tcp_reset(struct sock *sk, struct sk_buff *skb)
>  {
>         trace_tcp_receive_reset(sk);
>
> +       /* mptcp can't tell us to ignore reset pkts,
> +        * so just ignore the return value of mptcp_incoming_options().
> +        */
>         if (sk_is_mptcp(sk))
>                 mptcp_incoming_options(sk, skb);
>
> @@ -4941,8 +4944,13 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
>         bool fragstolen;
>         int eaten;
>
> -       if (sk_is_mptcp(sk))
> -               mptcp_incoming_options(sk, skb);
> +       /* If a subflow has been reset, the packet should not continue
> +        * to be processed, drop the packet.
> +        */
> +       if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) {
> +               __kfree_skb(skb);
> +               return;
> +       }
>
>         if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) {
>                 __kfree_skb(skb);
> @@ -6523,8 +6531,11 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
>         case TCP_CLOSING:
>         case TCP_LAST_ACK:
>                 if (!before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) {
> -                       if (sk_is_mptcp(sk))
> -                               mptcp_incoming_options(sk, skb);
> +                       /* If a subflow has been reset, the packet should not
> +                        * continue to be processed, drop the packet.
> +                        */
> +                       if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb))
> +                               goto discard;
>                         break;
>                 }
>                 fallthrough;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index b5850af..4452455 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1035,7 +1035,8 @@ static bool add_addr_hmac_valid(struct mptcp_sock *msk,
>         return hmac == mp_opt->ahmac;
>  }
>
> -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
> +/* Return false if a subflow has been reset, else return true */
> +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>  {
>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> @@ -1053,12 +1054,16 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>                         __mptcp_check_push(subflow->conn, sk);
>                 __mptcp_data_acked(subflow->conn);
>                 mptcp_data_unlock(subflow->conn);
> -               return;
> +               return true;
>         }
>
>         mptcp_get_options(sk, skb, &mp_opt);
> +
> +       /* The subflow can be in close state only if check_fully_established()
> +        * just sent a reset. If so, tell the caller to ignore the current packet.
> +        */
>         if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
> -               return;
> +               return sk->sk_state != TCP_CLOSE;
>
>         if (mp_opt.fastclose &&
>             msk->local_key == mp_opt.rcvr_key) {
> @@ -1100,7 +1105,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>         }
>
>         if (!mp_opt.dss)
> -               return;
> +               return true;
>
>         /* we can't wait for recvmsg() to update the ack_seq, otherwise
>          * monodirectional flows will stuck
> @@ -1119,12 +1124,12 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>                     schedule_work(&msk->work))
>                         sock_hold(subflow->conn);
>
> -               return;
> +               return true;
>         }
>
>         mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
>         if (!mpext)
> -               return;
> +               return true;
>
>         memset(mpext, 0, sizeof(*mpext));
>
> @@ -1153,6 +1158,8 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>                 if (mpext->csum_reqd)
>                         mpext->csum = mp_opt.csum;
>         }
> +

How about adding a label 'out:' here, and all above 'return true' can use
'goto out' instead. This is more like the kernel code style. WDYT?

-Geliang

> +       return true;
>  }
>
>  static void mptcp_set_rwin(const struct tcp_sock *tp)
> --
> 1.8.3.1
>
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset
  2021-06-28  8:35   ` Geliang Tang
@ 2021-06-28 16:45     ` Matthieu Baerts
  2021-06-29  2:54       ` Jianguo Wu
  2021-06-29  2:55     ` Jianguo Wu
  1 sibling, 1 reply; 10+ messages in thread
From: Matthieu Baerts @ 2021-06-28 16:45 UTC (permalink / raw)
  To: Geliang Tang, Jianguo Wu; +Cc: mptcp, Paolo Abeni, Mat Martineau

Hi Jianguo, Geliang,

On 28/06/2021 10:35, Geliang Tang wrote:
> Thank you for this new patch set!
> 
> There are three checkpatch.pl warnings in this patch set, please fix them.
> And a minor comment below.

As pointed out by Paolo on IRC, these modifications are required but
they are minor. Because it is already your v8 and your first major
series, we did these small modifications for you ;)

Note that we didn't change the "goto" just to return "true".
Also, I didn't add any Acked-by or RvB tags because none were given but
I can easily add them later if I get any but I guess Mat will send these
patches later to netdev with his SoB.

- 6bb69e9b2123: mptcp: fix warning in __skb_flow_dissect() when do syn
cookie for subflow join

- ee4bcc0a7a64: mptcp: remove redundant req destruct in subflow_check_req()

- f0056198a479: mptcp: fix syncookie process if mptcp can not_accept new
subflow

- b65e3f889d44: mptcp: avoid processing packet if a subflow reset

- fc66a740c7c7: selftests: mptcp: fix case multiple subflows limited by
server

- Results: 31aa314277b1..60851d13158d

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210628T164431
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210628T164431

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset
  2021-06-28 16:45     ` Matthieu Baerts
@ 2021-06-29  2:54       ` Jianguo Wu
  0 siblings, 0 replies; 10+ messages in thread
From: Jianguo Wu @ 2021-06-29  2:54 UTC (permalink / raw)
  To: Matthieu Baerts, Geliang Tang; +Cc: mptcp, Paolo Abeni, Mat Martineau



On 2021/6/29 0:45, Matthieu Baerts wrote:
> Hi Jianguo, Geliang,
> 
> On 28/06/2021 10:35, Geliang Tang wrote:
>> Thank you for this new patch set!
>>
>> There are three checkpatch.pl warnings in this patch set, please fix them.
>> And a minor comment below.
> 
> As pointed out by Paolo on IRC, these modifications are required but
> they are minor. Because it is already your v8 and your first major
> series, we did these small modifications for you ;)
> 
> Note that we didn't change the "goto" just to return "true".
> Also, I didn't add any Acked-by or RvB tags because none were given but
> I can easily add them later if I get any but I guess Mat will send these
> patches later to netdev with his SoB.
> 

Hi Matt,

Thanks for all your work :)

> - 6bb69e9b2123: mptcp: fix warning in __skb_flow_dissect() when do syn
> cookie for subflow join
> 
> - ee4bcc0a7a64: mptcp: remove redundant req destruct in subflow_check_req()
> 
> - f0056198a479: mptcp: fix syncookie process if mptcp can not_accept new
> subflow
> 
> - b65e3f889d44: mptcp: avoid processing packet if a subflow reset
> 
> - fc66a740c7c7: selftests: mptcp: fix case multiple subflows limited by
> server
> 
> - Results: 31aa314277b1..60851d13158d
> 
> Builds and tests are now in progress:
> 
> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210628T164431
> https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210628T164431
> 
> Cheers,
> Matt
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset
  2021-06-28  8:35   ` Geliang Tang
  2021-06-28 16:45     ` Matthieu Baerts
@ 2021-06-29  2:55     ` Jianguo Wu
  1 sibling, 0 replies; 10+ messages in thread
From: Jianguo Wu @ 2021-06-29  2:55 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp, Paolo Abeni, Mat Martineau

Hi Geliang,

On 2021/6/28 16:35, Geliang Tang wrote:
> Hi Jianguo,
> 
> Thank you for this new patch set!
> 
> There are three checkpatch.pl warnings in this patch set, please fix them.
> And a minor comment below.
> 

Thanks for your checking and comment, I should do more checking next time.

> ---------------------------------------------------------------
> 0001-mptcp-fix-warning-in-__skb_flow_dissect-when-do-syn-.patch
> ---------------------------------------------------------------
> WARNING: Possible unwrapped commit description (prefer a maximum 75
> chars per line)
> #7:
> I did stress test with wrk(https://github.com/wg/wrk) and
> webfsd(https://github.com/ourway/webfsd)
> 
> total: 0 errors, 1 warnings, 0 checks, 22 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> 0001-mptcp-fix-warning-in-__skb_flow_dissect-when-do-syn-.patch has
> style problems, please review.
> ---------------------------------------------------------------
> 0002-mptcp-remove-redundant-req-destruct-in-subflow_check.patch
> ---------------------------------------------------------------
> WARNING: Possible unwrapped commit description (prefer a maximum 75
> chars per line)
> #14:
> tcp_v4_reqsk_destructor() twice, and may double free inet_rsk(req)->ireq_opt.
> 
> total: 0 errors, 1 warnings, 0 checks, 11 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> 0002-mptcp-remove-redundant-req-destruct-in-subflow_check.patch has
> style problems, please review.
> ---------------------------------------------------------------
> 0005-selftests-mptcp-fix-case-multiple-subflows-limited-b.patch
> ---------------------------------------------------------------
> WARNING: Possible unwrapped commit description (prefer a maximum 75
> chars per line)
> #7:
> After patch "mptcp: fix syncookie process if mptcp can not_accept new subflow",
> 
> total: 0 errors, 1 warnings, 8 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> 0005-selftests-mptcp-fix-case-multiple-subflows-limited-b.patch has
> style problems, please review.
> 
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 
> 
> <wujianguo106@163.com> 于2021年6月26日周六 下午5:17写道:
>>
>> From: Jianguo Wu <wujianguo@chinatelecom.cn>
>>
>> If check_fully_established() causes a subflow reset, it should not
>> continue to process the packet in tcp_data_queue().
>> Add a return value to mptcp_incoming_options(), and return false if a
>> subflow has been reset, else return true. Then drop the packet in
>> tcp_data_queue()/tcp_rcv_state_process() if mptcp_incoming_options()
>> return false.
>>
>> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
>> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn>
>> ---
>>  include/net/mptcp.h  |  5 +++--
>>  net/ipv4/tcp_input.c | 19 +++++++++++++++----
>>  net/mptcp/options.c  | 19 +++++++++++++------
>>  3 files changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
>> index cb580b0..8b5af68 100644
>> --- a/include/net/mptcp.h
>> +++ b/include/net/mptcp.h
>> @@ -105,7 +105,7 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
>>  bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>>                                unsigned int *size, unsigned int remaining,
>>                                struct mptcp_out_options *opts);
>> -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb);
>> +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb);
>>
>>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>                          struct mptcp_out_options *opts);
>> @@ -227,9 +227,10 @@ static inline bool mptcp_established_options(struct sock *sk,
>>         return false;
>>  }
>>
>> -static inline void mptcp_incoming_options(struct sock *sk,
>> +static inline bool mptcp_incoming_options(struct sock *sk,
>>                                           struct sk_buff *skb)
>>  {
>> +       return true;
>>  }
>>
>>  static inline void mptcp_skb_ext_move(struct sk_buff *to,
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index 7d5e59f..4bacd7d 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -4247,6 +4247,9 @@ void tcp_reset(struct sock *sk, struct sk_buff *skb)
>>  {
>>         trace_tcp_receive_reset(sk);
>>
>> +       /* mptcp can't tell us to ignore reset pkts,
>> +        * so just ignore the return value of mptcp_incoming_options().
>> +        */
>>         if (sk_is_mptcp(sk))
>>                 mptcp_incoming_options(sk, skb);
>>
>> @@ -4941,8 +4944,13 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
>>         bool fragstolen;
>>         int eaten;
>>
>> -       if (sk_is_mptcp(sk))
>> -               mptcp_incoming_options(sk, skb);
>> +       /* If a subflow has been reset, the packet should not continue
>> +        * to be processed, drop the packet.
>> +        */
>> +       if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) {
>> +               __kfree_skb(skb);
>> +               return;
>> +       }
>>
>>         if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) {
>>                 __kfree_skb(skb);
>> @@ -6523,8 +6531,11 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
>>         case TCP_CLOSING:
>>         case TCP_LAST_ACK:
>>                 if (!before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) {
>> -                       if (sk_is_mptcp(sk))
>> -                               mptcp_incoming_options(sk, skb);
>> +                       /* If a subflow has been reset, the packet should not
>> +                        * continue to be processed, drop the packet.
>> +                        */
>> +                       if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb))
>> +                               goto discard;
>>                         break;
>>                 }
>>                 fallthrough;
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index b5850af..4452455 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -1035,7 +1035,8 @@ static bool add_addr_hmac_valid(struct mptcp_sock *msk,
>>         return hmac == mp_opt->ahmac;
>>  }
>>
>> -void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>> +/* Return false if a subflow has been reset, else return true */
>> +bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>  {
>>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>         struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>> @@ -1053,12 +1054,16 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>                         __mptcp_check_push(subflow->conn, sk);
>>                 __mptcp_data_acked(subflow->conn);
>>                 mptcp_data_unlock(subflow->conn);
>> -               return;
>> +               return true;
>>         }
>>
>>         mptcp_get_options(sk, skb, &mp_opt);
>> +
>> +       /* The subflow can be in close state only if check_fully_established()
>> +        * just sent a reset. If so, tell the caller to ignore the current packet.
>> +        */
>>         if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
>> -               return;
>> +               return sk->sk_state != TCP_CLOSE;
>>
>>         if (mp_opt.fastclose &&
>>             msk->local_key == mp_opt.rcvr_key) {
>> @@ -1100,7 +1105,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>         }
>>
>>         if (!mp_opt.dss)
>> -               return;
>> +               return true;
>>
>>         /* we can't wait for recvmsg() to update the ack_seq, otherwise
>>          * monodirectional flows will stuck
>> @@ -1119,12 +1124,12 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>                     schedule_work(&msk->work))
>>                         sock_hold(subflow->conn);
>>
>> -               return;
>> +               return true;
>>         }
>>
>>         mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
>>         if (!mpext)
>> -               return;
>> +               return true;
>>
>>         memset(mpext, 0, sizeof(*mpext));
>>
>> @@ -1153,6 +1158,8 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>                 if (mpext->csum_reqd)
>>                         mpext->csum = mp_opt.csum;
>>         }
>> +
> 
> How about adding a label 'out:' here, and all above 'return true' can use
> 'goto out' instead. This is more like the kernel code style. WDYT?
> 
> -Geliang
> 
>> +       return true;
>>  }
>>
>>  static void mptcp_set_rwin(const struct tcp_sock *tp)
>> --
>> 1.8.3.1
>>
>>
>>


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-06-29  3:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-26  9:16 [PATCH mptcp-net v8 0/5] Fix some mptcp syncookie process bugs wujianguo106
2021-06-26  9:16 ` [PATCH mptcp-net v8 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
2021-06-26  9:16 ` [PATCH mptcp-net v8 2/5] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106
2021-06-26  9:16 ` [PATCH mptcp-net v8 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106
2021-06-26  9:16 ` [PATCH mptcp-net v8 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
2021-06-28  8:35   ` Geliang Tang
2021-06-28 16:45     ` Matthieu Baerts
2021-06-29  2:54       ` Jianguo Wu
2021-06-29  2:55     ` Jianguo Wu
2021-06-26  9:16 ` [PATCH mptcp-net v8 5/5] selftests: mptcp: fix case multiple subflows limited by server wujianguo106

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).