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

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

Jason Xing (2):
  tcp: add more DROP REASONs in cookie check
  tcp: add more DROP REASONS in child process

 include/net/dropreason-core.h | 18 ++++++++++++++++++
 include/net/tcp.h             |  8 +++++---
 net/ipv4/syncookies.c         | 18 ++++++++++++++----
 net/ipv4/tcp_input.c          | 19 +++++++++++++++----
 net/ipv4/tcp_ipv4.c           | 13 +++++++------
 net/ipv4/tcp_minisocks.c      |  4 ++--
 net/ipv6/tcp_ipv6.c           |  6 +++---
 7 files changed, 64 insertions(+), 22 deletions(-)

-- 
2.37.3

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Since the commit 8eba65fa5f06 ("net: tcp: use kfree_skb_reason() for tcp_v{4,6}_do_rcv()")
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 ++++++++++++
 include/net/tcp.h             |  3 ++-
 net/ipv4/syncookies.c         | 18 ++++++++++++++----
 net/ipv4/tcp_ipv4.c           |  7 ++++---
 4 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 6d3a20163260..85a19b883dee 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/include/net/tcp.h b/include/net/tcp.h
index 58e65af74ad1..e3b07d2790c4 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -492,7 +492,8 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
 				 struct request_sock *req,
 				 struct dst_entry *dst);
 int __cookie_v4_check(const struct iphdr *iph, const struct tcphdr *th);
-struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb);
+struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
+				 enum skb_drop_reason *reason);
 struct request_sock *cookie_tcp_reqsk_alloc(const struct request_sock_ops *ops,
 					    struct sock *sk, struct sk_buff *skb,
 					    struct tcp_options_received *tcp_opt,
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index be88bf586ff9..9febad3a3150 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -395,7 +395,8 @@ static struct request_sock *cookie_tcp_check(struct net *net, struct sock *sk,
  * Output is listener if incoming packet would not create a child
  *           NULL if memory could not be allocated.
  */
-struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
+struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
+					     enum skb_drop_reason *reason)
 {
 	struct ip_options *opt = &TCP_SKB_CB(skb)->header.h4.opt;
 	const struct tcphdr *th = tcp_hdr(skb);
@@ -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);
@@ -477,6 +484,9 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 	 */
 	if (ret)
 		inet_sk(ret)->cork.fl.u.ip4 = fl4;
+	else
+		*reason = SKB_DROP_REASON_COOKIE_NOCHILD;
+
 out:
 	return ret;
 out_free:
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 0c50c5a32b84..b63b0efa111d 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1846,13 +1846,14 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
 }
 EXPORT_SYMBOL(tcp_v4_syn_recv_sock);
 
-static struct sock *tcp_v4_cookie_check(struct sock *sk, struct sk_buff *skb)
+static struct sock *tcp_v4_cookie_check(struct sock *sk, struct sk_buff *skb,
+			 enum skb_drop_reason *reason)
 {
 #ifdef CONFIG_SYN_COOKIES
 	const struct tcphdr *th = tcp_hdr(skb);
 
 	if (!th->syn)
-		sk = cookie_v4_check(sk, skb);
+		sk = cookie_v4_check(sk, skb, reason);
 #endif
 	return sk;
 }
@@ -1912,7 +1913,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 		goto csum_err;
 
 	if (sk->sk_state == TCP_LISTEN) {
-		struct sock *nsk = tcp_v4_cookie_check(sk, skb);
+		struct sock *nsk = tcp_v4_cookie_check(sk, skb, &reason);
 
 		if (!nsk)
 			goto discard;
-- 
2.37.3

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Since the commit 8eba65fa5f06 ("net: tcp: use kfree_skb_reason() for tcp_v{4,6}_do_rcv()")
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 ++++++++++++
 include/net/tcp.h             |  3 ++-
 net/ipv4/syncookies.c         | 18 ++++++++++++++----
 net/ipv4/tcp_ipv4.c           |  7 ++++---
 4 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 6d3a20163260..85a19b883dee 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/include/net/tcp.h b/include/net/tcp.h
index 58e65af74ad1..e3b07d2790c4 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -492,7 +492,8 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
 				 struct request_sock *req,
 				 struct dst_entry *dst);
 int __cookie_v4_check(const struct iphdr *iph, const struct tcphdr *th);
-struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb);
+struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
+				 enum skb_drop_reason *reason);
 struct request_sock *cookie_tcp_reqsk_alloc(const struct request_sock_ops *ops,
 					    struct sock *sk, struct sk_buff *skb,
 					    struct tcp_options_received *tcp_opt,
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index be88bf586ff9..9febad3a3150 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -395,7 +395,8 @@ static struct request_sock *cookie_tcp_check(struct net *net, struct sock *sk,
  * Output is listener if incoming packet would not create a child
  *           NULL if memory could not be allocated.
  */
-struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
+struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
+					     enum skb_drop_reason *reason)
 {
 	struct ip_options *opt = &TCP_SKB_CB(skb)->header.h4.opt;
 	const struct tcphdr *th = tcp_hdr(skb);
@@ -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);
@@ -477,6 +484,9 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 	 */
 	if (ret)
 		inet_sk(ret)->cork.fl.u.ip4 = fl4;
+	else
+		*reason = SKB_DROP_REASON_COOKIE_NOCHILD;
+
 out:
 	return ret;
 out_free:
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 0c50c5a32b84..b63b0efa111d 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1846,13 +1846,14 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
 }
 EXPORT_SYMBOL(tcp_v4_syn_recv_sock);
 
-static struct sock *tcp_v4_cookie_check(struct sock *sk, struct sk_buff *skb)
+static struct sock *tcp_v4_cookie_check(struct sock *sk, struct sk_buff *skb,
+			 enum skb_drop_reason *reason)
 {
 #ifdef CONFIG_SYN_COOKIES
 	const struct tcphdr *th = tcp_hdr(skb);
 
 	if (!th->syn)
-		sk = cookie_v4_check(sk, skb);
+		sk = cookie_v4_check(sk, skb, reason);
 #endif
 	return sk;
 }
@@ -1912,7 +1913,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 		goto csum_err;
 
 	if (sk->sk_state == TCP_LISTEN) {
-		struct sock *nsk = tcp_v4_cookie_check(sk, skb);
+		struct sock *nsk = tcp_v4_cookie_check(sk, skb, &reason);
 
 		if (!nsk)
 			goto discard;
-- 
2.37.3

[PATCH net-next 2/2] tcp: add more DROP REASONS in child 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 |  6 ++++++
 include/net/tcp.h             |  5 +++--
 net/ipv4/tcp_input.c          | 19 +++++++++++++++----
 net/ipv4/tcp_ipv4.c           |  6 +++---
 net/ipv4/tcp_minisocks.c      |  4 ++--
 net/ipv6/tcp_ipv6.c           |  6 +++---
 6 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 85a19b883dee..980fd4442b7c 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)		\
@@ -203,6 +205,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
diff --git a/include/net/tcp.h b/include/net/tcp.h
index e3b07d2790c4..8c87170cb0b5 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -348,7 +348,8 @@ 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);
+int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
+			enum skb_drop_reason *drop_reason);
 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);
@@ -397,7 +398,7 @@ 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,
-		      struct sk_buff *skb);
+		      struct sk_buff *skb, enum skb_drop_reason *reason);
 void tcp_enter_loss(struct sock *sk);
 void tcp_cwnd_reduction(struct sock *sk, int newly_acked_sacked, int newly_lost, int flag);
 void tcp_clear_retrans(struct tcp_sock *tp);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2d20edf652e6..bacb1140dab3 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6616,7 +6616,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)
+int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
+					 enum skb_drop_reason *drop_reason)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct inet_connection_sock *icsk = inet_csk(sk);
@@ -6632,8 +6633,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 		goto discard;
 
 	case TCP_LISTEN:
-		if (th->ack)
+		if (th->ack) {
+			SKB_DR_SET(*drop_reason, TCP_FLAGS);
 			return 1;
+		}
 
 		if (th->rst) {
 			SKB_DR_SET(reason, TCP_RESET);
@@ -6653,8 +6656,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 			local_bh_enable();
 			rcu_read_unlock();
 
-			if (!acceptable)
+			if (!acceptable) {
+				SKB_DR_SET(*drop_reason, TCP_CONNREQNOTACCEPTABLE);
 				return 1;
+			}
 			consume_skb(skb);
 			return 0;
 		}
@@ -6704,8 +6709,11 @@ 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)
+		if (sk->sk_state == TCP_SYN_RECV) {
+			if ((int)reason < 0)
+				*drop_reason = -reason;
 			return 1;	/* send one RST */
+		}
 		/* accept old ack during closing */
 		if ((int)reason < 0) {
 			tcp_send_challenge_ack(sk);
@@ -6781,6 +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);
+			SKB_DR_SET(*drop_reason, TCP_ABORTONDATA);
 			return 1;
 		}
 		if (TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(skb)->seq &&
@@ -6790,6 +6799,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);
+			SKB_DR_SET(*drop_reason, TCP_ABORTONDATA);
 			return 1;
 		}
 
@@ -6855,6 +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);
+				SKB_DR_SET(*drop_reason, TCP_ABORTONDATA);
 				return 1;
 			}
 		}
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index b63b0efa111d..7da62af0d890 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1918,7 +1918,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 		if (!nsk)
 			goto discard;
 		if (nsk != sk) {
-			if (tcp_child_process(sk, nsk, skb)) {
+			if (tcp_child_process(sk, nsk, skb, &reason)) {
 				rsk = nsk;
 				goto reset;
 			}
@@ -1927,7 +1927,7 @@ 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)) {
+	if (tcp_rcv_state_process(sk, skb, &reason)) {
 		rsk = sk;
 		goto reset;
 	}
@@ -2276,7 +2276,7 @@ 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)) {
+		} else if (tcp_child_process(sk, nsk, skb, &drop_reason)) {
 			tcp_v4_send_reset(nsk, skb);
 			goto discard_and_relse;
 		} else {
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 9e85f2a0bddd..49a88bf47b79 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -912,7 +912,7 @@ EXPORT_SYMBOL(tcp_check_req);
  */
 
 int tcp_child_process(struct sock *parent, struct sock *child,
-		      struct sk_buff *skb)
+		      struct sk_buff *skb, enum skb_drop_reason *reason)
 	__releases(&((child)->sk_lock.slock))
 {
 	int ret = 0;
@@ -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);
+		ret = tcp_rcv_state_process(child, skb, reason);
 		/* Wakeup parent, send SIGIO */
 		if (state == TCP_SYN_RECV && child->sk_state != state)
 			parent->sk_data_ready(parent);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 57b25b1fc9d9..ced52f7aa5bb 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1657,7 +1657,7 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 			goto discard;
 
 		if (nsk != sk) {
-			if (tcp_child_process(sk, nsk, skb))
+			if (tcp_child_process(sk, nsk, skb, &reason))
 				goto reset;
 			if (opt_skb)
 				__kfree_skb(opt_skb);
@@ -1666,7 +1666,7 @@ 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))
+	if (tcp_rcv_state_process(sk, skb, &reason))
 		goto reset;
 	if (opt_skb)
 		goto ipv6_pktoptions;
@@ -1856,7 +1856,7 @@ 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)) {
+		} else if (tcp_child_process(sk, nsk, skb, &drop_reason)) {
 			tcp_v6_send_reset(nsk, skb);
 			goto discard_and_relse;
 		} else {
-- 
2.37.3

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

[Jason Xing]

On Sun, Feb 4, 2024 at 6:46 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> 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.

Hello, any suggestions? Those names in the patchset could be improper,
but I've already tried to name them in English :S

Thanks,
Jason

>
> Jason Xing (2):
>   tcp: add more DROP REASONs in cookie check
>   tcp: add more DROP REASONS in child process
>
>  include/net/dropreason-core.h | 18 ++++++++++++++++++
>  include/net/tcp.h             |  8 +++++---
>  net/ipv4/syncookies.c         | 18 ++++++++++++++----
>  net/ipv4/tcp_input.c          | 19 +++++++++++++++----
>  net/ipv4/tcp_ipv4.c           | 13 +++++++------
>  net/ipv4/tcp_minisocks.c      |  4 ++--
>  net/ipv6/tcp_ipv6.c           |  6 +++---
>  7 files changed, 64 insertions(+), 22 deletions(-)
>
> --
> 2.37.3
>

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

[Jakub Kicinski]

On Sun,  4 Feb 2024 18:45:58 +0800 Jason Xing wrote:
> 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.

Please run checkpatch --strict on the patches, post v2 with warnings
fixed.

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

[Eric Dumazet]

On Wed, Feb 7, 2024 at 3:24 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Sun, Feb 4, 2024 at 6:46 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > 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.
>
> Hello, any suggestions? Those names in the patchset could be improper,
> but I've already tried to name them in English :S
>

Adding &drop_reason parameters all over the places adds more code for
CONFIG_STACKPROTECTOR=y builds,
because of added canary checks.

Please make sure not to slow down the TCP fast path, while we work
hard in the opposite direction.

Also, sending patch series over weekends increases the chance for them
being lost, just my personal opinion...


> Thanks,
> Jason
>
> >
> > Jason Xing (2):
> >   tcp: add more DROP REASONs in cookie check
> >   tcp: add more DROP REASONS in child process
> >
> >  include/net/dropreason-core.h | 18 ++++++++++++++++++
> >  include/net/tcp.h             |  8 +++++---
> >  net/ipv4/syncookies.c         | 18 ++++++++++++++----
> >  net/ipv4/tcp_input.c          | 19 +++++++++++++++----
> >  net/ipv4/tcp_ipv4.c           | 13 +++++++------
> >  net/ipv4/tcp_minisocks.c      |  4 ++--
> >  net/ipv6/tcp_ipv6.c           |  6 +++---
> >  7 files changed, 64 insertions(+), 22 deletions(-)
> >
> > --
> > 2.37.3
> >

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

[Jason Xing]

On Wed, Feb 7, 2024 at 11:03 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun,  4 Feb 2024 18:45:58 +0800 Jason Xing wrote:
> > 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.
>
> Please run checkpatch --strict on the patches, post v2 with warnings
> fixed.

Thanks, I'll do that.

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

[Jason Xing]

On Wed, Feb 7, 2024 at 5:22 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Feb 7, 2024 at 3:24 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > On Sun, Feb 4, 2024 at 6:46 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > 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.
> >
> > Hello, any suggestions? Those names in the patchset could be improper,
> > but I've already tried to name them in English :S
> >
>
> Adding &drop_reason parameters all over the places adds more code for
> CONFIG_STACKPROTECTOR=y builds,
> because of added canary checks.

Indeed.

It took me some while to consider whether I should add more reasons
into the fast path.

But for now the NOT_SPECIFIED fake reason does not work if we really
want to know some useful hints.
What do you think? Should I give up this patch series or come up with
other better ideas?

>
> Please make sure not to slow down the TCP fast path, while we work
> hard in the opposite direction.

I tested some times by using netperf, it's not that easy to observe
the obvious differences before/after this patch applied.

>
> Also, sending patch series over weekends increases the chance for them
> being lost, just my personal opinion...

Oh, I see :S

Thanks,
Jason

>
>
> > Thanks,
> > Jason
> >
> > >
> > > Jason Xing (2):
> > >   tcp: add more DROP REASONs in cookie check
> > >   tcp: add more DROP REASONS in child process
> > >
> > >  include/net/dropreason-core.h | 18 ++++++++++++++++++
> > >  include/net/tcp.h             |  8 +++++---
> > >  net/ipv4/syncookies.c         | 18 ++++++++++++++----
> > >  net/ipv4/tcp_input.c          | 19 +++++++++++++++----
> > >  net/ipv4/tcp_ipv4.c           | 13 +++++++------
> > >  net/ipv4/tcp_minisocks.c      |  4 ++--
> > >  net/ipv6/tcp_ipv6.c           |  6 +++---
> > >  7 files changed, 64 insertions(+), 22 deletions(-)
> > >
> > > --
> > > 2.37.3
> > >

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

[Eric Dumazet]

On Wed, Feb 7, 2024 at 2:25 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>

> Indeed.
>
> It took me some while to consider whether I should add more reasons
> into the fast path.
>
> But for now the NOT_SPECIFIED fake reason does not work if we really
> want to know some useful hints.
> What do you think? Should I give up this patch series or come up with
> other better ideas?

Perhaps find a way to reuse return values from functions to carry a drop_reason.

>
> >
> > Please make sure not to slow down the TCP fast path, while we work
> > hard in the opposite direction.
>
> I tested some times by using netperf, it's not that easy to observe
> the obvious differences before/after this patch applied.

Sure, the difference is only noticeable on moderate load, when a cpu
receives one packet in a while.

icache pressure, something hard to measure with synthetic benchmarks,
but visible in real workloads in the long term.

At Google, we definitely see an increase of network cpu costs releases
after releases.

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

[Jason Xing]

On Wed, Feb 7, 2024 at 9:37 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Feb 7, 2024 at 2:25 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
>
> > Indeed.
> >
> > It took me some while to consider whether I should add more reasons
> > into the fast path.
> >
> > But for now the NOT_SPECIFIED fake reason does not work if we really
> > want to know some useful hints.
> > What do you think? Should I give up this patch series or come up with
> > other better ideas?
>
> Perhaps find a way to reuse return values from functions to carry a drop_reason.

It seems feasible to reuse return values, let me work on it :)

>
> >
> > >
> > > Please make sure not to slow down the TCP fast path, while we work
> > > hard in the opposite direction.
> >
> > I tested some times by using netperf, it's not that easy to observe
> > the obvious differences before/after this patch applied.
>
> Sure, the difference is only noticeable on moderate load, when a cpu
> receives one packet in a while.
>
> icache pressure, something hard to measure with synthetic benchmarks,
> but visible in real workloads in the long term.
>
> At Google, we definitely see an increase of network cpu costs releases
> after releases.

Thanks for your explanations.

Thanks,
Jason