| Message ID | 1034de3d-5528-ea65-6deb-8a67955f1042@163.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | [1/3] mptcp: fix warning in __skb_flow_dissect() when do syn cookie for subflow join | expand |
Hello, On Wed, 2021-06-09 at 18:39 +0800, Jianguo Wu wrote: > From: Jianguo Wu <wujianguo@chinatelecom.cn> > > Lots of "TCP: tcp_fin: Impossible, sk->sk_state=7" in client side when doing stress testing. > There are at least two cases may trigger this warning: > 1. mptcp is in syncookie, and server recv MP_JOIN SYN request, in subflow_check_req(), > the mptcp_can_accept_new_subflow() return false, so subflow_init_req_cookie_join_save() > isn't called, i.e. not store the data present in the MP_JOIN syn request and the random > nonce in hash table - join_entries[], but still send synack. When recv 3rd-ack, > mptcp_token_join_cookie_init_state() will return false, and 3rd-ack is dropped, then if mptcp > conn is closed by client, client will send a DATA_FIN and a MPTCP FIN, the DATA_FIN doesn't have > MP_CAPABLE or MP_JOIN, so mptcp_subflow_init_cookie_req() will return 0, and pass the cookie check, > MP_JOIN request is fallback to normal TCP. Server will send a TCP FIN if closed, in client side, > when process TCP FIN, it will do reset, the code path is: > tcp_data_queue()->mptcp_incoming_options()->check_fully_established()->mptcp_subflow_reset(). > mptcp_subflow_reset() will set sock state to TCP_CLOSE, so tcp_fin will hit TCP_CLOSE, and print the warning. > 2. mptcp is in syncookie, and server recv 3rd-ack, in mptcp_subflow_init_cookie_req(), mptcp_can_accept_new_subflow() > return false, and subflow_req->mp_join is not set to 1, so in subflow_syn_recv_sock() will not reset the MP_JOIN > subflow, but fallback to normal TCP, and then the same thing happens when server will send a TCP FIN if closed. Minor nit: each line in the commit message should not be longer than 72 chars. > For case1, subflow_check_req() return -EPERM, then tcp_conn_request() will drop MP_JOIN SYN. > For case2, let subflow_syn_recv_sock() do mptcp_can_accept_new_subflow() check, and do fatal fallback, send reset. Possibly worthy 2 separate patches. pktdrill test cases would help. I still have to wrap my mind on the above, still I have a relevant comment, see below... > And do sanity check in tcp_data_queue(). > > Fixes: 9466a1ccebbe("mptcp: enable JOIN requests even if cookies are in use") > Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> > --- > net/ipv4/tcp_input.c | 7 ++++++- > net/mptcp/subflow.c | 6 +++--- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 7d5e59f..537f24a 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -4941,8 +4941,13 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb) > bool fragstolen; > int eaten; > > - if (sk_is_mptcp(sk)) > + if (sk_is_mptcp(sk)) { > mptcp_incoming_options(sk, skb); > + if (sk->sk_state == TCP_CLOSE) { > + __kfree_skb(skb); > + return; > + } > + } _if_ I read correctly the commit message, this is needed to avoid processing FIN packets on when check_fully_established()/check_fully_established() causes a subflow reset. I think we can handle the above in the MPTCP code - in check_fully_established() - setting: TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; so that the following check will drop the pkt: > if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { > __kfree_skb(skb); > /P
Hi Paolo, On 2021/6/9 22:18, Paolo Abeni wrote: > Hello, > > On Wed, 2021-06-09 at 18:39 +0800, Jianguo Wu wrote: >> From: Jianguo Wu <wujianguo@chinatelecom.cn> >> >> Lots of "TCP: tcp_fin: Impossible, sk->sk_state=7" in client side when doing stress testing. >> There are at least two cases may trigger this warning: >> 1. mptcp is in syncookie, and server recv MP_JOIN SYN request, in subflow_check_req(), >> the mptcp_can_accept_new_subflow() return false, so subflow_init_req_cookie_join_save() >> isn't called, i.e. not store the data present in the MP_JOIN syn request and the random >> nonce in hash table - join_entries[], but still send synack. When recv 3rd-ack, >> mptcp_token_join_cookie_init_state() will return false, and 3rd-ack is dropped, then if mptcp >> conn is closed by client, client will send a DATA_FIN and a MPTCP FIN, the DATA_FIN doesn't have >> MP_CAPABLE or MP_JOIN, so mptcp_subflow_init_cookie_req() will return 0, and pass the cookie check, >> MP_JOIN request is fallback to normal TCP. Server will send a TCP FIN if closed, in client side, >> when process TCP FIN, it will do reset, the code path is: >> tcp_data_queue()->mptcp_incoming_options()->check_fully_established()->mptcp_subflow_reset(). >> mptcp_subflow_reset() will set sock state to TCP_CLOSE, so tcp_fin will hit TCP_CLOSE, and print the warning. >> 2. mptcp is in syncookie, and server recv 3rd-ack, in mptcp_subflow_init_cookie_req(), mptcp_can_accept_new_subflow() >> return false, and subflow_req->mp_join is not set to 1, so in subflow_syn_recv_sock() will not reset the MP_JOIN >> subflow, but fallback to normal TCP, and then the same thing happens when server will send a TCP FIN if closed. > > Minor nit: each line in the commit message should not be longer than 72 > chars. > Ok, I will be more carefully with this. >> For case1, subflow_check_req() return -EPERM, then tcp_conn_request() will drop MP_JOIN SYN. >> For case2, let subflow_syn_recv_sock() do mptcp_can_accept_new_subflow() check, and do fatal fallback, send reset. > > Possibly worthy 2 separate patches. > > pktdrill test cases would help. > > I still have to wrap my mind on the above, still I have a relevant > comment, see below... > >> And do sanity check in tcp_data_queue(). >> >> Fixes: 9466a1ccebbe("mptcp: enable JOIN requests even if cookies are in use") >> Signed-off-by: Jianguo Wu <wujianguo@chinatelecom.cn> >> --- >> net/ipv4/tcp_input.c | 7 ++++++- >> net/mptcp/subflow.c | 6 +++--- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c >> index 7d5e59f..537f24a 100644 >> --- a/net/ipv4/tcp_input.c >> +++ b/net/ipv4/tcp_input.c >> @@ -4941,8 +4941,13 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb) >> bool fragstolen; >> int eaten; >> >> - if (sk_is_mptcp(sk)) >> + if (sk_is_mptcp(sk)) { >> mptcp_incoming_options(sk, skb); >> + if (sk->sk_state == TCP_CLOSE) { >> + __kfree_skb(skb); >> + return; >> + } >> + } > > _if_ I read correctly the commit message, this is needed to avoid > processing FIN packets on > when check_fully_established()/check_fully_established() causes a > subflow reset. > Yes > I think we can handle the above in the MPTCP code - in > check_fully_established() - setting: > > TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; > I will use this. > so that the following check will drop the pkt: > >> if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { >> __kfree_skb(skb); >> > > /P >
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 7d5e59f..537f24a 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -4941,8 +4941,13 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb) bool fragstolen; int eaten; - if (sk_is_mptcp(sk)) + if (sk_is_mptcp(sk)) { mptcp_incoming_options(sk, skb); + if (sk->sk_state == TCP_CLOSE) { + __kfree_skb(skb); + return; + } + } if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { __kfree_skb(skb); diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 75ed530..6d98e19 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -224,6 +224,8 @@ static int subflow_check_req(struct request_sock *req, if (unlikely(req->syncookie)) { if (mptcp_can_accept_new_subflow(subflow_req->msk)) subflow_init_req_cookie_join_save(subflow_req, skb); + else + return -EPERM; } pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token, @@ -263,9 +265,7 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req, if (!mptcp_token_join_cookie_init_state(subflow_req, skb)) return -EINVAL; - if (mptcp_can_accept_new_subflow(subflow_req->msk)) - subflow_req->mp_join = 1; - + subflow_req->mp_join = 1; subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq - 1; }