[PATCH v2 net-next 0/2] add more drop reasons in tcp receive path

[PATCH v2 net-next 0/2] add more drop reasons in tcp receive path

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

When I was debugging the reason about why the skb should be dropped in
syn cookie mode, I found out that this NOT_SPECIFIED reason is too
general. Thus I decided to refine it.

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 (2):
  tcp: add more DROP REASONs in cookie check
  tcp: add more DROP REASONs in receive process

 include/net/dropreason-core.h | 23 ++++++++++++++++++++++-
 include/net/tcp.h             |  4 ++--
 net/ipv4/syncookies.c         | 20 ++++++++++++++++----
 net/ipv4/tcp_input.c          | 26 +++++++++++++++++---------
 net/ipv4/tcp_ipv4.c           | 21 +++++++++++++--------
 net/ipv4/tcp_minisocks.c      | 10 +++++-----
 net/ipv6/tcp_ipv6.c           | 19 ++++++++++++-------
 7 files changed, 87 insertions(+), 36 deletions(-)

-- 
2.37.3

[PATCH v2 net-next 1/2] tcp: add more DROP REASONs in cookie check

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Since we've already introduced the drop reason mechanism, this function
is always using NOT_SPECIFIED which is too general and unhelpful to us
if we want to track this part.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/dropreason-core.h | 12 ++++++++++++
 net/ipv4/syncookies.c         | 20 ++++++++++++++++----
 net/ipv4/tcp_ipv4.c           |  2 +-
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 6d3a20163260..efbc5dfd9e84 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -6,6 +6,7 @@
 #define DEFINE_DROP_REASON(FN, FNe)	\
 	FN(NOT_SPECIFIED)		\
 	FN(NO_SOCKET)			\
+	FN(NO_REQSK_ALLOC)		\
 	FN(PKT_TOO_SMALL)		\
 	FN(TCP_CSUM)			\
 	FN(SOCKET_FILTER)		\
@@ -43,10 +44,12 @@
 	FN(TCP_FASTOPEN)		\
 	FN(TCP_OLD_ACK)			\
 	FN(TCP_TOO_OLD_ACK)		\
+	FN(COOKIE_NOCHILD)		\
 	FN(TCP_ACK_UNSENT_DATA)		\
 	FN(TCP_OFO_QUEUE_PRUNE)		\
 	FN(TCP_OFO_DROP)		\
 	FN(IP_OUTNOROUTES)		\
+	FN(IP_ROUTEOUTPUTKEY)		\
 	FN(BPF_CGROUP_EGRESS)		\
 	FN(IPV6DISABLED)		\
 	FN(NEIGH_CREATEFAIL)		\
@@ -54,6 +57,7 @@
 	FN(NEIGH_QUEUEFULL)		\
 	FN(NEIGH_DEAD)			\
 	FN(TC_EGRESS)			\
+	FN(SECURITY_HOOK)		\
 	FN(QDISC_DROP)			\
 	FN(CPU_BACKLOG)			\
 	FN(XDP)				\
@@ -107,6 +111,8 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_NOT_SPECIFIED,
 	/** @SKB_DROP_REASON_NO_SOCKET: socket not found */
 	SKB_DROP_REASON_NO_SOCKET,
+	/** @SKB_DROP_REASON_NO_REQSK_ALLOC: request socket allocation failed */
+	SKB_DROP_REASON_NO_REQSK_ALLOC,
 	/** @SKB_DROP_REASON_PKT_TOO_SMALL: packet size is too small */
 	SKB_DROP_REASON_PKT_TOO_SMALL,
 	/** @SKB_DROP_REASON_TCP_CSUM: TCP checksum error */
@@ -243,6 +249,8 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_TCP_OLD_ACK,
 	/** @SKB_DROP_REASON_TCP_TOO_OLD_ACK: TCP ACK is too old */
 	SKB_DROP_REASON_TCP_TOO_OLD_ACK,
+	/** @SKB_DROP_REASON_COOKIE_NOCHILD: no child socket in cookie mode */
+	SKB_DROP_REASON_COOKIE_NOCHILD,
 	/**
 	 * @SKB_DROP_REASON_TCP_ACK_UNSENT_DATA: TCP ACK for data we haven't
 	 * sent yet
@@ -254,6 +262,8 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_TCP_OFO_DROP,
 	/** @SKB_DROP_REASON_IP_OUTNOROUTES: route lookup failed */
 	SKB_DROP_REASON_IP_OUTNOROUTES,
+	/** @SKB_DROP_REASON_IP_ROUTEOUTPUTKEY: route output key failed */
+	SKB_DROP_REASON_IP_ROUTEOUTPUTKEY,
 	/**
 	 * @SKB_DROP_REASON_BPF_CGROUP_EGRESS: dropped by BPF_PROG_TYPE_CGROUP_SKB
 	 * eBPF program
@@ -271,6 +281,8 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_NEIGH_DEAD,
 	/** @SKB_DROP_REASON_TC_EGRESS: dropped in TC egress HOOK */
 	SKB_DROP_REASON_TC_EGRESS,
+	/** @SKB_DROP_REASON_SECURITY_HOOK: dropped due to security HOOK */
+	SKB_DROP_REASON_SECURITY_HOOK,
 	/**
 	 * @SKB_DROP_REASON_QDISC_DROP: dropped by qdisc when packet outputting (
 	 * failed to enqueue to current qdisc)
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index be88bf586ff9..6eb559ee20f9 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -399,6 +399,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 {
 	struct ip_options *opt = &TCP_SKB_CB(skb)->header.h4.opt;
 	const struct tcphdr *th = tcp_hdr(skb);
+	enum skb_drop_reason reason;
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct inet_request_sock *ireq;
 	struct net *net = sock_net(sk);
@@ -420,8 +421,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 		if (IS_ERR(req))
 			goto out;
 	}
-	if (!req)
+	if (!req) {
+		reason = SKB_DROP_REASON_NO_REQSK_ALLOC;
 		goto out_drop;
+	}
 
 	ireq = inet_rsk(req);
 
@@ -433,8 +436,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 	 */
 	RCU_INIT_POINTER(ireq->ireq_opt, tcp_v4_save_options(net, skb));
 
-	if (security_inet_conn_request(sk, skb, req))
+	if (security_inet_conn_request(sk, skb, req)) {
+		reason = SKB_DROP_REASON_SECURITY_HOOK;
 		goto out_free;
+	}
 
 	tcp_ao_syncookie(sk, skb, req, AF_INET);
 
@@ -451,8 +456,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 			   ireq->ir_loc_addr, th->source, th->dest, sk->sk_uid);
 	security_req_classify_flow(req, flowi4_to_flowi_common(&fl4));
 	rt = ip_route_output_key(net, &fl4);
-	if (IS_ERR(rt))
+	if (IS_ERR(rt)) {
+		reason = SKB_DROP_REASON_IP_ROUTEOUTPUTKEY;
 		goto out_free;
+	}
 
 	/* Try to redo what tcp_v4_send_synack did. */
 	req->rsk_window_clamp = tp->window_clamp ? :dst_metric(&rt->dst, RTAX_WINDOW);
@@ -475,12 +482,17 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 	/* ip_queue_xmit() depends on our flow being setup
 	 * Normal sockets get it right from inet_csk_route_child_sock()
 	 */
-	if (ret)
+	if (ret) {
 		inet_sk(ret)->cork.fl.u.ip4 = fl4;
+	} else {
+		reason = SKB_DROP_REASON_COOKIE_NOCHILD;
+		goto out_drop;
+	}
 out:
 	return ret;
 out_free:
 	reqsk_free(req);
 out_drop:
+	kfree_skb_reason(skb, reason);
 	return NULL;
 }
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 0c50c5a32b84..0a944e109088 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1915,7 +1915,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 		struct sock *nsk = tcp_v4_cookie_check(sk, skb);
 
 		if (!nsk)
-			goto discard;
+			return 0;
 		if (nsk != sk) {
 			if (tcp_child_process(sk, nsk, skb)) {
 				rsk = nsk;
-- 
2.37.3

[PATCH v2 net-next 2/2] tcp: add more DROP REASONs in receive process

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

As the title said, add more reasons to narrow down the range about
why the skb should be dropped.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/dropreason-core.h | 11 ++++++++++-
 include/net/tcp.h             |  4 ++--
 net/ipv4/tcp_input.c          | 26 +++++++++++++++++---------
 net/ipv4/tcp_ipv4.c           | 19 ++++++++++++-------
 net/ipv4/tcp_minisocks.c      | 10 +++++-----
 net/ipv6/tcp_ipv6.c           | 19 ++++++++++++-------
 6 files changed, 58 insertions(+), 31 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index efbc5dfd9e84..9a7643be9d07 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)			\
@@ -203,6 +206,10 @@ 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: con req not acceptable */
+	SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE,
+	/** @SKB_DROP_REASON_TCP_ABORTONDATA: abort on data */
+	SKB_DROP_REASON_TCP_ABORTONDATA,
 	/**
 	 * @SKB_DROP_REASON_TCP_ZEROWINDOW: TCP receive window size is zero,
 	 * see LINUX_MIB_TCPZEROWINDOWDROP
@@ -227,13 +234,15 @@ 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_ACK_INVALID_SEQUENCE: Not acceptable ACK SEQ field */
+	SKB_DROP_REASON_TCP_INVALID_ACK_SEQUENCE,
 	/** @SKB_DROP_REASON_TCP_RESET: Invalid RST packet */
 	SKB_DROP_REASON_TCP_RESET,
 	/**
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 58e65af74ad1..1d9b2a766b5e 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);
@@ -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_input.c b/net/ipv4/tcp_input.c
index 2d20edf652e6..81fe584aa777 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,7 @@ 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;
+	return reason;
 }
 
 static void tcp_rcv_synrecv_state_fastopen(struct sock *sk)
@@ -6616,7 +6618,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);
@@ -6633,7 +6636,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);
@@ -6654,7 +6657,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 			rcu_read_unlock();
 
 			if (!acceptable)
-				return 1;
+				return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE;
 			consume_skb(skb);
 			return 0;
 		}
@@ -6704,8 +6707,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);
@@ -6781,7 +6789,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)) {
@@ -6790,7 +6798,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);
@@ -6855,7 +6863,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;
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 0a944e109088..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;
 			}
@@ -1926,7 +1927,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;
 	}
@@ -2275,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/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 9e85f2a0bddd..8d23e28a9f04 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -911,11 +911,11 @@ EXPORT_SYMBOL(tcp_check_req);
  * be created.
  */
 
-int tcp_child_process(struct sock *parent, struct sock *child,
-		      struct sk_buff *skb)
+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;
 	int state = child->sk_state;
 
 	/* record sk_napi_id and sk_rx_queue_mapping of child. */
@@ -923,7 +923,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 +937,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 57b25b1fc9d9..0baddbaf9663 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1657,7 +1657,8 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 			goto discard;
 
 		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);
@@ -1666,7 +1667,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;
@@ -1856,12 +1858,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

Re: [PATCH v2 net-next 2/2] tcp: add more DROP REASONs in receive process

[Kuniyuki Iwashima]

From: Jason Xing <kerneljasonxing@gmail.com>
Date: Fri,  9 Feb 2024 14:12:13 +0800
> From: Jason Xing <kernelxing@tencent.com>
> 
> As the title said, add more reasons to narrow down the range about
> why the skb should be dropped.
> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  include/net/dropreason-core.h | 11 ++++++++++-
>  include/net/tcp.h             |  4 ++--
>  net/ipv4/tcp_input.c          | 26 +++++++++++++++++---------
>  net/ipv4/tcp_ipv4.c           | 19 ++++++++++++-------
>  net/ipv4/tcp_minisocks.c      | 10 +++++-----
>  net/ipv6/tcp_ipv6.c           | 19 ++++++++++++-------
>  6 files changed, 58 insertions(+), 31 deletions(-)
> 
> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index efbc5dfd9e84..9a7643be9d07 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)		\
[...]
> @@ -6654,7 +6657,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
>  			rcu_read_unlock();
>  
>  			if (!acceptable)
> -				return 1;
> +				return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE;

This sounds a bit ambiguous, and I think it can be more specific
if tcp_conn_request() returns the drop reason and we change the
acceptable evaluation.

  acceptable = icsk->icsk_af_ops->conn_request(sk, skb) >= 0;


>  			consume_skb(skb);
>  			return 0;
>  		}

Re: [PATCH v2 net-next 2/2] tcp: add more DROP REASONs in receive process

[Jason Xing]

On Fri, Feb 9, 2024 at 5:15 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Jason Xing <kerneljasonxing@gmail.com>
> Date: Fri,  9 Feb 2024 14:12:13 +0800
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > As the title said, add more reasons to narrow down the range about
> > why the skb should be dropped.
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  include/net/dropreason-core.h | 11 ++++++++++-
> >  include/net/tcp.h             |  4 ++--
> >  net/ipv4/tcp_input.c          | 26 +++++++++++++++++---------
> >  net/ipv4/tcp_ipv4.c           | 19 ++++++++++++-------
> >  net/ipv4/tcp_minisocks.c      | 10 +++++-----
> >  net/ipv6/tcp_ipv6.c           | 19 ++++++++++++-------
> >  6 files changed, 58 insertions(+), 31 deletions(-)
> >
> > diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> > index efbc5dfd9e84..9a7643be9d07 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)              \
> [...]
> > @@ -6654,7 +6657,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
> >                       rcu_read_unlock();
> >
> >                       if (!acceptable)
> > -                             return 1;
> > +                             return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE;
>
> This sounds a bit ambiguous, and I think it can be more specific
> if tcp_conn_request() returns the drop reason and we change the
> acceptable evaluation.

Sure, are you suggesting adding more reasons into .conn_request
callback functions, like tcp_v4_conn_request(), right?

If you don't mind, I can do it next time because it involves more
effort which could be put into a seperate patch or patchset.

Thanks,
Jason

>
>   acceptable = icsk->icsk_af_ops->conn_request(sk, skb) >= 0;
>
>
> >                       consume_skb(skb);
> >                       return 0;
> >               }

Re: [PATCH v2 net-next 2/2] tcp: add more DROP REASONs in receive process

[Eric Dumazet]

On Fri, Feb 9, 2024 at 7:12 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> As the title said, add more reasons to narrow down the range about
> why the skb should be dropped.
>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  include/net/dropreason-core.h | 11 ++++++++++-
>  include/net/tcp.h             |  4 ++--
>  net/ipv4/tcp_input.c          | 26 +++++++++++++++++---------
>  net/ipv4/tcp_ipv4.c           | 19 ++++++++++++-------
>  net/ipv4/tcp_minisocks.c      | 10 +++++-----
>  net/ipv6/tcp_ipv6.c           | 19 ++++++++++++-------
>  6 files changed, 58 insertions(+), 31 deletions(-)
>
> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index efbc5dfd9e84..9a7643be9d07 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)                   \
> @@ -203,6 +206,10 @@ 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: con req not acceptable */
> +       SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE,
> +       /** @SKB_DROP_REASON_TCP_ABORTONDATA: abort on data */
> +       SKB_DROP_REASON_TCP_ABORTONDATA,
>         /**
>          * @SKB_DROP_REASON_TCP_ZEROWINDOW: TCP receive window size is zero,
>          * see LINUX_MIB_TCPZEROWINDOWDROP
> @@ -227,13 +234,15 @@ 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_ACK_INVALID_SEQUENCE: Not acceptable ACK SEQ field */
> +       SKB_DROP_REASON_TCP_INVALID_ACK_SEQUENCE,
>         /** @SKB_DROP_REASON_TCP_RESET: Invalid RST packet */
>         SKB_DROP_REASON_TCP_RESET,
>         /**
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 58e65af74ad1..1d9b2a766b5e 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);
> @@ -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_input.c b/net/ipv4/tcp_input.c
> index 2d20edf652e6..81fe584aa777 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,7 @@ 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;
> +       return reason;
>  }
>
>  static void tcp_rcv_synrecv_state_fastopen(struct sock *sk)
> @@ -6616,7 +6618,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);
> @@ -6633,7 +6636,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);
> @@ -6654,7 +6657,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
>                         rcu_read_unlock();
>
>                         if (!acceptable)
> -                               return 1;
> +                               return SKB_DROP_REASON_TCP_CONNREQNOTACCEPTABLE;
>                         consume_skb(skb);
>                         return 0;
>                 }
> @@ -6704,8 +6707,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;

Your patch is too large/risky, not that I am trying to be gentle here...

You should know better that we are not going to accept it as is.
Please split your patches into smaller ones.

Eg, a single patch to change tcp_rcv_synsent_state_process() return value.
It used to return -1, 0, or 1.
Take the time to document for tcp_rcv_synsent_state_process() what are
the new possible return values.

Then other changes, one at a time, in a logical way.

Smaller patches are easier to review, even if it forces the author to
think a bit more about how to
make his series a work of art. Everyone wins, because we spend less
time and we learn faster.

Thank you.

Re: [PATCH v2 net-next 2/2] tcp: add more DROP REASONs in receive process

[Jason Xing]

[...]
>
> Your patch is too large/risky, not that I am trying to be gentle here...
>
> You should know better that we are not going to accept it as is.
> Please split your patches into smaller ones.
>
> Eg, a single patch to change tcp_rcv_synsent_state_process() return value.
> It used to return -1, 0, or 1.
> Take the time to document for tcp_rcv_synsent_state_process() what are
> the new possible return values.
>
> Then other changes, one at a time, in a logical way.
>
> Smaller patches are easier to review, even if it forces the author to
> think a bit more about how to
> make his series a work of art. Everyone wins, because we spend less
> time and we learn faster.

Now I understand. I'll separate the current patch.

One more question about whether I should split the 1/2 patch into some
smaller patches, say:
1) first, add some definitions into include/net/dropreason-core.h
2) then use those definitions in cookie_v4_check()
or
1) first, add some definitions.
2) second, add kfree_skb_reason() into cookie_v4_check() but only
NOT_SPECIFIED fake reason; get rid of 'goto discard;' in
tcp_v4_do_rcv() if @nsk is NULL.
3) third, use those definitions.
something like that...this way could make each patch more precise and clean.

What would you recommend about 1/2 patch?

Thanks,
Jason

>
> Thank you.