| Message ID | 20240212092827.75378-4-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | introduce dropreasons in tcp receive path | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/apply | fail | Patch does not apply to net-next |
On Mon, Feb 12, 2024 at 10:29 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > From: Jason Xing <kernelxing@tencent.com> > if (!acceptable) > - return 1; > + /* This reason isn't clear. We can refine it in the future */ > + return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE; tcp_conn_request() might return 0 when a syncookie has been generated. Technically speaking, the incoming SYN was not dropped :) I think you need to have a patch to change tcp_conn_request() and its friends to return a 'refined' drop_reason to avoid future questions / patches.
Hello Eric, On Mon, Feb 12, 2024 at 11:33 PM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Feb 12, 2024 at 10:29 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > From: Jason Xing <kernelxing@tencent.com> > > > > if (!acceptable) > > - return 1; > > + /* This reason isn't clear. We can refine it in the future */ > > + return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE; > > tcp_conn_request() might return 0 when a syncookie has been generated. > > Technically speaking, the incoming SYN was not dropped :) > > I think you need to have a patch to change tcp_conn_request() and its > friends to return a 'refined' drop_reason > to avoid future questions / patches. Thanks for your advice. Sure. That's on my to-do list since Kuniyuki pointed out[1] this before. I will get it started as soon as the current two patchsets are reviewed. For now, I think, what I wrote doesn't change the old behaviour, right ? [1]: https://lore.kernel.org/netdev/20240209091454.32323-1-kuniyu@amazon.com/ Thanks, Jason
On Mon, Feb 12, 2024 at 4:53 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > Hello Eric, > > On Mon, Feb 12, 2024 at 11:33 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Mon, Feb 12, 2024 at 10:29 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > > > From: Jason Xing <kernelxing@tencent.com> > > > > > > > if (!acceptable) > > > - return 1; > > > + /* This reason isn't clear. We can refine it in the future */ > > > + return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE; > > > > tcp_conn_request() might return 0 when a syncookie has been generated. > > > > Technically speaking, the incoming SYN was not dropped :) > > > > I think you need to have a patch to change tcp_conn_request() and its > > friends to return a 'refined' drop_reason > > to avoid future questions / patches. > > Thanks for your advice. > > Sure. That's on my to-do list since Kuniyuki pointed out[1] this > before. I will get it started as soon as the current two patchsets are > reviewed. For now, I think, what I wrote doesn't change the old > behaviour, right ? > Lets not add a drop_reason that will soon be obsolete.
On Tue, Feb 13, 2024 at 12:19 AM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Feb 12, 2024 at 4:53 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > Hello Eric, > > > > On Mon, Feb 12, 2024 at 11:33 PM Eric Dumazet <edumazet@google.com> wrote: > > > > > > On Mon, Feb 12, 2024 at 10:29 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > > > > > From: Jason Xing <kernelxing@tencent.com> > > > > > > > > > > if (!acceptable) > > > > - return 1; > > > > + /* This reason isn't clear. We can refine it in the future */ > > > > + return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE; > > > > > > tcp_conn_request() might return 0 when a syncookie has been generated. > > > > > > Technically speaking, the incoming SYN was not dropped :) > > > > > > I think you need to have a patch to change tcp_conn_request() and its > > > friends to return a 'refined' drop_reason > > > to avoid future questions / patches. > > > > Thanks for your advice. > > > > Sure. That's on my to-do list since Kuniyuki pointed out[1] this > > before. I will get it started as soon as the current two patchsets are > > reviewed. For now, I think, what I wrote doesn't change the old > > behaviour, right ? > > > > Lets not add a drop_reason that will soon be obsolete. I will update it(add one or more patches) in the v4 patchset :) Thanks, Jason
On Mon, Feb 12, 2024 at 11:33 PM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Feb 12, 2024 at 10:29 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > From: Jason Xing <kernelxing@tencent.com> > > > > if (!acceptable) > > - return 1; > > + /* This reason isn't clear. We can refine it in the future */ > > + return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE; > > tcp_conn_request() might return 0 when a syncookie has been generated. > > Technically speaking, the incoming SYN was not dropped :) Hi Eric, Kuniyuki Sorry, I should have checked tcp_conn_request() carefully last night. Today, I checked tcp_conn_request() over and over again. I didn't find there is any chance to return a negative/positive value, only 0. It means @acceptable is always true and it should never return TCP_CONNREQNOTACCEPTABLE for TCP ipv4/6 protocol and never trigger a reset in this way. For DCCP, there are chances to return -1 in dccp_v4_conn_request(). But I don't think we've already added drop reasons in DCCP before. If I understand correctly, there is no need to do any refinement or even introduce TCP_CONNREQNOTACCEPTABLE new dropreason about the .conn_request() for TCP. Should I add a NEW kfree_skb_reason() in tcp_conn_request() for those labels, like drop_and_release, drop_and_free, drop, and not return a drop reason to its caller tcp_rcv_state_process()? Please correct me if I'm wrong... Thanks, Jason > > I think you need to have a patch to change tcp_conn_request() and its > friends to return a 'refined' drop_reason > to avoid future questions / patches.
From: Jason Xing <kerneljasonxing@gmail.com> Date: Tue, 13 Feb 2024 09:48:04 +0800 > On Mon, Feb 12, 2024 at 11:33 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Mon, Feb 12, 2024 at 10:29 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > > > From: Jason Xing <kernelxing@tencent.com> > > > > > > > if (!acceptable) > > > - return 1; > > > + /* This reason isn't clear. We can refine it in the future */ > > > + return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE; > > > > tcp_conn_request() might return 0 when a syncookie has been generated. > > > > Technically speaking, the incoming SYN was not dropped :) > > Hi Eric, Kuniyuki > > Sorry, I should have checked tcp_conn_request() carefully last night. > Today, I checked tcp_conn_request() over and over again. > > I didn't find there is any chance to return a negative/positive value, > only 0. It means @acceptable is always true and it should never return > TCP_CONNREQNOTACCEPTABLE for TCP ipv4/6 protocol and never trigger a > reset in this way. Ah right, I remember I digged the same thing before and even in the initial commit, conn_request() always returned 0 and tcp_rcv_state_process() tested it with if (retval < 0). I think we can clean up the leftover with some comments above ->conn_request() definition so that we can convert it to void when we deprecate DCCP in the near future. > > For DCCP, there are chances to return -1 in dccp_v4_conn_request(). > But I don't think we've already added drop reasons in DCCP before. > > If I understand correctly, there is no need to do any refinement or > even introduce TCP_CONNREQNOTACCEPTABLE new dropreason about the > .conn_request() for TCP. > > Should I add a NEW kfree_skb_reason() in tcp_conn_request() for those > labels, like drop_and_release, drop_and_free, drop, and not return a > drop reason to its caller tcp_rcv_state_process()? Most interested reasons will be covered by - reqsk q : net_info_ratelimited() in tcp_syn_flood_action() or net_dbg_ratelimited() in pr_drop_req() or __NET_INC_STATS(net, LINUX_MIB_LISTENDROPS) in tcp_listendrop() - accept q: NET_INC_STATS(net, LINUX_MIB_LISTENOVERFLOWS) or __NET_INC_STATS(net, LINUX_MIB_LISTENDROPS) in tcp_listendrop() and could be refined by drop reason, but I'm not sure if drop reason is used under such a pressured situation. Also, these failures are now treated with consume_skb(). Whichever is fine to me, no strong preference. > > Please correct me if I'm wrong... > > Thanks, > Jason > > > > > I think you need to have a patch to change tcp_conn_request() and its > > friends to return a 'refined' drop_reason > > to avoid future questions / patches.
On Tue, Feb 13, 2024 at 12:07 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > From: Jason Xing <kerneljasonxing@gmail.com> > Date: Tue, 13 Feb 2024 09:48:04 +0800 > > On Mon, Feb 12, 2024 at 11:33 PM Eric Dumazet <edumazet@google.com> wrote: > > > > > > On Mon, Feb 12, 2024 at 10:29 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > > > > > From: Jason Xing <kernelxing@tencent.com> > > > > > > > > > > if (!acceptable) > > > > - return 1; > > > > + /* This reason isn't clear. We can refine it in the future */ > > > > + return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE; > > > > > > tcp_conn_request() might return 0 when a syncookie has been generated. > > > > > > Technically speaking, the incoming SYN was not dropped :) > > > > Hi Eric, Kuniyuki > > > > Sorry, I should have checked tcp_conn_request() carefully last night. > > Today, I checked tcp_conn_request() over and over again. > > > > I didn't find there is any chance to return a negative/positive value, > > only 0. It means @acceptable is always true and it should never return > > TCP_CONNREQNOTACCEPTABLE for TCP ipv4/6 protocol and never trigger a > > reset in this way. > > Ah right, I remember I digged the same thing before and even in the > initial commit, conn_request() always returned 0 and tcp_rcv_state_process() > tested it with if (retval < 0). Good. Thanks for your double check :) > > I think we can clean up the leftover with some comments above > ->conn_request() definition so that we can convert it to void > when we deprecate DCCP in the near future. In the next version, I will remove the new SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE and the comment line which I added and keep it as the old way, namely, returning 1. > > > > > > For DCCP, there are chances to return -1 in dccp_v4_conn_request(). > > But I don't think we've already added drop reasons in DCCP before. > > > > If I understand correctly, there is no need to do any refinement or > > even introduce TCP_CONNREQNOTACCEPTABLE new dropreason about the > > .conn_request() for TCP. > > > > Should I add a NEW kfree_skb_reason() in tcp_conn_request() for those > > labels, like drop_and_release, drop_and_free, drop, and not return a > > drop reason to its caller tcp_rcv_state_process()? > > Most interested reasons will be covered by > > - reqsk q : net_info_ratelimited() in tcp_syn_flood_action() or > net_dbg_ratelimited() in pr_drop_req() or > __NET_INC_STATS(net, LINUX_MIB_LISTENDROPS) in tcp_listendrop() > - accept q: NET_INC_STATS(net, LINUX_MIB_LISTENOVERFLOWS) or > __NET_INC_STATS(net, LINUX_MIB_LISTENDROPS) in tcp_listendrop() > > and could be refined by drop reason, but I'm not sure if drop reason > is used under such a pressured situation. Interesting. Let us wait for Eric's response. Thanks, Jason > > Also, these failures are now treated with consume_skb(). > > Whichever is fine to me, no strong preference. > > > > > > Please correct me if I'm wrong... > > > > Thanks, > > Jason > > > > > > > > I think you need to have a patch to change tcp_conn_request() and its > > > friends to return a 'refined' drop_reason > > > to avoid future questions / patches.
> > Hi Eric, Kuniyuki > > Sorry, I should have checked tcp_conn_request() carefully last night. > Today, I checked tcp_conn_request() over and over again. > > I didn't find there is any chance to return a negative/positive value, > only 0. It means @acceptable is always true and it should never return > TCP_CONNREQNOTACCEPTABLE for TCP ipv4/6 protocol and never trigger a > reset in this way. > Then send a cleanup, thanks. A standalone patch is going to be simpler than reviewing a whole series.
On Tue, Feb 13, 2024 at 5:35 PM Eric Dumazet <edumazet@google.com> wrote: > > > > > Hi Eric, Kuniyuki > > > > Sorry, I should have checked tcp_conn_request() carefully last night. > > Today, I checked tcp_conn_request() over and over again. > > > > I didn't find there is any chance to return a negative/positive value, > > only 0. It means @acceptable is always true and it should never return > > TCP_CONNREQNOTACCEPTABLE for TCP ipv4/6 protocol and never trigger a > > reset in this way. > > > > Then send a cleanup, thanks. > > A standalone patch is going to be simpler than reviewing a whole series. I fear that I could misunderstand what you mean. I'm not that familiar with how it works. Please enlighten me, thanks. Are you saying I don't need to send a new version of the current patch series, only send a patch after this series is applied? A standalone patch goes like this based on this patch series: diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h index 92513acca431..c059f7fc79f9 100644 --- a/include/net/dropreason-core.h +++ b/include/net/dropreason-core.h @@ -31,7 +31,6 @@ FN(TCP_AOFAILURE) \ FN(SOCKET_BACKLOG) \ FN(TCP_FLAGS) \ - FN(TCP_CONNREQNOTACCEPTABLE) \ FN(TCP_ABORTONDATA) \ FN(TCP_ZEROWINDOW) \ FN(TCP_OLD_DATA) \ @@ -210,12 +209,6 @@ enum skb_drop_reason { SKB_DROP_REASON_SOCKET_BACKLOG, /** @SKB_DROP_REASON_TCP_FLAGS: TCP flags invalid */ SKB_DROP_REASON_TCP_FLAGS, - /** - * @SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE: connection request is not - * acceptable. This reason currently is a little bit obscure. It could - * be split into more specific reasons in the future. - */ - SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE, /** * @SKB_DROP_REASON_TCP_ABORTONDATA: abort on data, corresponding to * LINUX_MIB_TCPABORTONDATA diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index d95f59f62742..023db3438b78 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -6658,8 +6658,7 @@ tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) rcu_read_unlock(); if (!acceptable) - /* This reason isn't clear. We can refine it in the future */ - return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE; + return 1; consume_skb(skb); return 0; } If that is so, what is the status of the current patch? Thanks, Jason
On Tue, Feb 13, 2024 at 11:30 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > On Tue, Feb 13, 2024 at 5:35 PM Eric Dumazet <edumazet@google.com> wrote: > > > > > > > > Hi Eric, Kuniyuki > > > > > > Sorry, I should have checked tcp_conn_request() carefully last night. > > > Today, I checked tcp_conn_request() over and over again. > > > > > > I didn't find there is any chance to return a negative/positive value, > > > only 0. It means @acceptable is always true and it should never return > > > TCP_CONNREQNOTACCEPTABLE for TCP ipv4/6 protocol and never trigger a > > > reset in this way. > > > > > > > Then send a cleanup, thanks. > > > > A standalone patch is going to be simpler than reviewing a whole series. > > I fear that I could misunderstand what you mean. I'm not that familiar > with how it works. Please enlighten me, thanks. > > Are you saying I don't need to send a new version of the current patch > series, only send a patch after this series is applied? > No. I suggested the clean up being sent before the series. If acceptable is always true in TCP, why keep dead code ? This would avoid many questions. diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 2d20edf652e6cb5eb56bda0107c99bed0b0a335f..b1c4462a0798c45e9b10d62715bc88fa35349078 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -6623,7 +6623,6 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) const struct tcphdr *th = tcp_hdr(skb); struct request_sock *req; int queued = 0; - bool acceptable; SKB_DR(reason); switch (sk->sk_state) { @@ -6649,12 +6648,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) */ rcu_read_lock(); local_bh_disable(); - acceptable = icsk->icsk_af_ops->conn_request(sk, skb) >= 0; + icsk->icsk_af_ops->conn_request(sk, skb); local_bh_enable(); rcu_read_unlock(); - if (!acceptable) - return 1; consume_skb(skb); return 0; }
On Tue, Feb 13, 2024 at 8:04 PM Eric Dumazet <edumazet@google.com> wrote: > > On Tue, Feb 13, 2024 at 11:30 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > On Tue, Feb 13, 2024 at 5:35 PM Eric Dumazet <edumazet@google.com> wrote: > > > > > > > > > > > Hi Eric, Kuniyuki > > > > > > > > Sorry, I should have checked tcp_conn_request() carefully last night. > > > > Today, I checked tcp_conn_request() over and over again. > > > > > > > > I didn't find there is any chance to return a negative/positive value, > > > > only 0. It means @acceptable is always true and it should never return > > > > TCP_CONNREQNOTACCEPTABLE for TCP ipv4/6 protocol and never trigger a > > > > reset in this way. > > > > > > > > > > Then send a cleanup, thanks. > > > > > > A standalone patch is going to be simpler than reviewing a whole series. > > > > I fear that I could misunderstand what you mean. I'm not that familiar > > with how it works. Please enlighten me, thanks. > > > > Are you saying I don't need to send a new version of the current patch > > series, only send a patch after this series is applied? > > > > No. I suggested the clean up being sent before the series. > > If acceptable is always true in TCP, why keep dead code ? > > This would avoid many questions. > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 2d20edf652e6cb5eb56bda0107c99bed0b0a335f..b1c4462a0798c45e9b10d62715bc88fa35349078 > 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -6623,7 +6623,6 @@ int tcp_rcv_state_process(struct sock *sk, > struct sk_buff *skb) > const struct tcphdr *th = tcp_hdr(skb); > struct request_sock *req; > int queued = 0; > - bool acceptable; > SKB_DR(reason); > > switch (sk->sk_state) { > @@ -6649,12 +6648,10 @@ int tcp_rcv_state_process(struct sock *sk, > struct sk_buff *skb) > */ > rcu_read_lock(); > local_bh_disable(); > - acceptable = > icsk->icsk_af_ops->conn_request(sk, skb) >= 0; > + icsk->icsk_af_ops->conn_request(sk, skb); > local_bh_enable(); > rcu_read_unlock(); > > - if (!acceptable) > - return 1; > consume_skb(skb); > return 0; > } Thanks for your explanation. Since the DCCP seems dead, there is no need to keep it for TCP as you said. I will send this patch first, then two updated patch series following. Thanks, Jason
> Thanks for your explanation. Since the DCCP seems dead, there is no Oops, I have to correct myself: it has nothing to do with DCCP because the caller of tcp_rcv_state_process() only exists in the TCP path. > need to keep it for TCP as you said. I will send this patch first, > then two updated patch series following. > > Thanks, > Jason
diff --git a/include/net/tcp.h b/include/net/tcp.h index 58e65af74ad1..e5af9a5b411b 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -348,7 +348,7 @@ void tcp_wfree(struct sk_buff *skb); void tcp_write_timer_handler(struct sock *sk); void tcp_delack_timer_handler(struct sock *sk); int tcp_ioctl(struct sock *sk, int cmd, int *karg); -int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb); +enum skb_drop_reason tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb); void tcp_rcv_established(struct sock *sk, struct sk_buff *skb); void tcp_rcv_space_adjust(struct sock *sk); int tcp_twsk_unique(struct sock *sk, struct sock *sktw, void *twp); diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index bfaf98c1f0ea..d95f59f62742 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -6619,7 +6619,8 @@ static void tcp_rcv_synrecv_state_fastopen(struct sock *sk) * address independent. */ -int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) +enum skb_drop_reason +tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) { struct tcp_sock *tp = tcp_sk(sk); struct inet_connection_sock *icsk = inet_csk(sk); @@ -6636,7 +6637,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) case TCP_LISTEN: if (th->ack) - return 1; + return SKB_DROP_REASON_TCP_FLAGS; if (th->rst) { SKB_DR_SET(reason, TCP_RESET); @@ -6657,7 +6658,8 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) rcu_read_unlock(); if (!acceptable) - return 1; + /* This reason isn't clear. We can refine it in the future */ + return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE; consume_skb(skb); return 0; } @@ -6707,8 +6709,13 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) FLAG_NO_CHALLENGE_ACK); if ((int)reason <= 0) { - if (sk->sk_state == TCP_SYN_RECV) - return 1; /* send one RST */ + if (sk->sk_state == TCP_SYN_RECV) { + /* send one RST */ + if (!reason) + return SKB_DROP_REASON_TCP_OLD_ACK; + else + return -reason; + } /* accept old ack during closing */ if ((int)reason < 0) { tcp_send_challenge_ack(sk); @@ -6784,7 +6791,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) if (READ_ONCE(tp->linger2) < 0) { tcp_done(sk); NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONDATA); - return 1; + return SKB_DROP_REASON_TCP_ABORTONDATA; } if (TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(skb)->seq && after(TCP_SKB_CB(skb)->end_seq - th->fin, tp->rcv_nxt)) { @@ -6793,7 +6800,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) tcp_fastopen_active_disable(sk); tcp_done(sk); NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONDATA); - return 1; + return SKB_DROP_REASON_TCP_ABORTONDATA; } tmo = tcp_fin_time(sk); @@ -6858,7 +6865,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb) after(TCP_SKB_CB(skb)->end_seq - th->fin, tp->rcv_nxt)) { NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONDATA); tcp_reset(sk, skb); - return 1; + return SKB_DROP_REASON_TCP_ABORTONDATA; } } fallthrough;