[PATCH mptcp-net v7 0/5] Fix some mptcp syncookie process bugs

[PATCH mptcp-net v7 0/5] Fix some mptcp syncookie process bugs

[wujianguo106]

From: wujianguo <wujianguo@chinatelecom.cn>

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                             | 22 ++++++++++++++--------
 net/mptcp/subflow.c                             | 11 +++--------
 net/mptcp/syncookies.c                          | 16 +++++++++++++++-
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  2 +-
 6 files changed, 51 insertions(+), 24 deletions(-)

-- 
1.8.3.1

[PATCH mptcp-net v7 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join

[wujianguo106]

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

[PATCH mptcp-net v7 2/5] 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 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

[PATCH mptcp-net v7 3/5] 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 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

[PATCH mptcp-net v7 4/5] 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().
Add a return value to mptcp_incoming_options(), and return 0 if a
subflow has been reset, else return 1. Then drop the packet in
tcp_data_queue()/tcp_rcv_state_process() if mptcp_incoming_options()
return 0.

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  | 22 ++++++++++++++--------
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index cb580b0..cbd511c 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);
+int 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 int mptcp_incoming_options(struct sock *sk,
 					  struct sk_buff *skb)
 {
+	return 1;
 }
 
 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..f4842b5 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -856,7 +856,8 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
 static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 				    struct mptcp_subflow_context *subflow,
 				    struct sk_buff *skb,
-				    struct mptcp_options_received *mp_opt)
+				    struct mptcp_options_received *mp_opt,
+				    bool *subflow_is_rst)
 {
 	/* here we can process OoO, in-window pkts, only in-sequence 4th ack
 	 * will make the subflow fully established
@@ -938,6 +939,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 	return true;
 
 reset:
+	*subflow_is_rst = true;
 	mptcp_subflow_reset(ssk);
 	return false;
 }
@@ -1035,12 +1037,14 @@ 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 0 if a subflow has been reset, else return 1 */
+int 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);
 	struct mptcp_options_received mp_opt;
 	struct mptcp_ext *mpext;
+	bool subflow_is_rst = false;
 
 	if (__mptcp_check_fallback(msk)) {
 		/* Keep it simple and unconditionally trigger send data cleanup and
@@ -1053,12 +1057,12 @@ 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 1;
 	}
 
 	mptcp_get_options(sk, skb, &mp_opt);
-	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
-		return;
+	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt, &subflow_is_rst))
+		return subflow_is_rst ? 0 : 1;
 
 	if (mp_opt.fastclose &&
 	    msk->local_key == mp_opt.rcvr_key) {
@@ -1100,7 +1104,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 	}
 
 	if (!mp_opt.dss)
-		return;
+		return 1;
 
 	/* we can't wait for recvmsg() to update the ack_seq, otherwise
 	 * monodirectional flows will stuck
@@ -1119,12 +1123,12 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		    schedule_work(&msk->work))
 			sock_hold(subflow->conn);
 
-		return;
+		return 1;
 	}
 
 	mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
 	if (!mpext)
-		return;
+		return 1;
 
 	memset(mpext, 0, sizeof(*mpext));
 
@@ -1153,6 +1157,8 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		if (mpext->csum_reqd)
 			mpext->csum = mp_opt.csum;
 	}
+
+	return 1;
 }
 
 static void mptcp_set_rwin(const struct tcp_sock *tp)
-- 
1.8.3.1

[PATCH mptcp-net v7 5/5] selftests: mptcp: fix case multiple subflows limited by server

[wujianguo106]

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

Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset

[Geliang Tang]

Hi Jianguo,

Thank you for this patchset!

<wujianguo106@163.com> 于2021年6月25日周五 下午4:26写道：
>
> 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 0 if a
> subflow has been reset, else return 1. Then drop the packet in
> tcp_data_queue()/tcp_rcv_state_process() if mptcp_incoming_options()
> return 0.
>
> 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  | 22 ++++++++++++++--------
>  3 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index cb580b0..cbd511c 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);
> +int 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 int mptcp_incoming_options(struct sock *sk,
>                                           struct sk_buff *skb)
>  {
> +       return 1;
>  }
>
>  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..f4842b5 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -856,7 +856,8 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
>  static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>                                     struct mptcp_subflow_context *subflow,
>                                     struct sk_buff *skb,
> -                                   struct mptcp_options_received *mp_opt)
> +                                   struct mptcp_options_received *mp_opt,
> +                                   bool *subflow_is_rst)
>  {
>         /* here we can process OoO, in-window pkts, only in-sequence 4th ack
>          * will make the subflow fully established
> @@ -938,6 +939,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>         return true;
>
>  reset:
> +       *subflow_is_rst = true;
>         mptcp_subflow_reset(ssk);
>         return false;
>  }
> @@ -1035,12 +1037,14 @@ 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 0 if a subflow has been reset, else return 1 */

How about returning a bool here, return true or return false?

-Geliang

> +int 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);
>         struct mptcp_options_received mp_opt;
>         struct mptcp_ext *mpext;
> +       bool subflow_is_rst = false;
>
>         if (__mptcp_check_fallback(msk)) {
>                 /* Keep it simple and unconditionally trigger send data cleanup and
> @@ -1053,12 +1057,12 @@ 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 1;
>         }
>
>         mptcp_get_options(sk, skb, &mp_opt);
> -       if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
> -               return;
> +       if (!check_fully_established(msk, sk, subflow, skb, &mp_opt, &subflow_is_rst))
> +               return subflow_is_rst ? 0 : 1;
>
>         if (mp_opt.fastclose &&
>             msk->local_key == mp_opt.rcvr_key) {
> @@ -1100,7 +1104,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>         }
>
>         if (!mp_opt.dss)
> -               return;
> +               return 1;
>
>         /* we can't wait for recvmsg() to update the ack_seq, otherwise
>          * monodirectional flows will stuck
> @@ -1119,12 +1123,12 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>                     schedule_work(&msk->work))
>                         sock_hold(subflow->conn);
>
> -               return;
> +               return 1;
>         }
>
>         mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
>         if (!mpext)
> -               return;
> +               return 1;
>
>         memset(mpext, 0, sizeof(*mpext));
>
> @@ -1153,6 +1157,8 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>                 if (mpext->csum_reqd)
>                         mpext->csum = mp_opt.csum;
>         }
> +
> +       return 1;
>  }
>
>  static void mptcp_set_rwin(const struct tcp_sock *tp)
> --
> 1.8.3.1
>
>
>

Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset

[Matthieu Baerts]

Hi Jianguo,

Thank you for working on that!

On 25/06/2021 10:25, 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().
> Add a return value to mptcp_incoming_options(), and return 0 if a
> subflow has been reset, else return 1.

A small detail but it looks strange to me to return +1.
Maybe clearer to return -1 in case of error or return a boolean?

If you decide to return -1, please check for '< 0':

  if (mptcp_incoming_options(...) < 0) // error: we discard
      goto discard;

and not:

  if (mptcp_incoming_options(...)) // no error but we discard??
      goto discard;

(Or replace subflow_is_rst by subflow_is_accepted and return this
variable but it might be strange to return that if you don't have a DSS,
etc.)

But if it is only me who find "strange" to return 0/+1, fine not to
change but it looks uncommon and maybe a source of misinterpretation :)

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

Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset

[Jianguo Wu]

On 2021/6/25 17:07, Matthieu Baerts wrote:
> Hi Jianguo,
> 
> Thank you for working on that!
> 
> On 25/06/2021 10:25, 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().
>> Add a return value to mptcp_incoming_options(), and return 0 if a
>> subflow has been reset, else return 1.
> 
> A small detail but it looks strange to me to return +1.
> Maybe clearer to return -1 in case of error or return a boolean?
> 

Hi Mat and Geliang,

Based on your comments, I will use boolean type as return value, return false if a subflow has been reset.
Thanks for your review!

> If you decide to return -1, please check for '< 0':
> 
>   if (mptcp_incoming_options(...) < 0) // error: we discard
>       goto discard;
> 
> and not:
> 
>   if (mptcp_incoming_options(...)) // no error but we discard??
>       goto discard;
> 
> (Or replace subflow_is_rst by subflow_is_accepted and return this
> variable but it might be strange to return that if you don't have a DSS,
> etc.)
> 
> But if it is only me who find "strange" to return 0/+1, fine not to
> change but it looks uncommon and maybe a source of misinterpretation :)
> 
> Cheers,
> Matt
> 

Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset

[Paolo Abeni]

On Fri, 2021-06-25 at 16:25 +0800, wujianguo106@163.com wrote:
> @@ -1035,12 +1037,14 @@ 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 0 if a subflow has been reset, else return 1 */
> +int 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);
>  	struct mptcp_options_received mp_opt;
>  	struct mptcp_ext *mpext;
> +	bool subflow_is_rst = false;

Additional small detail: please use the reverse xmas tree order for
variables definition.

Thanks!

/P

Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset

[Paolo Abeni]

Hi,

I'm sorry for the partial feedback in my previous reply.

On Fri, 2021-06-25 at 16:25 +0800, wujianguo106@163.com wrote:
> @@ -856,7 +856,8 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
>  static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>  				    struct mptcp_subflow_context *subflow,
>  				    struct sk_buff *skb,
> -				    struct mptcp_options_received *mp_opt)
> +				    struct mptcp_options_received *mp_opt,
> +				    bool *subflow_is_rst)

This additional argument is not needed...

[...]
> @@ -1053,12 +1057,12 @@ 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 1;
>  	}
>  
>  	mptcp_get_options(sk, skb, &mp_opt);
> -	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
> -		return;
> +	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt, &subflow_is_rst))
> +		return subflow_is_rst ? 0 : 1;


... here you can simply:

		return sk->sk_state != TCP_CLOSE;

plus some comment above alike:

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

/P

Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset

[Mat Martineau]

On Fri, 25 Jun 2021, Jianguo Wu wrote:

>
>
> On 2021/6/25 17:07, Matthieu Baerts wrote:
>> Hi Jianguo,
>>
>> Thank you for working on that!
>>
>> On 25/06/2021 10:25, 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().
>>> Add a return value to mptcp_incoming_options(), and return 0 if a
>>> subflow has been reset, else return 1.
>>
>> A small detail but it looks strange to me to return +1.
>> Maybe clearer to return -1 in case of error or return a boolean?
>>
>
> Hi Mat and Geliang,
>
> Based on your comments, I will use boolean type as return value, return false if a subflow has been reset.
> Thanks for your review!

Thanks - I think the 'bool' fits better here than -1/0.

-Mat


>
>> If you decide to return -1, please check for '< 0':
>>
>>   if (mptcp_incoming_options(...) < 0) // error: we discard
>>       goto discard;
>>
>> and not:
>>
>>   if (mptcp_incoming_options(...)) // no error but we discard??
>>       goto discard;
>>
>> (Or replace subflow_is_rst by subflow_is_accepted and return this
>> variable but it might be strange to return that if you don't have a DSS,
>> etc.)
>>
>> But if it is only me who find "strange" to return 0/+1, fine not to
>> change but it looks uncommon and maybe a source of misinterpretation :)
>>
>> Cheers,
>> Matt
>>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset

[Jianguo Wu]

Hi Paolo,

On 2021/6/25 18:45, Paolo Abeni wrote:
> Hi,
> 
> I'm sorry for the partial feedback in my previous reply.
> 
> On Fri, 2021-06-25 at 16:25 +0800, wujianguo106@163.com wrote:
>> @@ -856,7 +856,8 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
>>  static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>>  				    struct mptcp_subflow_context *subflow,
>>  				    struct sk_buff *skb,
>> -				    struct mptcp_options_received *mp_opt)
>> +				    struct mptcp_options_received *mp_opt,
>> +				    bool *subflow_is_rst)
> 
> This additional argument is not needed...
> 
> [...]
>> @@ -1053,12 +1057,12 @@ 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 1;
>>  	}
>>  
>>  	mptcp_get_options(sk, skb, &mp_opt);
>> -	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
>> -		return;
>> +	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt, &subflow_is_rst))
>> +		return subflow_is_rst ? 0 : 1;
> 
> 
> ... here you can simply:
> 
> 		return sk->sk_state != TCP_CLOSE;
> 
> plus some comment above alike:
> 
> """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"""
> 

Thank you for all your reviews and suggestions！New version coming soon.

> /P
>