* [PATCH mptcp-net v7 0/5] Fix some mptcp syncookie process bugs
@ 2021-06-25 8:25 wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: wujianguo106 @ 2021-06-25 8:25 UTC (permalink / raw)
To: mptcp; +Cc: pabeni, mathew.j.martineau
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH mptcp-net v7 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join
2021-06-25 8:25 [PATCH mptcp-net v7 0/5] Fix some mptcp syncookie process bugs wujianguo106
@ 2021-06-25 8:25 ` wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 2/5] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: wujianguo106 @ 2021-06-25 8:25 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] 13+ messages in thread
* [PATCH mptcp-net v7 2/5] mptcp: remove redundant req destruct in subflow_check_req()
2021-06-25 8:25 [PATCH mptcp-net v7 0/5] Fix some mptcp syncookie process bugs wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
@ 2021-06-25 8:25 ` wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: wujianguo106 @ 2021-06-25 8:25 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] 13+ messages in thread
* [PATCH mptcp-net v7 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow
2021-06-25 8:25 [PATCH mptcp-net v7 0/5] Fix some mptcp syncookie process bugs wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 2/5] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106
@ 2021-06-25 8:25 ` wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 5/5] selftests: mptcp: fix case multiple subflows limited by server wujianguo106
4 siblings, 0 replies; 13+ messages in thread
From: wujianguo106 @ 2021-06-25 8:25 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] 13+ messages in thread
* [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset
2021-06-25 8:25 [PATCH mptcp-net v7 0/5] Fix some mptcp syncookie process bugs wujianguo106
` (2 preceding siblings ...)
2021-06-25 8:25 ` [PATCH mptcp-net v7 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106
@ 2021-06-25 8:25 ` wujianguo106
2021-06-25 8:58 ` Geliang Tang
` (3 more replies)
2021-06-25 8:25 ` [PATCH mptcp-net v7 5/5] selftests: mptcp: fix case multiple subflows limited by server wujianguo106
4 siblings, 4 replies; 13+ messages in thread
From: wujianguo106 @ 2021-06-25 8:25 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 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH mptcp-net v7 5/5] selftests: mptcp: fix case multiple subflows limited by server
2021-06-25 8:25 [PATCH mptcp-net v7 0/5] Fix some mptcp syncookie process bugs wujianguo106
` (3 preceding siblings ...)
2021-06-25 8:25 ` [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
@ 2021-06-25 8:25 ` wujianguo106
4 siblings, 0 replies; 13+ messages in thread
From: wujianguo106 @ 2021-06-25 8:25 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] 13+ messages in thread
* Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset
2021-06-25 8:25 ` [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
@ 2021-06-25 8:58 ` Geliang Tang
2021-06-25 9:07 ` Matthieu Baerts
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Geliang Tang @ 2021-06-25 8:58 UTC (permalink / raw)
To: Jianguo Wu; +Cc: mptcp, Paolo Abeni, Mat Martineau
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
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset
2021-06-25 8:25 ` [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
2021-06-25 8:58 ` Geliang Tang
@ 2021-06-25 9:07 ` Matthieu Baerts
2021-06-25 9:46 ` Jianguo Wu
2021-06-25 10:38 ` Paolo Abeni
2021-06-25 10:45 ` Paolo Abeni
3 siblings, 1 reply; 13+ messages in thread
From: Matthieu Baerts @ 2021-06-25 9:07 UTC (permalink / raw)
To: wujianguo106, mptcp; +Cc: pabeni, mathew.j.martineau
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset
2021-06-25 9:07 ` Matthieu Baerts
@ 2021-06-25 9:46 ` Jianguo Wu
2021-06-26 1:07 ` Mat Martineau
0 siblings, 1 reply; 13+ messages in thread
From: Jianguo Wu @ 2021-06-25 9:46 UTC (permalink / raw)
To: Matthieu Baerts, mptcp; +Cc: pabeni, mathew.j.martineau, Geliang Tang
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset
2021-06-25 8:25 ` [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
2021-06-25 8:58 ` Geliang Tang
2021-06-25 9:07 ` Matthieu Baerts
@ 2021-06-25 10:38 ` Paolo Abeni
2021-06-25 10:45 ` Paolo Abeni
3 siblings, 0 replies; 13+ messages in thread
From: Paolo Abeni @ 2021-06-25 10:38 UTC (permalink / raw)
To: wujianguo106, mptcp; +Cc: mathew.j.martineau
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset
2021-06-25 8:25 ` [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
` (2 preceding siblings ...)
2021-06-25 10:38 ` Paolo Abeni
@ 2021-06-25 10:45 ` Paolo Abeni
2021-06-26 8:59 ` Jianguo Wu
3 siblings, 1 reply; 13+ messages in thread
From: Paolo Abeni @ 2021-06-25 10:45 UTC (permalink / raw)
To: wujianguo106, mptcp; +Cc: mathew.j.martineau
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset
2021-06-25 9:46 ` Jianguo Wu
@ 2021-06-26 1:07 ` Mat Martineau
0 siblings, 0 replies; 13+ messages in thread
From: Mat Martineau @ 2021-06-26 1:07 UTC (permalink / raw)
To: Jianguo Wu; +Cc: Matthieu Baerts, mptcp, pabeni, Geliang Tang
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset
2021-06-25 10:45 ` Paolo Abeni
@ 2021-06-26 8:59 ` Jianguo Wu
0 siblings, 0 replies; 13+ messages in thread
From: Jianguo Wu @ 2021-06-26 8:59 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: mathew.j.martineau
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-06-26 9:15 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-25 8:25 [PATCH mptcp-net v7 0/5] Fix some mptcp syncookie process bugs wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 1/5] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 2/5] mptcp: remove redundant req destruct in subflow_check_req() wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 3/5] mptcp: fix syncookie process if mptcp can not_accept new subflow wujianguo106
2021-06-25 8:25 ` [PATCH mptcp-net v7 4/5] mptcp: avoid processing packet if a subflow reset wujianguo106
2021-06-25 8:58 ` Geliang Tang
2021-06-25 9:07 ` Matthieu Baerts
2021-06-25 9:46 ` Jianguo Wu
2021-06-26 1:07 ` Mat Martineau
2021-06-25 10:38 ` Paolo Abeni
2021-06-25 10:45 ` Paolo Abeni
2021-06-26 8:59 ` Jianguo Wu
2021-06-25 8:25 ` [PATCH mptcp-net v7 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).