[PATCH net-next v5 00/11] introduce drop reasons for tcp receive path

[PATCH net-next v5 00/11] introduce drop reasons for 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.

v5:
Link: https://lore.kernel.org/netdev/20240213134205.8705-1-kerneljasonxing@gmail.com/
Link: https://lore.kernel.org/netdev/20240213140508.10878-1-kerneljasonxing@gmail.com/
1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new
   one (Eric, David)
2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket
   allocation (Eric)
3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
4. avoid duplication of these opt_skb tests/actions (Eric)
5. Use new name (TCP_ABORT_ON_DATA) for readability (David)
6. Reuse IP_OUTNOROUTES instead of INVALID_DST (Eric)


---
HISTORY
This series is combined with 2 series sent before suggested by Jakub. So
I'm going to separately write changelogs for each of them.

PATCH 1/11 - 5/11
preivious Link: https://lore.kernel.org/netdev/20240213134205.8705-1-kerneljasonxing@gmail.com/
Summary
1. introduce all the dropreasons we need, [1/11] patch.
2. use new dropreasons in ipv4 cookie check, [2/11],[3/11] patch.
3. use new dropreasons ipv6 cookie check, [4/11],[5/11] patch.

v4:
Link: https://lore.kernel.org/netdev/20240212172302.3f95e454@kernel.org/
1. Fix misspelled name in Kdoc as suggested by Jakub.

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.


PATCH 6/11 - 11/11
previous Link: https://lore.kernel.org/netdev/20240213140508.10878-1-kerneljasonxing@gmail.com/
v4:
Link: https://lore.kernel.org/netdev/CANn89iJar+H3XkQ8HpsirH7b-_sbFe9NBUdAAO3pNJK3CKr_bg@mail.gmail.com/
Link: https://lore.kernel.org/netdev/20240213131205.4309-1-kerneljasonxing@gmail.com/
Already got rid of @acceptable in tcp_rcv_state_process(), so I need to
remove *TCP_CONNREQNOTACCEPTABLE related codes which I wrote in the v3
series.

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 (11):
  tcp: add dropreasons definitions and prepare for cookie check
  tcp: directly drop skb in cookie check for ipv4
  tcp: use drop reasons in cookie check for ipv4
  tcp: directly drop skb in cookie check for ipv6
  tcp: use drop reasons in cookie check for ipv6
  tcp: introduce 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_SPECIFIED reason in tcp_v4/6_do_rcv

 include/net/dropreason-core.h | 21 ++++++++++++++++++++-
 include/net/tcp.h             |  4 ++--
 net/ipv4/syncookies.c         | 20 ++++++++++++++++----
 net/ipv4/tcp_input.c          | 25 +++++++++++++++++--------
 net/ipv4/tcp_ipv4.c           | 22 +++++++++++++---------
 net/ipv4/tcp_minisocks.c      |  9 +++++----
 net/ipv6/syncookies.c         | 18 +++++++++++++++---
 net/ipv6/tcp_ipv6.c           | 31 ++++++++++++++++---------------
 8 files changed, 104 insertions(+), 46 deletions(-)

-- 
2.37.3

[PATCH net-next v5 01/11] tcp: add a dropreason definitions and prepare for cookie check

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Only add one drop reason to detect the condition of skb dropped
because of hook points in cookie check for later use.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
--
v5
Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/
1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David)
2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
4. Reuse IP_OUTNOROUTES instead of INVALID_DST (Eric)
5. adjust the title and description.

v4
Link: https://lore.kernel.org/netdev/20240212172302.3f95e454@kernel.org/
1. fix misspelled name in kdoc as Jakub said
---
 include/net/dropreason-core.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 6d3a20163260..3aaccad4eb20 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -54,6 +54,7 @@
 	FN(NEIGH_QUEUEFULL)		\
 	FN(NEIGH_DEAD)			\
 	FN(TC_EGRESS)			\
+	FN(SECURITY_HOOK)		\
 	FN(QDISC_DROP)			\
 	FN(CPU_BACKLOG)			\
 	FN(XDP)				\
@@ -271,6 +272,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)
-- 
2.37.3

[PATCH net-next v5 02/11] tcp: directly drop skb in cookie check for ipv4

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Only move the skb drop from tcp_v4_do_rcv() to cookie_v4_check() itself,
no other changes made. It can help us refine the specific drop reasons
later.

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

diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index be88bf586ff9..38f331da6677 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -408,6 +408,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 	struct rtable *rt;
 	__u8 rcv_wscale;
 	int full_space;
+	SKB_DR(reason);
 
 	if (!READ_ONCE(net->ipv4.sysctl_tcp_syncookies) ||
 	    !th->ack || th->rst)
@@ -477,10 +478,13 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 	 */
 	if (ret)
 		inet_sk(ret)->cork.fl.u.ip4 = fl4;
+	else
+		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 net-next v5 03/11] tcp: use drop reasons in cookie check for ipv4

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Now it's time to use the prepared definitions to refine this part.
Four reasons used might enough for now, I think.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
--
v5:
Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/
1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David)
2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
---
 net/ipv4/syncookies.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 38f331da6677..aeb61c880fbd 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -421,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) {
+		SKB_DR_SET(reason, NOMEM);
 		goto out_drop;
+	}
 
 	ireq = inet_rsk(req);
 
@@ -434,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)) {
+		SKB_DR_SET(reason, SECURITY_HOOK);
 		goto out_free;
+	}
 
 	tcp_ao_syncookie(sk, skb, req, AF_INET);
 
@@ -452,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)) {
+		SKB_DR_SET(reason, IP_OUTNOROUTES);
 		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);
@@ -476,10 +482,12 @@ 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
+	} else {
+		SKB_DR_SET(reason, NO_SOCKET);
 		goto out_drop;
+	}
 out:
 	return ret;
 out_free:
-- 
2.37.3

[PATCH net-next v5 04/11] tcp: directly drop skb in cookie check for ipv6

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Like previous patch does, only moving skb drop logical code to
cookie_v6_check() for later refinement.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
--
v5
Link: https://lore.kernel.org/netdev/CANn89iKz7=1q7e8KY57Dn3ED7O=RCOfLxoHQKO4eNXnZa1OPWg@mail.gmail.com/
1. avoid duplication of these opt_skb tests/actions (Eric)
---
 net/ipv6/syncookies.c |  4 ++++
 net/ipv6/tcp_ipv6.c   | 11 ++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index 6b9c69278819..ea0d9954a29f 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -177,6 +177,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	struct sock *ret = sk;
 	__u8 rcv_wscale;
 	int full_space;
+	SKB_DR(reason);
 
 	if (!READ_ONCE(net->ipv4.sysctl_tcp_syncookies) ||
 	    !th->ack || th->rst)
@@ -256,10 +257,13 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	ireq->ecn_ok &= cookie_ecn_ok(net, dst);
 
 	ret = tcp_get_cookie_sock(sk, skb, req, dst);
+	if (!ret)
+		goto out_drop;
 out:
 	return ret;
 out_free:
 	reqsk_free(req);
 out_drop:
+	kfree_skb_reason(skb, reason);
 	return NULL;
 }
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 57b25b1fc9d9..1ca4f11c3d6f 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1653,16 +1653,13 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 	if (sk->sk_state == TCP_LISTEN) {
 		struct sock *nsk = tcp_v6_cookie_check(sk, skb);
 
-		if (!nsk)
-			goto discard;
-
-		if (nsk != sk) {
+		if (nsk && nsk != sk) {
 			if (tcp_child_process(sk, nsk, skb))
 				goto reset;
-			if (opt_skb)
-				__kfree_skb(opt_skb);
-			return 0;
 		}
+		if (opt_skb)
+			__kfree_skb(opt_skb);
+		return 0;
 	} else
 		sock_rps_save_rxhash(sk, skb);
 
-- 
2.37.3

[PATCH net-next v5 05/11] tcp: use drop reasons in cookie check for ipv6

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Like what I did to ipv4 mode, refine this part: adding more drop
reasons for better tracing.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
--
v5:
Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
1. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
2. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
3. Reuse IP_OUTNOROUTES instead of INVALID_DST (Eric)
---
 net/ipv6/syncookies.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index ea0d9954a29f..bf51db679bd6 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -190,16 +190,20 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 		if (IS_ERR(req))
 			goto out;
 	}
-	if (!req)
+	if (!req) {
+		SKB_DR_SET(reason, NOMEM);
 		goto out_drop;
+	}
 
 	ireq = inet_rsk(req);
 
 	ireq->ir_v6_rmt_addr = ipv6_hdr(skb)->saddr;
 	ireq->ir_v6_loc_addr = ipv6_hdr(skb)->daddr;
 
-	if (security_inet_conn_request(sk, skb, req))
+	if (security_inet_conn_request(sk, skb, req)) {
+		SKB_DR_SET(reason, SECURITY_HOOK);
 		goto out_free;
+	}
 
 	if (ipv6_opt_accepted(sk, skb, &TCP_SKB_CB(skb)->header.h6) ||
 	    np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
@@ -236,8 +240,10 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 		security_req_classify_flow(req, flowi6_to_flowi_common(&fl6));
 
 		dst = ip6_dst_lookup_flow(net, sk, &fl6, final_p);
-		if (IS_ERR(dst))
+		if (IS_ERR(dst)) {
+			SKB_DR_SET(reason, IP_OUTNOROUTES);
 			goto out_free;
+		}
 	}
 
 	req->rsk_window_clamp = tp->window_clamp ? :dst_metric(dst, RTAX_WINDOW);
@@ -257,8 +263,10 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	ireq->ecn_ok &= cookie_ecn_ok(net, dst);
 
 	ret = tcp_get_cookie_sock(sk, skb, req, dst);
-	if (!ret)
+	if (!ret) {
+		SKB_DR_SET(reason, NO_SOCKET);
 		goto out_drop;
+	}
 out:
 	return ret;
 out_free:
-- 
2.37.3

[PATCH net-next v5 06/11] tcp: introduce 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>
--
v5:
Link: https://lore.kernel.org/netdev/3a495358-4c47-4a9f-b116-5f9c8b44e5ab@kernel.org/
1. Use new name (TCP_ABORT_ON_DATA) for readability (David)
2. change the title of this patch
---
 include/net/dropreason-core.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 3aaccad4eb20..581775763db7 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -30,6 +30,7 @@
 	FN(TCP_AOFAILURE)		\
 	FN(SOCKET_BACKLOG)		\
 	FN(TCP_FLAGS)			\
+	FN(TCP_ABORT_ON_DATA)	\
 	FN(TCP_ZEROWINDOW)		\
 	FN(TCP_OLD_DATA)		\
 	FN(TCP_OVERWINDOW)		\
@@ -37,6 +38,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)			\
@@ -198,6 +200,11 @@ 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_ABORT_ON_DATA: abort on data, corresponding to
+	 * LINUX_MIB_TCPABORTONDATA
+	 */
+	SKB_DROP_REASON_TCP_ABORT_ON_DATA,
 	/**
 	 * @SKB_DROP_REASON_TCP_ZEROWINDOW: TCP receive window size is zero,
 	 * see LINUX_MIB_TCPZEROWINDOWDROP
@@ -222,13 +229,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 v5 07/11] 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 b1c4462a0798..43194918ab45 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 v5 08/11] 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>
--
v5:
Link: https://lore.kernel.org/netdev/3a495358-4c47-4a9f-b116-5f9c8b44e5ab@kernel.org/
1. Use new name (TCP_ABORT_ON_DATA) for readability (David)
---
 include/net/tcp.h    |  2 +-
 net/ipv4/tcp_input.c | 20 +++++++++++++-------
 2 files changed, 14 insertions(+), 8 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 43194918ab45..f89af858dfae 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);
@@ -6635,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);
@@ -6704,8 +6705,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 +6787,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_ABORT_ON_DATA;
 		}
 		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 +6796,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_ABORT_ON_DATA;
 		}
 
 		tmo = tcp_fin_time(sk);
@@ -6855,7 +6861,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_ABORT_ON_DATA;
 			}
 		}
 		fallthrough;
-- 
2.37.3

[PATCH net-next v5 09/11] 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 1ca4f11c3d6f..b13eb4985152 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1663,7 +1663,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 v5 10/11] 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 b13eb4985152..4b2afb9fe293 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1654,7 +1654,8 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 		struct sock *nsk = tcp_v6_cookie_check(sk, skb);
 
 		if (nsk && nsk != sk) {
-			if (tcp_child_process(sk, nsk, skb))
+			reason = tcp_child_process(sk, nsk, skb);
+			if (reason)
 				goto reset;
 		}
 		if (opt_skb)
@@ -1854,12 +1855,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 v5 11/11] tcp: get rid of NOT_SPECIFIED 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>
--
v5:
1. change the misspelled word in the title
---
 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 4b2afb9fe293..4035cbee32cb 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

Re: [PATCH net-next v5 00/11] introduce drop reasons for tcp receive path

[Paolo Abeni]

Hi,

On Thu, 2024-02-15 at 09:20 +0800, Jason Xing 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.
> 
> v5:
> Link: https://lore.kernel.org/netdev/20240213134205.8705-1-kerneljasonxing@gmail.com/
> Link: https://lore.kernel.org/netdev/20240213140508.10878-1-kerneljasonxing@gmail.com/
> 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new
>    one (Eric, David)
> 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket
>    allocation (Eric)
> 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
> 4. avoid duplication of these opt_skb tests/actions (Eric)
> 5. Use new name (TCP_ABORT_ON_DATA) for readability (David)
> 6. Reuse IP_OUTNOROUTES instead of INVALID_DST (Eric)

It looks like this is causing a lot of self-test failures:

https://netdev.bots.linux.dev/contest.html?pw-n=0&branch=net-next-2024-02-15--06-00&pass=0&skip=0

due to tcp connect timeout, e.g.:

https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/466281/9-tcp-fastopen-backup-key-sh/stdout

please have look.

Thanks!

Paolo

Re: [PATCH net-next v5 04/11] tcp: directly drop skb in cookie check for ipv6

[Jason Xing]

On Thu, Feb 15, 2024 at 9:20 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Like previous patch does, only moving skb drop logical code to
> cookie_v6_check() for later refinement.
>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> --
> v5
> Link: https://lore.kernel.org/netdev/CANn89iKz7=1q7e8KY57Dn3ED7O=RCOfLxoHQKO4eNXnZa1OPWg@mail.gmail.com/
> 1. avoid duplication of these opt_skb tests/actions (Eric)
> ---
>  net/ipv6/syncookies.c |  4 ++++
>  net/ipv6/tcp_ipv6.c   | 11 ++++-------
>  2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> index 6b9c69278819..ea0d9954a29f 100644
> --- a/net/ipv6/syncookies.c
> +++ b/net/ipv6/syncookies.c
> @@ -177,6 +177,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>         struct sock *ret = sk;
>         __u8 rcv_wscale;
>         int full_space;
> +       SKB_DR(reason);
>
>         if (!READ_ONCE(net->ipv4.sysctl_tcp_syncookies) ||
>             !th->ack || th->rst)
> @@ -256,10 +257,13 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>         ireq->ecn_ok &= cookie_ecn_ok(net, dst);
>
>         ret = tcp_get_cookie_sock(sk, skb, req, dst);
> +       if (!ret)
> +               goto out_drop;
>  out:
>         return ret;
>  out_free:
>         reqsk_free(req);
>  out_drop:
> +       kfree_skb_reason(skb, reason);
>         return NULL;
>  }
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 57b25b1fc9d9..1ca4f11c3d6f 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1653,16 +1653,13 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
>         if (sk->sk_state == TCP_LISTEN) {
>                 struct sock *nsk = tcp_v6_cookie_check(sk, skb);
>
> -               if (!nsk)
> -                       goto discard;
> -
> -               if (nsk != sk) {
> +               if (nsk && nsk != sk) {
>                         if (tcp_child_process(sk, nsk, skb))
>                                 goto reset;
> -                       if (opt_skb)
> -                               __kfree_skb(opt_skb);
> -                       return 0;
>                 }
> +               if (opt_skb)
> +                       __kfree_skb(opt_skb);
> +               return 0;

Oops, the above lines went wrong, which could cause the error that
Paolo reported in my patch[0/11] email.

The error could happen when if:
    nsk != NULL && nsk == sk
it will return 0, which is against old behaviour :(

I will fix this...

>         } else
>                 sock_rps_save_rxhash(sk, skb);
>
> --
> 2.37.3
>

Re: [PATCH net-next v5 00/11] introduce drop reasons for tcp receive path

[Jason Xing]

On Thu, Feb 15, 2024 at 6:12 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hi,
>
> On Thu, 2024-02-15 at 09:20 +0800, Jason Xing 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.
> >
> > v5:
> > Link: https://lore.kernel.org/netdev/20240213134205.8705-1-kerneljasonxing@gmail.com/
> > Link: https://lore.kernel.org/netdev/20240213140508.10878-1-kerneljasonxing@gmail.com/
> > 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new
> >    one (Eric, David)
> > 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket
> >    allocation (Eric)
> > 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
> > 4. avoid duplication of these opt_skb tests/actions (Eric)
> > 5. Use new name (TCP_ABORT_ON_DATA) for readability (David)
> > 6. Reuse IP_OUTNOROUTES instead of INVALID_DST (Eric)
>
> It looks like this is causing a lot of self-test failures:
>
> https://netdev.bots.linux.dev/contest.html?pw-n=0&branch=net-next-2024-02-15--06-00&pass=0&skip=0
>
> due to tcp connect timeout, e.g.:
>
> https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/466281/9-tcp-fastopen-backup-key-sh/stdout
>
> please have look.

Thanks for your report. I double checked the series and found out my
new version of patch [4/11] is wrong. I will fix that.

I will continue to check the logic of the rest.

Thanks,
Jason

>
> Thanks!
>
> Paolo
>

[PATCH net-next v5 03/11] tcp: use drop reasons in cookie check for ipv4

[Kuniyuki Iwashima]

From: Jason Xing <kerneljasonxing@gmail.com>
Date: Thu, 15 Feb 2024 09:20:19 +0800
> From: Jason Xing <kernelxing@tencent.com>
> 
> Now it's time to use the prepared definitions to refine this part.
> Four reasons used might enough for now, I think.
> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> --
> v5:
> Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
> Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/
> 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David)
> 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
> 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
> ---
>  net/ipv4/syncookies.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index 38f331da6677..aeb61c880fbd 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -421,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) {
> +		SKB_DR_SET(reason, NOMEM);

NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails.


>  		goto out_drop;
> +	}
>  
>  	ireq = inet_rsk(req);
>  
> @@ -434,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)) {
> +		SKB_DR_SET(reason, SECURITY_HOOK);
>  		goto out_free;
> +	}
>  
>  	tcp_ao_syncookie(sk, skb, req, AF_INET);
>  
> @@ -452,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)) {
> +		SKB_DR_SET(reason, IP_OUTNOROUTES);
>  		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);
> @@ -476,10 +482,12 @@ 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
> +	} else {
> +		SKB_DR_SET(reason, NO_SOCKET);

This also seems wrong to me.

e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk),
then the listener is actually found.


>  		goto out_drop;
> +	}
>  out:
>  	return ret;
>  out_free:
> -- 
> 2.37.3
> 

Re: [PATCH net-next v5 03/11] tcp: use drop reasons in cookie check for ipv4

[Jason Xing]

On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Jason Xing <kerneljasonxing@gmail.com>
> Date: Thu, 15 Feb 2024 09:20:19 +0800
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Now it's time to use the prepared definitions to refine this part.
> > Four reasons used might enough for now, I think.
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > --
> > v5:
> > Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
> > Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/
> > 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David)
> > 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
> > 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
> > ---
> >  net/ipv4/syncookies.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > index 38f331da6677..aeb61c880fbd 100644
> > --- a/net/ipv4/syncookies.c
> > +++ b/net/ipv4/syncookies.c
> > @@ -421,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) {
> > +             SKB_DR_SET(reason, NOMEM);
>
> NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails.

Thanks for your careful check. It's true. I didn't check the MPTCP
path about how to handle it.

It also means that what I did to the cookie_v6_check() is also wrong.

[...]
> >       /* Try to redo what tcp_v4_send_synack did. */
> >       req->rsk_window_clamp = tp->window_clamp ? :dst_metric(&rt->dst, RTAX_WINDOW);
> > @@ -476,10 +482,12 @@ 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
> > +     } else {
> > +             SKB_DR_SET(reason, NO_SOCKET);
>
> This also seems wrong to me.
>
> e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk),
> then the listener is actually found.

Initially I thought using a not-that-clear name could be helpfull,
though. NO_SOCKET here means no child socket can be used if I add a
new description to SKB_DROP_REASON_NO_SOCKET.

If the idea is proper, how about using NO_SOCKET for the first point
you said to explain that there is no request socket that can be used?

If not, for both of the points you mentioned, it seems I have to add
back those two new reasons (perhaps with a better name updated)?
1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request
socket allocation in cookie_v4/6_check())
2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket
fetching in cookie_v4/6_check())

Now I'm struggling with the name and whether I should introduce some
new reasons like what I did in the old version of the series :S

If someone comes up with a good name or a good way to explain them,
please tell me, thanks!

also cc Eric, David

Thanks,
Jason

>
>
> >               goto out_drop;
> > +     }
> >  out:
> >       return ret;
> >  out_free:
> > --
> > 2.37.3
> >

Re: [PATCH net-next v5 03/11] tcp: use drop reasons in cookie check for ipv4

[Kuniyuki Iwashima]

From: Jason Xing <kerneljasonxing@gmail.com>
Date: Fri, 16 Feb 2024 09:28:26 +0800
> On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > From: Jason Xing <kerneljasonxing@gmail.com>
> > Date: Thu, 15 Feb 2024 09:20:19 +0800
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Now it's time to use the prepared definitions to refine this part.
> > > Four reasons used might enough for now, I think.
> > >
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > --
> > > v5:
> > > Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
> > > Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/
> > > 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David)
> > > 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
> > > 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
> > > ---
> > >  net/ipv4/syncookies.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > > index 38f331da6677..aeb61c880fbd 100644
> > > --- a/net/ipv4/syncookies.c
> > > +++ b/net/ipv4/syncookies.c
> > > @@ -421,8 +421,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> > >               if (IS_ERR(req))
> > >                       goto out;

I noticed in this case (ret == sk) we can set drop reason in
tcp_v4_do_rcv() as INVALID_COOKIE or something else.


> > >       }
> > > -     if (!req)
> > > +     if (!req) {
> > > +             SKB_DR_SET(reason, NOMEM);
> >
> > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails.
> 
> Thanks for your careful check. It's true. I didn't check the MPTCP
> path about how to handle it.
> 
> It also means that what I did to the cookie_v6_check() is also wrong.

Yes, same for the v6 change.


> 
> [...]
> > >       /* Try to redo what tcp_v4_send_synack did. */
> > >       req->rsk_window_clamp = tp->window_clamp ? :dst_metric(&rt->dst, RTAX_WINDOW);
> > > @@ -476,10 +482,12 @@ 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
> > > +     } else {
> > > +             SKB_DR_SET(reason, NO_SOCKET);
> >
> > This also seems wrong to me.
> >
> > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk),
> > then the listener is actually found.
> 
> Initially I thought using a not-that-clear name could be helpfull,
> though. NO_SOCKET here means no child socket can be used if I add a
> new description to SKB_DROP_REASON_NO_SOCKET.

Currently, NO_SOCKET is used only when sk lookup fails.  Mixing
different reasons sounds like pushing it back to NOT_SPECIFIED.
We could distinguish them by the caller IP though.


> 
> If the idea is proper, how about using NO_SOCKET for the first point
> you said to explain that there is no request socket that can be used?
> 
> If not, for both of the points you mentioned, it seems I have to add
> back those two new reasons (perhaps with a better name updated)?
> 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request
> socket allocation in cookie_v4/6_check())
> 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket
> fetching in cookie_v4/6_check())
> 
> Now I'm struggling with the name and whether I should introduce some
> new reasons like what I did in the old version of the series :S

Why naming is hard would be because there are multiple reasons of
failure.  One way to be more specific is moving kfree_skb_reason()
into callee as you did in patch 2.


> If someone comes up with a good name or a good way to explain them,
> please tell me, thanks!

For 1. no idea :p

For 2. Maybe VALID_COOKIE ?  we drop the valid cookie in the same
function, but due to LSM or L3 layer, so the reason could be said
as L4-specific ?


> 
> also cc Eric, David
> 
> Thanks,
> Jason
> 

Re: [PATCH net-next v5 03/11] tcp: use drop reasons in cookie check for ipv4

[Jason Xing]

On Fri, Feb 16, 2024 at 11:03 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Jason Xing <kerneljasonxing@gmail.com>
> Date: Fri, 16 Feb 2024 09:28:26 +0800
> > On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > >
> > > From: Jason Xing <kerneljasonxing@gmail.com>
> > > Date: Thu, 15 Feb 2024 09:20:19 +0800
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > Now it's time to use the prepared definitions to refine this part.
> > > > Four reasons used might enough for now, I think.
> > > >
> > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > --
> > > > v5:
> > > > Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
> > > > Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/
> > > > 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David)
> > > > 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
> > > > 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
> > > > ---
> > > >  net/ipv4/syncookies.c | 18 +++++++++++++-----
> > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > > > index 38f331da6677..aeb61c880fbd 100644
> > > > --- a/net/ipv4/syncookies.c
> > > > +++ b/net/ipv4/syncookies.c
> > > > @@ -421,8 +421,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> > > >               if (IS_ERR(req))
> > > >                       goto out;
>
> I noticed in this case (ret == sk) we can set drop reason in
> tcp_v4_do_rcv() as INVALID_COOKIE or something else.

If cookie_v4_check() returns the sk which is the same as the first
parameter of its caller (tcp_v4_do_rcv()), then we cannot directly
drop it because it is against old behaviours and causes errors. It
should go into tcp_rcv_state_process() later. The similar mistake I
made was reported by Paolo in the patch [0/11] (see link[1] as below).

link[1]: https://lore.kernel.org/netdev/c987d2c79e4a4655166eb8eafef473384edb37fb.camel@redhat.com/

>
>
> > > >       }
> > > > -     if (!req)
> > > > +     if (!req) {
> > > > +             SKB_DR_SET(reason, NOMEM);
> > >
> > > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails.
> >
> > Thanks for your careful check. It's true. I didn't check the MPTCP
> > path about how to handle it.
> >
> > It also means that what I did to the cookie_v6_check() is also wrong.
>
> Yes, same for the v6 change.
>
>
> >
> > [...]
> > > >       /* Try to redo what tcp_v4_send_synack did. */
> > > >       req->rsk_window_clamp = tp->window_clamp ? :dst_metric(&rt->dst, RTAX_WINDOW);
> > > > @@ -476,10 +482,12 @@ 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
> > > > +     } else {
> > > > +             SKB_DR_SET(reason, NO_SOCKET);
> > >
> > > This also seems wrong to me.
> > >
> > > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk),
> > > then the listener is actually found.
> >
> > Initially I thought using a not-that-clear name could be helpfull,
> > though. NO_SOCKET here means no child socket can be used if I add a
> > new description to SKB_DROP_REASON_NO_SOCKET.
>
> Currently, NO_SOCKET is used only when sk lookup fails.  Mixing
> different reasons sounds like pushing it back to NOT_SPECIFIED.
> We could distinguish them by the caller IP though.

It makes some sense, but I still think NO_SOCKET is just a mixture of
three kinds of cases (no sk during lookup process, no child sk, no
reqsk).
Let me think about it.

>
>
> >
> > If the idea is proper, how about using NO_SOCKET for the first point
> > you said to explain that there is no request socket that can be used?
> >
> > If not, for both of the points you mentioned, it seems I have to add
> > back those two new reasons (perhaps with a better name updated)?
> > 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request
> > socket allocation in cookie_v4/6_check())
> > 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket
> > fetching in cookie_v4/6_check())
> >
> > Now I'm struggling with the name and whether I should introduce some
> > new reasons like what I did in the old version of the series :S
>
> Why naming is hard would be because there are multiple reasons of
> failure.  One way to be more specific is moving kfree_skb_reason()
> into callee as you did in patch 2.
>
>
> > If someone comes up with a good name or a good way to explain them,
> > please tell me, thanks!
>
> For 1. no idea :p
>
> For 2. Maybe VALID_COOKIE ?  we drop the valid cookie in the same
> function, but due to LSM or L3 layer, so the reason could be said
> as L4-specific ?

Thanks for your idea :)

For 2, if we're on the same page and talk about how to handle
tcp_get_cookie_sock(), the name is not that appropriate because as you
said in your previous email it could fail due to full of accept queue
instead of cookie problem.

If we're talking about cookie_tcp_check(), the name is also not that
good because the drop reason could be no memory which is unrelated to
cookie, right?

COOKIE, it seems, cannot be the keyword/generic reason to conclude all
the reasons for either of them...

Thanks,
Jason

>
>
> >
> > also cc Eric, David
> >
> > Thanks,
> > Jason
> >

Re: [PATCH net-next v5 03/11] tcp: use drop reasons in cookie check for ipv4

[Kuniyuki Iwashima]

From: Jason Xing <kerneljasonxing@gmail.com>
Date: Fri, 16 Feb 2024 11:50:45 +0800
> On Fri, Feb 16, 2024 at 11:03 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > From: Jason Xing <kerneljasonxing@gmail.com>
> > Date: Fri, 16 Feb 2024 09:28:26 +0800
> > > On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > > >
> > > > From: Jason Xing <kerneljasonxing@gmail.com>
> > > > Date: Thu, 15 Feb 2024 09:20:19 +0800
> > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > >
> > > > > Now it's time to use the prepared definitions to refine this part.
> > > > > Four reasons used might enough for now, I think.
> > > > >
> > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > > --
> > > > > v5:
> > > > > Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
> > > > > Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/
> > > > > 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David)
> > > > > 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
> > > > > 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
> > > > > ---
> > > > >  net/ipv4/syncookies.c | 18 +++++++++++++-----
> > > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > > > > index 38f331da6677..aeb61c880fbd 100644
> > > > > --- a/net/ipv4/syncookies.c
> > > > > +++ b/net/ipv4/syncookies.c
> > > > > @@ -421,8 +421,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> > > > >               if (IS_ERR(req))
> > > > >                       goto out;
> >
> > I noticed in this case (ret == sk) we can set drop reason in
> > tcp_v4_do_rcv() as INVALID_COOKIE or something else.
> 
> If cookie_v4_check() returns the sk which is the same as the first
> parameter of its caller (tcp_v4_do_rcv()), then we cannot directly
> drop it

No, I meant we can just set drop reason, not calling kfree_skb_reason()
just after cookie_v4_check().

Then, in tcp_v4_do_rcv(), the default reason is NOT_SPECIFIED, but
INVALID_COOKIE only when cookie_v4_check() returns the listener.

---8<---
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 0c50c5a32b84..05cd697a7c07 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1923,6 +1923,8 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 			}
 			return 0;
 		}
+
+		reason = SKB_DROP_REASON_INVALID_COOKIE;
 	} else
 		sock_rps_save_rxhash(sk, skb);
 
---8<---


> because it is against old behaviours and causes errors. It
> should go into tcp_rcv_state_process() later. The similar mistake I
> made was reported by Paolo in the patch [0/11] (see link[1] as below).
> 
> link[1]: https://lore.kernel.org/netdev/c987d2c79e4a4655166eb8eafef473384edb37fb.camel@redhat.com/
> 
> >
> >
> > > > >       }
> > > > > -     if (!req)
> > > > > +     if (!req) {
> > > > > +             SKB_DR_SET(reason, NOMEM);
> > > >
> > > > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails.
> > >
> > > Thanks for your careful check. It's true. I didn't check the MPTCP
> > > path about how to handle it.
> > >
> > > It also means that what I did to the cookie_v6_check() is also wrong.
> >
> > Yes, same for the v6 change.
> >
> >
> > >
> > > [...]
> > > > >       /* Try to redo what tcp_v4_send_synack did. */
> > > > >       req->rsk_window_clamp = tp->window_clamp ? :dst_metric(&rt->dst, RTAX_WINDOW);
> > > > > @@ -476,10 +482,12 @@ 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
> > > > > +     } else {
> > > > > +             SKB_DR_SET(reason, NO_SOCKET);
> > > >
> > > > This also seems wrong to me.
> > > >
> > > > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk),
> > > > then the listener is actually found.
> > >
> > > Initially I thought using a not-that-clear name could be helpfull,
> > > though. NO_SOCKET here means no child socket can be used if I add a
> > > new description to SKB_DROP_REASON_NO_SOCKET.
> >
> > Currently, NO_SOCKET is used only when sk lookup fails.  Mixing
> > different reasons sounds like pushing it back to NOT_SPECIFIED.
> > We could distinguish them by the caller IP though.
> 
> It makes some sense, but I still think NO_SOCKET is just a mixture of
> three kinds of cases (no sk during lookup process, no child sk, no
> reqsk).
> Let me think about it.
> 
> >
> >
> > >
> > > If the idea is proper, how about using NO_SOCKET for the first point
> > > you said to explain that there is no request socket that can be used?
> > >
> > > If not, for both of the points you mentioned, it seems I have to add
> > > back those two new reasons (perhaps with a better name updated)?
> > > 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request
> > > socket allocation in cookie_v4/6_check())
> > > 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket
> > > fetching in cookie_v4/6_check())
> > >
> > > Now I'm struggling with the name and whether I should introduce some
> > > new reasons like what I did in the old version of the series :S
> >
> > Why naming is hard would be because there are multiple reasons of
> > failure.  One way to be more specific is moving kfree_skb_reason()
> > into callee as you did in patch 2.
> >
> >
> > > If someone comes up with a good name or a good way to explain them,
> > > please tell me, thanks!
> >
> > For 1. no idea :p
> >
> > For 2. Maybe VALID_COOKIE ?  we drop the valid cookie in the same
> > function, but due to LSM or L3 layer, so the reason could be said
> > as L4-specific ?
> 
> Thanks for your idea :)
> 
> For 2, if we're on the same page and talk about how to handle
> tcp_get_cookie_sock(), the name is not that appropriate because as you
> said in your previous email it could fail due to full of accept queue
> instead of cookie problem.

That's why I wrote _VALID_ COOKIE.  Here we know the cookie was valid
but somehow 3WHS failed.  If we want to be more specific, what is not
appropriate would be the place where we set the reason or call kfree_skb().


> 
> If we're talking about cookie_tcp_check(), the name is also not that
> good because the drop reason could be no memory which is unrelated to
> cookie, right?
> 
> COOKIE, it seems, cannot be the keyword/generic reason to conclude all
> the reasons for either of them...
> 
> Thanks,
> Jason
> 
> >
> >
> > >
> > > also cc Eric, David
> > >
> > > Thanks,
> > > Jason
> > >

Re: [PATCH net-next v5 03/11] tcp: use drop reasons in cookie check for ipv4

[Jason Xing]

On Fri, Feb 16, 2024 at 12:11 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Jason Xing <kerneljasonxing@gmail.com>
> Date: Fri, 16 Feb 2024 11:50:45 +0800
> > On Fri, Feb 16, 2024 at 11:03 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > >
> > > From: Jason Xing <kerneljasonxing@gmail.com>
> > > Date: Fri, 16 Feb 2024 09:28:26 +0800
> > > > On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > > > >
> > > > > From: Jason Xing <kerneljasonxing@gmail.com>
> > > > > Date: Thu, 15 Feb 2024 09:20:19 +0800
> > > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > > >
> > > > > > Now it's time to use the prepared definitions to refine this part.
> > > > > > Four reasons used might enough for now, I think.
> > > > > >
> > > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > > > --
> > > > > > v5:
> > > > > > Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
> > > > > > Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/
> > > > > > 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David)
> > > > > > 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
> > > > > > 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
> > > > > > ---
> > > > > >  net/ipv4/syncookies.c | 18 +++++++++++++-----
> > > > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > > > > > index 38f331da6677..aeb61c880fbd 100644
> > > > > > --- a/net/ipv4/syncookies.c
> > > > > > +++ b/net/ipv4/syncookies.c
> > > > > > @@ -421,8 +421,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> > > > > >               if (IS_ERR(req))
> > > > > >                       goto out;
> > >
> > > I noticed in this case (ret == sk) we can set drop reason in
> > > tcp_v4_do_rcv() as INVALID_COOKIE or something else.
> >
> > If cookie_v4_check() returns the sk which is the same as the first
> > parameter of its caller (tcp_v4_do_rcv()), then we cannot directly
> > drop it
>
> No, I meant we can just set drop reason, not calling kfree_skb_reason()
> just after cookie_v4_check().
>
> Then, in tcp_v4_do_rcv(), the default reason is NOT_SPECIFIED, but
> INVALID_COOKIE only when cookie_v4_check() returns the listener.
>
> ---8<---
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 0c50c5a32b84..05cd697a7c07 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1923,6 +1923,8 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
>                         }
>                         return 0;
>                 }
> +
> +               reason = SKB_DROP_REASON_INVALID_COOKIE;
>         } else
>                 sock_rps_save_rxhash(sk, skb);

After this, it will go into 'reason = tcp_rcv_state_process()' which
will replace INVALID_COOKIE reason if the kernel is shipped with this
series.

>
> ---8<---
>
>
> > because it is against old behaviours and causes errors. It
> > should go into tcp_rcv_state_process() later. The similar mistake I
> > made was reported by Paolo in the patch [0/11] (see link[1] as below).
> >
> > link[1]: https://lore.kernel.org/netdev/c987d2c79e4a4655166eb8eafef473384edb37fb.camel@redhat.com/
> >
> > >
> > >
> > > > > >       }
> > > > > > -     if (!req)
> > > > > > +     if (!req) {
> > > > > > +             SKB_DR_SET(reason, NOMEM);
> > > > >
> > > > > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails.
> > > >
> > > > Thanks for your careful check. It's true. I didn't check the MPTCP
> > > > path about how to handle it.
> > > >
> > > > It also means that what I did to the cookie_v6_check() is also wrong.
> > >
> > > Yes, same for the v6 change.
> > >
> > >
> > > >
> > > > [...]
> > > > > >       /* Try to redo what tcp_v4_send_synack did. */
> > > > > >       req->rsk_window_clamp = tp->window_clamp ? :dst_metric(&rt->dst, RTAX_WINDOW);
> > > > > > @@ -476,10 +482,12 @@ 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
> > > > > > +     } else {
> > > > > > +             SKB_DR_SET(reason, NO_SOCKET);
> > > > >
> > > > > This also seems wrong to me.
> > > > >
> > > > > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk),
> > > > > then the listener is actually found.
> > > >
> > > > Initially I thought using a not-that-clear name could be helpfull,
> > > > though. NO_SOCKET here means no child socket can be used if I add a
> > > > new description to SKB_DROP_REASON_NO_SOCKET.
> > >
> > > Currently, NO_SOCKET is used only when sk lookup fails.  Mixing
> > > different reasons sounds like pushing it back to NOT_SPECIFIED.
> > > We could distinguish them by the caller IP though.
> >
> > It makes some sense, but I still think NO_SOCKET is just a mixture of
> > three kinds of cases (no sk during lookup process, no child sk, no
> > reqsk).
> > Let me think about it.
> >
> > >
> > >
> > > >
> > > > If the idea is proper, how about using NO_SOCKET for the first point
> > > > you said to explain that there is no request socket that can be used?
> > > >
> > > > If not, for both of the points you mentioned, it seems I have to add
> > > > back those two new reasons (perhaps with a better name updated)?
> > > > 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request
> > > > socket allocation in cookie_v4/6_check())
> > > > 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket
> > > > fetching in cookie_v4/6_check())
> > > >
> > > > Now I'm struggling with the name and whether I should introduce some
> > > > new reasons like what I did in the old version of the series :S
> > >
> > > Why naming is hard would be because there are multiple reasons of
> > > failure.  One way to be more specific is moving kfree_skb_reason()
> > > into callee as you did in patch 2.
> > >
> > >
> > > > If someone comes up with a good name or a good way to explain them,
> > > > please tell me, thanks!
> > >
> > > For 1. no idea :p
> > >
> > > For 2. Maybe VALID_COOKIE ?  we drop the valid cookie in the same
> > > function, but due to LSM or L3 layer, so the reason could be said
> > > as L4-specific ?
> >
> > Thanks for your idea :)
> >
> > For 2, if we're on the same page and talk about how to handle
> > tcp_get_cookie_sock(), the name is not that appropriate because as you
> > said in your previous email it could fail due to full of accept queue
> > instead of cookie problem.
>
> That's why I wrote _VALID_ COOKIE.  Here we know the cookie was valid
> but somehow 3WHS failed.  If we want to be more specific, what is not
> appropriate would be the place where we set the reason or call kfree_skb().

Ah, I see. It does make sense if we use __VALID__. Let us hear more
voices, then I can accurately update the changes in the next version
:)

Really hope it can be done soon. It almost killed me :(

Thanks,
Jason

>
>
> >
> > If we're talking about cookie_tcp_check(), the name is also not that
> > good because the drop reason could be no memory which is unrelated to
> > cookie, right?
> >
> > COOKIE, it seems, cannot be the keyword/generic reason to conclude all
> > the reasons for either of them...
> >
> > Thanks,
> > Jason
> >
> > >
> > >
> > > >
> > > > also cc Eric, David
> > > >
> > > > Thanks,
> > > > Jason
> > > >