[PATCH net-next v3 0/6] introduce dropreasons in tcp receive path

[PATCH net-next v3 0/6] introduce dropreasons in tcp receive path

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

As titled said, we're going to refine the NOT_SPECIFIED reason in the
tcp v4/6 receive fast path.

This serie is another one of the v2 patchset[1] which are here split
into six patches. Besides, this patch is made on top of the previous
serie[2] I submitted some time ago.

[1]
Link: https://lore.kernel.org/all/20240209061213.72152-3-kerneljasonxing@gmail.com/
[2]
Link: https://lore.kernel.org/all/20240212052513.37914-1-kerneljasonxing@gmail.com/

v3:
Link: https://lore.kernel.org/all/CANn89iK40SoyJ8fS2U5kp3pDruo=zfQNPL-ppOF+LYaS9z-MVA@mail.gmail.com/
1. Split that patch into some smaller ones as suggested by Eric.

v2:
Link: https://lore.kernel.org/all/20240204104601.55760-1-kerneljasonxing@gmail.com/
1. change the title of 2/2 patch.
2. fix some warnings checkpatch tool showed before.
3. use return value instead of adding more parameters suggested by Eric.


Jason Xing (6):
  tcp: introduce another three dropreasons in receive path
  tcp: add more specific possible drop reasons in
    tcp_rcv_synsent_state_process()
  tcp: add dropreasons in tcp_rcv_state_process()
  tcp: make the dropreason really work when calling
    tcp_rcv_state_process()
  tcp: make dropreason in tcp_child_process() work
  tcp: get rid of NOT_SEPCIFIED reason in tcp_v4/6_do_rcv

 include/net/dropreason-core.h | 22 +++++++++++++++++++++-
 include/net/tcp.h             |  4 ++--
 net/ipv4/tcp_input.c          | 28 +++++++++++++++++++---------
 net/ipv4/tcp_ipv4.c           | 20 ++++++++++++--------
 net/ipv4/tcp_minisocks.c      |  9 +++++----
 net/ipv6/tcp_ipv6.c           | 20 ++++++++++++--------
 6 files changed, 71 insertions(+), 32 deletions(-)

-- 
2.37.3

[PATCH net-next v3 1/6] tcp: introduce another three dropreasons in receive path

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Soon later patches can use these relatively more accurate
reasons to recognise and find out the cause.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/dropreason-core.h | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 7cedece5dbbb..92513acca431 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -31,6 +31,8 @@
 	FN(TCP_AOFAILURE)		\
 	FN(SOCKET_BACKLOG)		\
 	FN(TCP_FLAGS)			\
+	FN(TCP_CONNREQNOTACCEPTABLE)			\
+	FN(TCP_ABORTONDATA)			\
 	FN(TCP_ZEROWINDOW)		\
 	FN(TCP_OLD_DATA)		\
 	FN(TCP_OVERWINDOW)		\
@@ -38,6 +40,7 @@
 	FN(TCP_RFC7323_PAWS)		\
 	FN(TCP_OLD_SEQUENCE)		\
 	FN(TCP_INVALID_SEQUENCE)	\
+	FN(TCP_INVALID_ACK_SEQUENCE)	\
 	FN(TCP_RESET)			\
 	FN(TCP_INVALID_SYN)		\
 	FN(TCP_CLOSE)			\
@@ -207,6 +210,17 @@ 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
+	 */
+	SKB_DROP_REASON_TCP_ABORTONDATA,
 	/**
 	 * @SKB_DROP_REASON_TCP_ZEROWINDOW: TCP receive window size is zero,
 	 * see LINUX_MIB_TCPZEROWINDOWDROP
@@ -231,13 +245,19 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_TCP_OFOMERGE,
 	/**
 	 * @SKB_DROP_REASON_TCP_RFC7323_PAWS: PAWS check, corresponding to
-	 * LINUX_MIB_PAWSESTABREJECTED
+	 * LINUX_MIB_PAWSESTABREJECTED, LINUX_MIB_PAWSACTIVEREJECTED
 	 */
 	SKB_DROP_REASON_TCP_RFC7323_PAWS,
 	/** @SKB_DROP_REASON_TCP_OLD_SEQUENCE: Old SEQ field (duplicate packet) */
 	SKB_DROP_REASON_TCP_OLD_SEQUENCE,
 	/** @SKB_DROP_REASON_TCP_INVALID_SEQUENCE: Not acceptable SEQ field */
 	SKB_DROP_REASON_TCP_INVALID_SEQUENCE,
+	/**
+	 * @SKB_DROP_REASON_TCP_INVALID_ACK_SEQUENCE: Not acceptable ACK SEQ
+	 * field. because of ack sequence is not in the window between snd_una
+	 * and snd_nxt
+	 */
+	SKB_DROP_REASON_TCP_INVALID_ACK_SEQUENCE,
 	/** @SKB_DROP_REASON_TCP_RESET: Invalid RST packet */
 	SKB_DROP_REASON_TCP_RESET,
 	/**
-- 
2.37.3

[PATCH net-next v3 2/6] tcp: add more specific possible drop reasons in tcp_rcv_synsent_state_process()

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

This patch does two things:
1) add two more new reasons
2) only change the return value(1) to various drop reason values
for the future use

For now, we still cannot trace those two reasons. We'll implement the full
function in the subsequent patch in this serie.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/ipv4/tcp_input.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2d20edf652e6..bfaf98c1f0ea 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6361,6 +6361,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 				inet_csk_reset_xmit_timer(sk,
 						ICSK_TIME_RETRANS,
 						TCP_TIMEOUT_MIN, TCP_RTO_MAX);
+			SKB_DR_SET(reason, TCP_INVALID_ACK_SEQUENCE);
 			goto reset_and_undo;
 		}
 
@@ -6369,6 +6370,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 			     tcp_time_stamp_ts(tp))) {
 			NET_INC_STATS(sock_net(sk),
 					LINUX_MIB_PAWSACTIVEREJECTED);
+			SKB_DR_SET(reason, TCP_RFC7323_PAWS);
 			goto reset_and_undo;
 		}
 
@@ -6572,7 +6574,8 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 reset_and_undo:
 	tcp_clear_options(&tp->rx_opt);
 	tp->rx_opt.mss_clamp = saved_clamp;
-	return 1;
+	/* we can reuse/return @reason to its caller to handle the exception */
+	return reason;
 }
 
 static void tcp_rcv_synrecv_state_fastopen(struct sock *sk)
-- 
2.37.3

[PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

In this patch, I equipped this function with more dropreasons, but
it still doesn't work yet, which I will do later.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/tcp.h    |  2 +-
 net/ipv4/tcp_input.c | 23 +++++++++++++++--------
 2 files changed, 16 insertions(+), 9 deletions(-)

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;
-- 
2.37.3

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Eric Dumazet]

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.

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Jason Xing]

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

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Eric Dumazet]

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.

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Jason Xing]

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

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Jason Xing]

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.

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Kuniyuki Iwashima]

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.

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Jason Xing]

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.

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Eric Dumazet]

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

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Jason Xing]

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

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Eric Dumazet]

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;
                }

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Jason Xing]

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

Re: [PATCH net-next v3 3/6] tcp: add dropreasons in tcp_rcv_state_process()

[Jason Xing]

> 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

[PATCH net-next v3 4/6] tcp: make the dropreason really work when calling tcp_rcv_state_process()

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Update three callers including both ipv4 and ipv6 and let the dropreason
mechanism work in reality.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/tcp.h        | 2 +-
 net/ipv4/tcp_ipv4.c      | 3 ++-
 net/ipv4/tcp_minisocks.c | 9 +++++----
 net/ipv6/tcp_ipv6.c      | 3 ++-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index e5af9a5b411b..1d9b2a766b5e 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -396,7 +396,7 @@ enum tcp_tw_status tcp_timewait_state_process(struct inet_timewait_sock *tw,
 struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 			   struct request_sock *req, bool fastopen,
 			   bool *lost_race);
-int tcp_child_process(struct sock *parent, struct sock *child,
+enum skb_drop_reason tcp_child_process(struct sock *parent, struct sock *child,
 		      struct sk_buff *skb);
 void tcp_enter_loss(struct sock *sk);
 void tcp_cwnd_reduction(struct sock *sk, int newly_acked_sacked, int newly_lost, int flag);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 0a944e109088..c79e25549972 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1926,7 +1926,8 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 	} else
 		sock_rps_save_rxhash(sk, skb);
 
-	if (tcp_rcv_state_process(sk, skb)) {
+	reason = tcp_rcv_state_process(sk, skb);
+	if (reason) {
 		rsk = sk;
 		goto reset;
 	}
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 9e85f2a0bddd..08d5b48540ea 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -911,11 +911,12 @@ EXPORT_SYMBOL(tcp_check_req);
  * be created.
  */
 
-int tcp_child_process(struct sock *parent, struct sock *child,
+enum skb_drop_reason
+tcp_child_process(struct sock *parent, struct sock *child,
 		      struct sk_buff *skb)
 	__releases(&((child)->sk_lock.slock))
 {
-	int ret = 0;
+	enum skb_drop_reason reason = SKB_NOT_DROPPED_YET;
 	int state = child->sk_state;
 
 	/* record sk_napi_id and sk_rx_queue_mapping of child. */
@@ -923,7 +924,7 @@ int tcp_child_process(struct sock *parent, struct sock *child,
 
 	tcp_segs_in(tcp_sk(child), skb);
 	if (!sock_owned_by_user(child)) {
-		ret = tcp_rcv_state_process(child, skb);
+		reason = tcp_rcv_state_process(child, skb);
 		/* Wakeup parent, send SIGIO */
 		if (state == TCP_SYN_RECV && child->sk_state != state)
 			parent->sk_data_ready(parent);
@@ -937,6 +938,6 @@ int tcp_child_process(struct sock *parent, struct sock *child,
 
 	bh_unlock_sock(child);
 	sock_put(child);
-	return ret;
+	return reason;
 }
 EXPORT_SYMBOL(tcp_child_process);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 27639ffcae2f..4924d41fb2b1 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1669,7 +1669,8 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 	} else
 		sock_rps_save_rxhash(sk, skb);
 
-	if (tcp_rcv_state_process(sk, skb))
+	reason = tcp_rcv_state_process(sk, skb);
+	if (reason)
 		goto reset;
 	if (opt_skb)
 		goto ipv6_pktoptions;
-- 
2.37.3

[PATCH net-next v3 5/6] tcp: make dropreason in tcp_child_process() work

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

It's time to let it work right now. We've already prepared for this:)

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/ipv4/tcp_ipv4.c | 16 ++++++++++------
 net/ipv6/tcp_ipv6.c | 16 ++++++++++------
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index c79e25549972..c886c671fae9 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1917,7 +1917,8 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 		if (!nsk)
 			return 0;
 		if (nsk != sk) {
-			if (tcp_child_process(sk, nsk, skb)) {
+			reason = tcp_child_process(sk, nsk, skb);
+			if (reason) {
 				rsk = nsk;
 				goto reset;
 			}
@@ -2276,12 +2277,15 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		if (nsk == sk) {
 			reqsk_put(req);
 			tcp_v4_restore_cb(skb);
-		} else if (tcp_child_process(sk, nsk, skb)) {
-			tcp_v4_send_reset(nsk, skb);
-			goto discard_and_relse;
 		} else {
-			sock_put(sk);
-			return 0;
+			drop_reason = tcp_child_process(sk, nsk, skb);
+			if (drop_reason) {
+				tcp_v4_send_reset(nsk, skb);
+				goto discard_and_relse;
+			} else {
+				sock_put(sk);
+				return 0;
+			}
 		}
 	}
 
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 4924d41fb2b1..73fef436dbf6 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1660,7 +1660,8 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 		}
 
 		if (nsk != sk) {
-			if (tcp_child_process(sk, nsk, skb))
+			reason = tcp_child_process(sk, nsk, skb);
+			if (reason)
 				goto reset;
 			if (opt_skb)
 				__kfree_skb(opt_skb);
@@ -1860,12 +1861,15 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 		if (nsk == sk) {
 			reqsk_put(req);
 			tcp_v6_restore_cb(skb);
-		} else if (tcp_child_process(sk, nsk, skb)) {
-			tcp_v6_send_reset(nsk, skb);
-			goto discard_and_relse;
 		} else {
-			sock_put(sk);
-			return 0;
+			drop_reason = tcp_child_process(sk, nsk, skb);
+			if (drop_reason) {
+				tcp_v6_send_reset(nsk, skb);
+				goto discard_and_relse;
+			} else {
+				sock_put(sk);
+				return 0;
+			}
 		}
 	}
 
-- 
2.37.3

[PATCH net-next v3 6/6] tcp: get rid of NOT_SEPCIFIED reason in tcp_v4/6_do_rcv

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Finally we can drop this obscure reason in receive path  because
we replaced with many other more accurate reasons before.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/ipv4/tcp_ipv4.c | 1 -
 net/ipv6/tcp_ipv6.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index c886c671fae9..82e63f6af34b 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1907,7 +1907,6 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 		return 0;
 	}
 
-	reason = SKB_DROP_REASON_NOT_SPECIFIED;
 	if (tcp_checksum_complete(skb))
 		goto csum_err;
 
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 73fef436dbf6..29a0fe0c8101 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1623,7 +1623,6 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 	if (np->rxopt.all)
 		opt_skb = skb_clone_and_charge_r(skb, sk);
 
-	reason = SKB_DROP_REASON_NOT_SPECIFIED;
 	if (sk->sk_state == TCP_ESTABLISHED) { /* Fast path */
 		struct dst_entry *dst;
 
-- 
2.37.3