[PATCH net-next v4 0/6] Implement reset reason mechanism to detect

[PATCH net-next v4 0/6] Implement reset reason mechanism to detect

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

In production, there are so many cases about why the RST skb is sent but
we don't have a very convenient/fast method to detect the exact underlying
reasons.

RST is implemented in two kinds: passive kind (like tcp_v4_send_reset())
and active kind (like tcp_send_active_reset()). The former can be traced
carefully 1) in TCP, with the help of drop reasons, which is based on
Eric's idea[1], 2) in MPTCP, with the help of reset options defined in
RFC 8684. The latter is relatively independent, which should be
implemented on our own.

In this series, I focus on the fundamental implement mostly about how
the rstreason mechnism works and give the detailed passive part as an
example, not including the active reset part. In future, we can go
further and refine those NOT_SPECIFIED reasons.

Here are some examples when tracing:
<idle>-0       [002] ..s1.  1830.262425: tcp_send_reset: skbaddr=x
        skaddr=x src=x dest=x state=x reason=NOT_SPECIFIED
<idle>-0       [002] ..s1.  1830.262425: tcp_send_reset: skbaddr=x
        skaddr=x src=x dest=x state=x reason=NO_SOCKET

[1]
Link: https://lore.kernel.org/all/CANn89iJw8x-LqgsWOeJQQvgVg6DnL5aBRLi10QN2WBdr+X4k=w@mail.gmail.com/

v4
Link: https://lore.kernel.org/all/20240409100934.37725-1-kerneljasonxing@gmail.com/
1. passing 'enum sk_rst_reason' for readability when tracing (Antoine)

v3
Link: https://lore.kernel.org/all/20240404072047.11490-1-kerneljasonxing@gmail.com/
1. rebase (mptcp part) and address what Mat suggested.

v2
Link: https://lore.kernel.org/all/20240403185033.47ebc6a9@kernel.org/
1. rebase against the latest net-next tree


Jason Xing (6):
  net: introduce rstreason to detect why the RST is sent
  rstreason: prepare for passive reset
  rstreason: prepare for active reset
  tcp: support rstreason for passive reset
  mptcp: support rstreason for passive reset
  rstreason: make it work in trace world

 include/net/request_sock.h |  4 +-
 include/net/rstreason.h    | 93 ++++++++++++++++++++++++++++++++++++++
 include/net/tcp.h          |  3 +-
 include/trace/events/tcp.h | 37 +++++++++++++--
 net/dccp/ipv4.c            | 10 ++--
 net/dccp/ipv6.c            | 10 ++--
 net/dccp/minisocks.c       |  3 +-
 net/ipv4/tcp.c             | 15 ++++--
 net/ipv4/tcp_ipv4.c        | 14 +++---
 net/ipv4/tcp_minisocks.c   |  3 +-
 net/ipv4/tcp_output.c      |  5 +-
 net/ipv4/tcp_timer.c       |  9 ++--
 net/ipv6/tcp_ipv6.c        | 17 ++++---
 net/mptcp/protocol.c       |  4 +-
 net/mptcp/subflow.c        | 27 ++++++++---
 15 files changed, 207 insertions(+), 47 deletions(-)
 create mode 100644 include/net/rstreason.h

-- 
2.37.3

[PATCH net-next v4 1/6] net: introduce rstreason to detect why the RST is sent

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Add a new standalone file for the easy future extension to support
both active reset and passive reset in the TCP/DCCP/MPTCP protocols.

This patch only does the preparations for reset reason mechanism,
nothing else changes.

The reset reasons are divided into three parts:
1) reuse drop reasons for passive reset in TCP
2) reuse MP_TCPRST option for MPTCP
3) our own reasons

I will implement the basic codes of active/passive reset reason in
those three protocols, which is not complete for this moment. But
it provides a new chance to let other people add more reasons into
it:)

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/rstreason.h | 93 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 include/net/rstreason.h

diff --git a/include/net/rstreason.h b/include/net/rstreason.h
new file mode 100644
index 000000000000..38c39d32a961
--- /dev/null
+++ b/include/net/rstreason.h
@@ -0,0 +1,93 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _LINUX_RSTREASON_H
+#define _LINUX_RSTREASON_H
+#include <net/dropreason-core.h>
+
+#define DEFINE_RST_REASON(FN, FNe)	\
+	FN(MPTCP_RST_EUNSPEC)		\
+	FN(MPTCP_RST_EMPTCP)		\
+	FN(MPTCP_RST_ERESOURCE)		\
+	FN(MPTCP_RST_EPROHIBIT)		\
+	FN(MPTCP_RST_EWQ2BIG)		\
+	FN(MPTCP_RST_EBADPERF)		\
+	FN(MPTCP_RST_EMIDDLEBOX)	\
+	FN(NOT_SPECIFIED)		\
+	FNe(MAX)
+
+#define RST_REASON_START (SKB_DROP_REASON_MAX + 1)
+
+/* There are three parts in order:
+ * 1) 0 - SKB_DROP_REASON_MAX: rely on drop reasons for passive reset in TCP
+ * 2) SKB_DROP_REASON_MAX + 1 - MPTCP_RST_EMIDDLEBOX: for MPTCP use
+ * 3) MPTCP_RST_EMIDDLEBOX - SK_RST_REASON_MAX: independent reset reason
+ */
+enum sk_rst_reason {
+	/* Leave this 'blank' part (0-SKB_DROP_REASON_MAX) for the reuse
+	 * of skb drop reason because rst reason relies on what drop reason
+	 * indicates exactly why it could happen.
+	 */
+
+	/* Copy from include/uapi/linux/mptcp.h.
+	 * These reset fields will not be changed since they adhere to
+	 * RFC 8684. So do not touch them. I'm going to list each definition
+	 * of them respectively.
+	 */
+	/* Unspecified error.
+	 * This is the default error; it implies that the subflow is no
+	 * longer available. The presence of this option shows that the
+	 * RST was generated by an MPTCP-aware device.
+	 */
+	SK_RST_REASON_MPTCP_RST_EUNSPEC = RST_REASON_START,
+	/* MPTCP-specific error.
+	 * An error has been detected in the processing of MPTCP options.
+	 * This is the usual reason code to return in the cases where a RST
+	 * is being sent to close a subflow because of an invalid response.
+	 */
+	SK_RST_REASON_MPTCP_RST_EMPTCP,
+	/* Lack of resources.
+	 * This code indicates that the sending host does not have enough
+	 * resources to support the terminated subflow.
+	 */
+	SK_RST_REASON_MPTCP_RST_ERESOURCE,
+	/* Administratively prohibited.
+	 * This code indicates that the requested subflow is prohibited by
+	 * the policies of the sending host.
+	 */
+	SK_RST_REASON_MPTCP_RST_EPROHIBIT,
+	/* Too much outstanding data.
+	 * This code indicates that there is an excessive amount of data
+	 * that needs to be transmitted over the terminated subflow while
+	 * having already been acknowledged over one or more other subflows.
+	 * This may occur if a path has been unavailable for a short period
+	 * and it is more efficient to reset and start again than it is to
+	 * retransmit the queued data.
+	 */
+	SK_RST_REASON_MPTCP_RST_EWQ2BIG,
+	/* Unacceptable performance.
+	 * This code indicates that the performance of this subflow was
+	 * too low compared to the other subflows of this Multipath TCP
+	 * connection.
+	 */
+	SK_RST_REASON_MPTCP_RST_EBADPERF,
+	/* Middlebox interference.
+	 * Middlebox interference has been detected over this subflow,
+	 * making MPTCP signaling invalid. For example, this may be sent
+	 * if the checksum does not validate.
+	 */
+	SK_RST_REASON_MPTCP_RST_EMIDDLEBOX,
+
+	/* For the real standalone socket reset reason, we start from here */
+	SK_RST_REASON_NOT_SPECIFIED,
+
+	/* Maximum of socket reset reasons.
+	 * It shouldn't be used as a real 'reason'.
+	 */
+	SK_RST_REASON_MAX,
+};
+
+static inline enum sk_rst_reason convert_mptcp_reason(int reason)
+{
+	return reason += RST_REASON_START;
+}
+#endif
-- 
2.37.3

[PATCH net-next v4 2/6] rstreason: prepare for passive reset

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Adjust the parameter and support passing reason of reset which
is for now NOT_SPECIFIED. No functional changes.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/request_sock.h |  4 +++-
 net/dccp/ipv4.c            | 10 ++++++----
 net/dccp/ipv6.c            | 10 ++++++----
 net/dccp/minisocks.c       |  3 ++-
 net/ipv4/tcp_ipv4.c        | 12 +++++++-----
 net/ipv4/tcp_minisocks.c   |  3 ++-
 net/ipv6/tcp_ipv6.c        | 15 +++++++++------
 net/mptcp/subflow.c        |  8 +++++---
 8 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 004e651e6067..bdc737832da6 100644
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h
@@ -18,6 +18,7 @@
 #include <linux/refcount.h>
 
 #include <net/sock.h>
+#include <net/rstreason.h>
 
 struct request_sock;
 struct sk_buff;
@@ -34,7 +35,8 @@ struct request_sock_ops {
 	void		(*send_ack)(const struct sock *sk, struct sk_buff *skb,
 				    struct request_sock *req);
 	void		(*send_reset)(const struct sock *sk,
-				      struct sk_buff *skb);
+				      struct sk_buff *skb,
+				      enum sk_rst_reason reason);
 	void		(*destructor)(struct request_sock *req);
 	void		(*syn_ack_timeout)(const struct request_sock *req);
 };
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index 9fc9cea4c251..ff41bd6f99c3 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -24,6 +24,7 @@
 #include <net/xfrm.h>
 #include <net/secure_seq.h>
 #include <net/netns/generic.h>
+#include <net/rstreason.h>
 
 #include "ackvec.h"
 #include "ccid.h"
@@ -521,7 +522,8 @@ static int dccp_v4_send_response(const struct sock *sk, struct request_sock *req
 	return err;
 }
 
-static void dccp_v4_ctl_send_reset(const struct sock *sk, struct sk_buff *rxskb)
+static void dccp_v4_ctl_send_reset(const struct sock *sk, struct sk_buff *rxskb,
+				   enum sk_rst_reason reason)
 {
 	int err;
 	const struct iphdr *rxiph;
@@ -706,7 +708,7 @@ int dccp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 	return 0;
 
 reset:
-	dccp_v4_ctl_send_reset(sk, skb);
+	dccp_v4_ctl_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 	kfree_skb(skb);
 	return 0;
 }
@@ -869,7 +871,7 @@ static int dccp_v4_rcv(struct sk_buff *skb)
 		if (nsk == sk) {
 			reqsk_put(req);
 		} else if (dccp_child_process(sk, nsk, skb)) {
-			dccp_v4_ctl_send_reset(sk, skb);
+			dccp_v4_ctl_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 			goto discard_and_relse;
 		} else {
 			sock_put(sk);
@@ -909,7 +911,7 @@ static int dccp_v4_rcv(struct sk_buff *skb)
 	if (dh->dccph_type != DCCP_PKT_RESET) {
 		DCCP_SKB_CB(skb)->dccpd_reset_code =
 					DCCP_RESET_CODE_NO_CONNECTION;
-		dccp_v4_ctl_send_reset(sk, skb);
+		dccp_v4_ctl_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 	}
 
 discard_it:
diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index c8ca703dc331..85f4b8fdbe5e 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -29,6 +29,7 @@
 #include <net/secure_seq.h>
 #include <net/netns/generic.h>
 #include <net/sock.h>
+#include <net/rstreason.h>
 
 #include "dccp.h"
 #include "ipv6.h"
@@ -256,7 +257,8 @@ static void dccp_v6_reqsk_destructor(struct request_sock *req)
 	kfree_skb(inet_rsk(req)->pktopts);
 }
 
-static void dccp_v6_ctl_send_reset(const struct sock *sk, struct sk_buff *rxskb)
+static void dccp_v6_ctl_send_reset(const struct sock *sk, struct sk_buff *rxskb,
+				   enum sk_rst_reason reason)
 {
 	const struct ipv6hdr *rxip6h;
 	struct sk_buff *skb;
@@ -656,7 +658,7 @@ static int dccp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 	return 0;
 
 reset:
-	dccp_v6_ctl_send_reset(sk, skb);
+	dccp_v6_ctl_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 discard:
 	if (opt_skb != NULL)
 		__kfree_skb(opt_skb);
@@ -762,7 +764,7 @@ static int dccp_v6_rcv(struct sk_buff *skb)
 		if (nsk == sk) {
 			reqsk_put(req);
 		} else if (dccp_child_process(sk, nsk, skb)) {
-			dccp_v6_ctl_send_reset(sk, skb);
+			dccp_v6_ctl_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 			goto discard_and_relse;
 		} else {
 			sock_put(sk);
@@ -801,7 +803,7 @@ static int dccp_v6_rcv(struct sk_buff *skb)
 	if (dh->dccph_type != DCCP_PKT_RESET) {
 		DCCP_SKB_CB(skb)->dccpd_reset_code =
 					DCCP_RESET_CODE_NO_CONNECTION;
-		dccp_v6_ctl_send_reset(sk, skb);
+		dccp_v6_ctl_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 	}
 
 discard_it:
diff --git a/net/dccp/minisocks.c b/net/dccp/minisocks.c
index 64d805b27add..251a57cf5822 100644
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -15,6 +15,7 @@
 #include <net/sock.h>
 #include <net/xfrm.h>
 #include <net/inet_timewait_sock.h>
+#include <net/rstreason.h>
 
 #include "ackvec.h"
 #include "ccid.h"
@@ -202,7 +203,7 @@ struct sock *dccp_check_req(struct sock *sk, struct sk_buff *skb,
 	DCCP_SKB_CB(skb)->dccpd_reset_code = DCCP_RESET_CODE_TOO_BUSY;
 drop:
 	if (dccp_hdr(skb)->dccph_type != DCCP_PKT_RESET)
-		req->rsk_ops->send_reset(sk, skb);
+		req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 
 	inet_csk_reqsk_queue_drop(sk, req);
 out:
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 52963c3bb8ca..441134aebc51 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -70,6 +70,7 @@
 #include <net/xfrm.h>
 #include <net/secure_seq.h>
 #include <net/busy_poll.h>
+#include <net/rstreason.h>
 
 #include <linux/inet.h>
 #include <linux/ipv6.h>
@@ -723,7 +724,8 @@ static bool tcp_v4_ao_sign_reset(const struct sock *sk, struct sk_buff *skb,
  *	Exception: precedence violation. We do not implement it in any case.
  */
 
-static void tcp_v4_send_reset(const struct sock *sk, struct sk_buff *skb)
+static void tcp_v4_send_reset(const struct sock *sk, struct sk_buff *skb,
+			      enum sk_rst_reason reason)
 {
 	const struct tcphdr *th = tcp_hdr(skb);
 	struct {
@@ -1933,7 +1935,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 	return 0;
 
 reset:
-	tcp_v4_send_reset(rsk, skb);
+	tcp_v4_send_reset(rsk, skb, SK_RST_REASON_NOT_SPECIFIED);
 discard:
 	kfree_skb_reason(skb, reason);
 	/* Be careful here. If this function gets more complicated and
@@ -2276,7 +2278,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		} else {
 			drop_reason = tcp_child_process(sk, nsk, skb);
 			if (drop_reason) {
-				tcp_v4_send_reset(nsk, skb);
+				tcp_v4_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
 				goto discard_and_relse;
 			}
 			sock_put(sk);
@@ -2354,7 +2356,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 bad_packet:
 		__TCP_INC_STATS(net, TCP_MIB_INERRS);
 	} else {
-		tcp_v4_send_reset(NULL, skb);
+		tcp_v4_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
 	}
 
 discard_it:
@@ -2405,7 +2407,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		tcp_v4_timewait_ack(sk, skb);
 		break;
 	case TCP_TW_RST:
-		tcp_v4_send_reset(sk, skb);
+		tcp_v4_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 		inet_twsk_deschedule_put(inet_twsk(sk));
 		goto discard_it;
 	case TCP_TW_SUCCESS:;
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 5b21a07ddf9a..2d7d81428c07 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -22,6 +22,7 @@
 #include <net/tcp.h>
 #include <net/xfrm.h>
 #include <net/busy_poll.h>
+#include <net/rstreason.h>
 
 static bool tcp_in_window(u32 seq, u32 end_seq, u32 s_win, u32 e_win)
 {
@@ -879,7 +880,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 		 * avoid becoming vulnerable to outside attack aiming at
 		 * resetting legit local connections.
 		 */
-		req->rsk_ops->send_reset(sk, skb);
+		req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 	} else if (fastopen) { /* received a valid RST pkt */
 		reqsk_fastopen_remove(sk, req, true);
 		tcp_reset(sk, skb);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index cffebaec66f1..6cad32430a12 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -60,6 +60,7 @@
 #include <net/secure_seq.h>
 #include <net/hotdata.h>
 #include <net/busy_poll.h>
+#include <net/rstreason.h>
 
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
@@ -69,7 +70,8 @@
 
 #include <trace/events/tcp.h>
 
-static void	tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb);
+static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb,
+			      enum sk_rst_reason reason);
 static void	tcp_v6_reqsk_send_ack(const struct sock *sk, struct sk_buff *skb,
 				      struct request_sock *req);
 
@@ -1006,7 +1008,8 @@ static void tcp_v6_send_response(const struct sock *sk, struct sk_buff *skb, u32
 	kfree_skb(buff);
 }
 
-static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
+static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb,
+			      enum sk_rst_reason reason)
 {
 	const struct tcphdr *th = tcp_hdr(skb);
 	struct ipv6hdr *ipv6h = ipv6_hdr(skb);
@@ -1675,7 +1678,7 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 	return 0;
 
 reset:
-	tcp_v6_send_reset(sk, skb);
+	tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 discard:
 	if (opt_skb)
 		__kfree_skb(opt_skb);
@@ -1861,7 +1864,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 		} else {
 			drop_reason = tcp_child_process(sk, nsk, skb);
 			if (drop_reason) {
-				tcp_v6_send_reset(nsk, skb);
+				tcp_v6_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
 				goto discard_and_relse;
 			}
 			sock_put(sk);
@@ -1937,7 +1940,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 bad_packet:
 		__TCP_INC_STATS(net, TCP_MIB_INERRS);
 	} else {
-		tcp_v6_send_reset(NULL, skb);
+		tcp_v6_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
 	}
 
 discard_it:
@@ -1992,7 +1995,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 		tcp_v6_timewait_ack(sk, skb);
 		break;
 	case TCP_TW_RST:
-		tcp_v6_send_reset(sk, skb);
+		tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 		inet_twsk_deschedule_put(inet_twsk(sk));
 		goto discard_it;
 	case TCP_TW_SUCCESS:
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 162b218d9858..744c87b6d5a4 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -20,6 +20,8 @@
 #include <net/transp_v6.h>
 #endif
 #include <net/mptcp.h>
+#include <net/rstreason.h>
+
 #include "protocol.h"
 #include "mib.h"
 
@@ -307,7 +309,7 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 
 	dst_release(dst);
 	if (!req->syncookie)
-		tcp_request_sock_ops.send_reset(sk, skb);
+		tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 	return NULL;
 }
 
@@ -374,7 +376,7 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 
 	dst_release(dst);
 	if (!req->syncookie)
-		tcp6_request_sock_ops.send_reset(sk, skb);
+		tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 	return NULL;
 }
 #endif
@@ -909,7 +911,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	tcp_rsk(req)->drop_req = true;
 	inet_csk_prepare_for_destroy_sock(child);
 	tcp_done(child);
-	req->rsk_ops->send_reset(sk, skb);
+	req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
 
 	/* The last child reference will be released by the caller */
 	return child;
-- 
2.37.3

[PATCH net-next v4 3/6] rstreason: prepare for active reset

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Like what we did to passive reset:
only passing possible reset reason in each active reset path.

No functional changes.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/tcp.h     |  3 ++-
 net/ipv4/tcp.c        | 15 ++++++++++-----
 net/ipv4/tcp_output.c |  3 ++-
 net/ipv4/tcp_timer.c  |  9 ++++++---
 net/mptcp/protocol.c  |  4 +++-
 net/mptcp/subflow.c   |  5 +++--
 6 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 9ab5b37e9d53..aa40913870e1 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -667,7 +667,8 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
 void tcp_send_probe0(struct sock *);
 int tcp_write_wakeup(struct sock *, int mib);
 void tcp_send_fin(struct sock *sk);
-void tcp_send_active_reset(struct sock *sk, gfp_t priority);
+void tcp_send_active_reset(struct sock *sk, gfp_t priority,
+			   enum sk_rst_reason reason);
 int tcp_send_synack(struct sock *);
 void tcp_push_one(struct sock *, unsigned int mss_now);
 void __tcp_send_ack(struct sock *sk, u32 rcv_nxt);
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 664c8ecb076b..d1610d4deb8f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -275,6 +275,7 @@
 #include <net/xfrm.h>
 #include <net/ip.h>
 #include <net/sock.h>
+#include <net/rstreason.h>
 
 #include <linux/uaccess.h>
 #include <asm/ioctls.h>
@@ -2805,7 +2806,8 @@ void __tcp_close(struct sock *sk, long timeout)
 		/* Unread data was tossed, zap the connection. */
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONCLOSE);
 		tcp_set_state(sk, TCP_CLOSE);
-		tcp_send_active_reset(sk, sk->sk_allocation);
+		tcp_send_active_reset(sk, sk->sk_allocation,
+				      SK_RST_REASON_NOT_SPECIFIED);
 	} else if (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime) {
 		/* Check zero linger _after_ checking for unread data. */
 		sk->sk_prot->disconnect(sk, 0);
@@ -2879,7 +2881,8 @@ void __tcp_close(struct sock *sk, long timeout)
 		struct tcp_sock *tp = tcp_sk(sk);
 		if (READ_ONCE(tp->linger2) < 0) {
 			tcp_set_state(sk, TCP_CLOSE);
-			tcp_send_active_reset(sk, GFP_ATOMIC);
+			tcp_send_active_reset(sk, GFP_ATOMIC,
+					      SK_RST_REASON_NOT_SPECIFIED);
 			__NET_INC_STATS(sock_net(sk),
 					LINUX_MIB_TCPABORTONLINGER);
 		} else {
@@ -2897,7 +2900,8 @@ void __tcp_close(struct sock *sk, long timeout)
 	if (sk->sk_state != TCP_CLOSE) {
 		if (tcp_check_oom(sk, 0)) {
 			tcp_set_state(sk, TCP_CLOSE);
-			tcp_send_active_reset(sk, GFP_ATOMIC);
+			tcp_send_active_reset(sk, GFP_ATOMIC,
+					      SK_RST_REASON_NOT_SPECIFIED);
 			__NET_INC_STATS(sock_net(sk),
 					LINUX_MIB_TCPABORTONMEMORY);
 		} else if (!check_net(sock_net(sk))) {
@@ -3001,7 +3005,7 @@ int tcp_disconnect(struct sock *sk, int flags)
 		/* The last check adjusts for discrepancy of Linux wrt. RFC
 		 * states
 		 */
-		tcp_send_active_reset(sk, gfp_any());
+		tcp_send_active_reset(sk, gfp_any(), SK_RST_REASON_NOT_SPECIFIED);
 		WRITE_ONCE(sk->sk_err, ECONNRESET);
 	} else if (old_state == TCP_SYN_SENT)
 		WRITE_ONCE(sk->sk_err, ECONNRESET);
@@ -4557,7 +4561,8 @@ int tcp_abort(struct sock *sk, int err)
 		smp_wmb();
 		sk_error_report(sk);
 		if (tcp_need_reset(sk->sk_state))
-			tcp_send_active_reset(sk, GFP_ATOMIC);
+			tcp_send_active_reset(sk, GFP_ATOMIC,
+					      SK_RST_REASON_NOT_SPECIFIED);
 		tcp_done(sk);
 	}
 
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 9282fafc0e61..5d8eab15f462 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3585,7 +3585,8 @@ void tcp_send_fin(struct sock *sk)
  * was unread data in the receive queue.  This behavior is recommended
  * by RFC 2525, section 2.17.  -DaveM
  */
-void tcp_send_active_reset(struct sock *sk, gfp_t priority)
+void tcp_send_active_reset(struct sock *sk, gfp_t priority,
+			   enum sk_rst_reason reason)
 {
 	struct sk_buff *skb;
 
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 976db57b95d4..83fe7f62f7f1 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/gfp.h>
 #include <net/tcp.h>
+#include <net/rstreason.h>
 
 static u32 tcp_clamp_rto_to_user_timeout(const struct sock *sk)
 {
@@ -127,7 +128,8 @@ static int tcp_out_of_resources(struct sock *sk, bool do_reset)
 		    (!tp->snd_wnd && !tp->packets_out))
 			do_reset = true;
 		if (do_reset)
-			tcp_send_active_reset(sk, GFP_ATOMIC);
+			tcp_send_active_reset(sk, GFP_ATOMIC,
+					      SK_RST_REASON_NOT_SPECIFIED);
 		tcp_done(sk);
 		__NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONMEMORY);
 		return 1;
@@ -768,7 +770,7 @@ static void tcp_keepalive_timer (struct timer_list *t)
 				goto out;
 			}
 		}
-		tcp_send_active_reset(sk, GFP_ATOMIC);
+		tcp_send_active_reset(sk, GFP_ATOMIC, SK_RST_REASON_NOT_SPECIFIED);
 		goto death;
 	}
 
@@ -795,7 +797,8 @@ static void tcp_keepalive_timer (struct timer_list *t)
 		    icsk->icsk_probes_out > 0) ||
 		    (user_timeout == 0 &&
 		    icsk->icsk_probes_out >= keepalive_probes(tp))) {
-			tcp_send_active_reset(sk, GFP_ATOMIC);
+			tcp_send_active_reset(sk, GFP_ATOMIC,
+					      SK_RST_REASON_NOT_SPECIFIED);
 			tcp_write_err(sk);
 			goto out;
 		}
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 995b53cd021c..212a1db3b2e3 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -21,6 +21,7 @@
 #endif
 #include <net/mptcp.h>
 #include <net/xfrm.h>
+#include <net/rstreason.h>
 #include <asm/ioctls.h>
 #include "protocol.h"
 #include "mib.h"
@@ -2565,7 +2566,8 @@ static void mptcp_check_fastclose(struct mptcp_sock *msk)
 
 		slow = lock_sock_fast(tcp_sk);
 		if (tcp_sk->sk_state != TCP_CLOSE) {
-			tcp_send_active_reset(tcp_sk, GFP_ATOMIC);
+			tcp_send_active_reset(tcp_sk, GFP_ATOMIC,
+					      SK_RST_REASON_NOT_SPECIFIED);
 			tcp_set_state(tcp_sk, TCP_CLOSE);
 		}
 		unlock_sock_fast(tcp_sk, slow);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 744c87b6d5a4..ba0a252c113f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -412,7 +412,7 @@ void mptcp_subflow_reset(struct sock *ssk)
 	/* must hold: tcp_done() could drop last reference on parent */
 	sock_hold(sk);
 
-	tcp_send_active_reset(ssk, GFP_ATOMIC);
+	tcp_send_active_reset(ssk, GFP_ATOMIC, SK_RST_REASON_NOT_SPECIFIED);
 	tcp_done(ssk);
 	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags))
 		mptcp_schedule_work(sk);
@@ -1348,7 +1348,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
 			tcp_set_state(ssk, TCP_CLOSE);
 			while ((skb = skb_peek(&ssk->sk_receive_queue)))
 				sk_eat_skb(ssk, skb);
-			tcp_send_active_reset(ssk, GFP_ATOMIC);
+			tcp_send_active_reset(ssk, GFP_ATOMIC,
+					      SK_RST_REASON_NOT_SPECIFIED);
 			WRITE_ONCE(subflow->data_avail, false);
 			return false;
 		}
-- 
2.37.3

[PATCH net-next v4 4/6] tcp: support rstreason for passive reset

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Reuse the dropreason logic to show the exact reason of tcp reset,
so we don't need to implement those duplicated reset reasons.
This patch replaces all the prior NOT_SPECIFIED reasons.

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

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 441134aebc51..863397c2a47b 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1935,7 +1935,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 	return 0;
 
 reset:
-	tcp_v4_send_reset(rsk, skb, SK_RST_REASON_NOT_SPECIFIED);
+	tcp_v4_send_reset(rsk, skb, (u32)reason);
 discard:
 	kfree_skb_reason(skb, reason);
 	/* Be careful here. If this function gets more complicated and
@@ -2278,7 +2278,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		} else {
 			drop_reason = tcp_child_process(sk, nsk, skb);
 			if (drop_reason) {
-				tcp_v4_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
+				tcp_v4_send_reset(nsk, skb, (u32)drop_reason);
 				goto discard_and_relse;
 			}
 			sock_put(sk);
@@ -2356,7 +2356,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 bad_packet:
 		__TCP_INC_STATS(net, TCP_MIB_INERRS);
 	} else {
-		tcp_v4_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
+		tcp_v4_send_reset(NULL, skb, (u32)drop_reason);
 	}
 
 discard_it:
@@ -2407,7 +2407,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		tcp_v4_timewait_ack(sk, skb);
 		break;
 	case TCP_TW_RST:
-		tcp_v4_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+		tcp_v4_send_reset(sk, skb, (u32)drop_reason);
 		inet_twsk_deschedule_put(inet_twsk(sk));
 		goto discard_it;
 	case TCP_TW_SUCCESS:;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 6cad32430a12..ba9d9ceb7e89 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1678,7 +1678,7 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 	return 0;
 
 reset:
-	tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+	tcp_v6_send_reset(sk, skb, (u32)reason);
 discard:
 	if (opt_skb)
 		__kfree_skb(opt_skb);
@@ -1864,7 +1864,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 		} else {
 			drop_reason = tcp_child_process(sk, nsk, skb);
 			if (drop_reason) {
-				tcp_v6_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
+				tcp_v6_send_reset(nsk, skb, (u32)drop_reason);
 				goto discard_and_relse;
 			}
 			sock_put(sk);
@@ -1940,7 +1940,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 bad_packet:
 		__TCP_INC_STATS(net, TCP_MIB_INERRS);
 	} else {
-		tcp_v6_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
+		tcp_v6_send_reset(NULL, skb, (u32)drop_reason);
 	}
 
 discard_it:
@@ -1995,7 +1995,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 		tcp_v6_timewait_ack(sk, skb);
 		break;
 	case TCP_TW_RST:
-		tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+		tcp_v6_send_reset(sk, skb, (u32)drop_reason);
 		inet_twsk_deschedule_put(inet_twsk(sk));
 		goto discard_it;
 	case TCP_TW_SUCCESS:
-- 
2.37.3

[PATCH net-next v4 5/6] mptcp: support rstreason for passive reset

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

It relys on what reset options in the skb are as rfc8684 says. Reusing
this logic can save us much energy. This patch replaces most of the prior
NOT_SPECIFIED reasons.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/mptcp/subflow.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index ba0a252c113f..25eaad94cb79 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -308,8 +308,12 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 		return dst;
 
 	dst_release(dst);
-	if (!req->syncookie)
-		tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+	if (!req->syncookie) {
+		struct mptcp_ext *mpext = mptcp_get_ext(skb);
+		enum sk_rst_reason reason = convert_mptcp_reason(mpext->reset_reason);
+
+		tcp_request_sock_ops.send_reset(sk, skb, reason);
+	}
 	return NULL;
 }
 
@@ -375,8 +379,12 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 		return dst;
 
 	dst_release(dst);
-	if (!req->syncookie)
-		tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+	if (!req->syncookie) {
+		struct mptcp_ext *mpext = mptcp_get_ext(skb);
+		enum sk_rst_reason reason = convert_mptcp_reason(mpext->reset_reason);
+
+		tcp6_request_sock_ops.send_reset(sk, skb, reason);
+	}
 	return NULL;
 }
 #endif
@@ -783,6 +791,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	bool fallback, fallback_is_fatal;
 	struct mptcp_sock *owner;
 	struct sock *child;
+	enum sk_rst_reason reason;
 
 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
 
@@ -911,7 +920,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	tcp_rsk(req)->drop_req = true;
 	inet_csk_prepare_for_destroy_sock(child);
 	tcp_done(child);
-	req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+	reason = convert_mptcp_reason(mptcp_get_ext(skb)->reset_reason);
+	req->rsk_ops->send_reset(sk, skb, reason);
 
 	/* The last child reference will be released by the caller */
 	return child;
-- 
2.37.3

[PATCH net-next v4 6/6] rstreason: make it work in trace world

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

At last, we should let it work by introducing this reset reason in
trace world.

One of the possible expected outputs is:
... tcp_send_reset: skbaddr=xxx skaddr=xxx src=xxx dest=xxx
state=TCP_ESTABLISHED reason=NOT_SPECIFIED

Signed-off-by: Jason Xing <kernelxing@tencent.com>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/trace/events/tcp.h | 37 +++++++++++++++++++++++++++++++++----
 net/ipv4/tcp_ipv4.c        |  2 +-
 net/ipv4/tcp_output.c      |  2 +-
 net/ipv6/tcp_ipv6.c        |  2 +-
 4 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index 5c04a61a11c2..b1455cbc0634 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -11,6 +11,7 @@
 #include <net/ipv6.h>
 #include <net/tcp.h>
 #include <linux/sock_diag.h>
+#include <net/rstreason.h>
 
 /*
  * tcp event with arguments sk and skb
@@ -74,20 +75,38 @@ DEFINE_EVENT(tcp_event_sk_skb, tcp_retransmit_skb,
 	TP_ARGS(sk, skb)
 );
 
+#undef FN1
+#define FN1(reason)	TRACE_DEFINE_ENUM(SK_RST_REASON_##reason);
+#undef FN2
+#define FN2(reason)	TRACE_DEFINE_ENUM(SKB_DROP_REASON_##reason);
+DEFINE_RST_REASON(FN1, FN1)
+
+#undef FN1
+#undef FNe1
+#define FN1(reason)	{ SK_RST_REASON_##reason, #reason },
+#define FNe1(reason)	{ SK_RST_REASON_##reason, #reason }
+
+#undef FN2
+#undef FNe2
+#define FN2(reason)	{ SKB_DROP_REASON_##reason, #reason },
+#define FNe2(reason)	{ SKB_DROP_REASON_##reason, #reason }
 /*
  * skb of trace_tcp_send_reset is the skb that caused RST. In case of
  * active reset, skb should be NULL
  */
 TRACE_EVENT(tcp_send_reset,
 
-	TP_PROTO(const struct sock *sk, const struct sk_buff *skb),
+	TP_PROTO(const struct sock *sk,
+		 const struct sk_buff *skb,
+		 const enum sk_rst_reason reason),
 
-	TP_ARGS(sk, skb),
+	TP_ARGS(sk, skb, reason),
 
 	TP_STRUCT__entry(
 		__field(const void *, skbaddr)
 		__field(const void *, skaddr)
 		__field(int, state)
+		__field(enum sk_rst_reason, reason)
 		__array(__u8, saddr, sizeof(struct sockaddr_in6))
 		__array(__u8, daddr, sizeof(struct sockaddr_in6))
 	),
@@ -113,14 +132,24 @@ TRACE_EVENT(tcp_send_reset,
 			 */
 			TP_STORE_ADDR_PORTS_SKB(skb, th, entry->daddr, entry->saddr);
 		}
+		__entry->reason = reason;
 	),
 
-	TP_printk("skbaddr=%p skaddr=%p src=%pISpc dest=%pISpc state=%s",
+	TP_printk("skbaddr=%p skaddr=%p src=%pISpc dest=%pISpc state=%s reason=%s",
 		  __entry->skbaddr, __entry->skaddr,
 		  __entry->saddr, __entry->daddr,
-		  __entry->state ? show_tcp_state_name(__entry->state) : "UNKNOWN")
+		  __entry->state ? show_tcp_state_name(__entry->state) : "UNKNOWN",
+		  __entry->reason < RST_REASON_START ?
+			__print_symbolic(__entry->reason, DEFINE_DROP_REASON(FN2, FNe2)) :
+			__print_symbolic(__entry->reason, DEFINE_RST_REASON(FN1, FNe1)))
 );
 
+#undef FN1
+#undef FNe1
+
+#undef FN2
+#undef FNe2
+
 /*
  * tcp event with arguments sk
  *
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 863397c2a47b..b35bb4a52464 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -871,7 +871,7 @@ static void tcp_v4_send_reset(const struct sock *sk, struct sk_buff *skb,
 	if (sk)
 		arg.bound_dev_if = sk->sk_bound_dev_if;
 
-	trace_tcp_send_reset(sk, skb);
+	trace_tcp_send_reset(sk, skb, reason);
 
 	BUILD_BUG_ON(offsetof(struct sock, sk_bound_dev_if) !=
 		     offsetof(struct inet_timewait_sock, tw_bound_dev_if));
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5d8eab15f462..196c25194acc 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3611,7 +3611,7 @@ void tcp_send_active_reset(struct sock *sk, gfp_t priority,
 	/* skb of trace_tcp_send_reset() keeps the skb that caused RST,
 	 * skb here is different to the troublesome skb, so use NULL
 	 */
-	trace_tcp_send_reset(sk, NULL);
+	trace_tcp_send_reset(sk, NULL, SK_RST_REASON_NOT_SPECIFIED);
 }
 
 /* Send a crossed SYN-ACK during socket establishment.
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index ba9d9ceb7e89..1dfe5b99476b 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1131,7 +1131,7 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb,
 			label = ip6_flowlabel(ipv6h);
 	}
 
-	trace_tcp_send_reset(sk, skb);
+	trace_tcp_send_reset(sk, skb, reason);
 
 	tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, 1,
 			     ipv6_get_dsfield(ipv6h), label, priority, txhash,
-- 
2.37.3

Re: [PATCH net-next v4 0/6] Implement reset reason mechanism to detect

[MPTCP CI]

Hi Jason,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8646443837

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ce1857255013
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=843592


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH net-next v4 5/6] mptcp: support rstreason for passive reset

[Eric Dumazet]

On Thu, Apr 11, 2024 at 1:57 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> It relys on what reset options in the skb are as rfc8684 says. Reusing
> this logic can save us much energy. This patch replaces most of the prior
> NOT_SPECIFIED reasons.
>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  net/mptcp/subflow.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index ba0a252c113f..25eaad94cb79 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -308,8 +308,12 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
>                 return dst;
>
>         dst_release(dst);
> -       if (!req->syncookie)
> -               tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +       if (!req->syncookie) {
> +               struct mptcp_ext *mpext = mptcp_get_ext(skb);
> +               enum sk_rst_reason reason = convert_mptcp_reason(mpext->reset_reason);
> +
> +               tcp_request_sock_ops.send_reset(sk, skb, reason);
> +       }
>         return NULL;
>  }
>
> @@ -375,8 +379,12 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
>                 return dst;
>
>         dst_release(dst);
> -       if (!req->syncookie)
> -               tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +       if (!req->syncookie) {
> +               struct mptcp_ext *mpext = mptcp_get_ext(skb);
> +               enum sk_rst_reason reason = convert_mptcp_reason(mpext->reset_reason);
> +
> +               tcp6_request_sock_ops.send_reset(sk, skb, reason);
> +       }
>         return NULL;
>  }
>  #endif
> @@ -783,6 +791,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>         bool fallback, fallback_is_fatal;
>         struct mptcp_sock *owner;
>         struct sock *child;
> +       enum sk_rst_reason reason;

reverse xmas tree ?

>
>         pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
>
> @@ -911,7 +920,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>         tcp_rsk(req)->drop_req = true;
>         inet_csk_prepare_for_destroy_sock(child);
>         tcp_done(child);
> -       req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +       reason = convert_mptcp_reason(mptcp_get_ext(skb)->reset_reason);
> +       req->rsk_ops->send_reset(sk, skb, reason);
>
>         /* The last child reference will be released by the caller */
>         return child;
> --
> 2.37.3
>

Re: [PATCH net-next v4 4/6] tcp: support rstreason for passive reset

[Eric Dumazet]

On Thu, Apr 11, 2024 at 1:57 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Reuse the dropreason logic to show the exact reason of tcp reset,
> so we don't need to implement those duplicated reset reasons.
> This patch replaces all the prior NOT_SPECIFIED reasons.
>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  net/ipv4/tcp_ipv4.c | 8 ++++----
>  net/ipv6/tcp_ipv6.c | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 441134aebc51..863397c2a47b 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1935,7 +1935,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
>         return 0;
>
>  reset:
> -       tcp_v4_send_reset(rsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +       tcp_v4_send_reset(rsk, skb, (u32)reason);
>  discard:
>         kfree_skb_reason(skb, reason);
>         /* Be careful here. If this function gets more complicated and
> @@ -2278,7 +2278,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>                 } else {
>                         drop_reason = tcp_child_process(sk, nsk, skb);
>                         if (drop_reason) {
> -                               tcp_v4_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +                               tcp_v4_send_reset(nsk, skb, (u32)drop_reason);

Are all these casts really needed ?

enum sk_rst_reason is not the same as u32 anyway ?



>                                 goto discard_and_relse;
>                         }
>                         sock_put(sk);
> @@ -2356,7 +2356,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>  bad_packet:
>                 __TCP_INC_STATS(net, TCP_MIB_INERRS);
>         } else {
> -               tcp_v4_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
> +               tcp_v4_send_reset(NULL, skb, (u32)drop_reason);
>         }
>
>  discard_it:
> @@ -2407,7 +2407,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>                 tcp_v4_timewait_ack(sk, skb);
>                 break;
>         case TCP_TW_RST:
> -               tcp_v4_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +               tcp_v4_send_reset(sk, skb, (u32)drop_reason);
>                 inet_twsk_deschedule_put(inet_twsk(sk));
>                 goto discard_it;
>         case TCP_TW_SUCCESS:;
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 6cad32430a12..ba9d9ceb7e89 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1678,7 +1678,7 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
>         return 0;
>
>  reset:
> -       tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +       tcp_v6_send_reset(sk, skb, (u32)reason);
>  discard:
>         if (opt_skb)
>                 __kfree_skb(opt_skb);
> @@ -1864,7 +1864,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
>                 } else {
>                         drop_reason = tcp_child_process(sk, nsk, skb);
>                         if (drop_reason) {
> -                               tcp_v6_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +                               tcp_v6_send_reset(nsk, skb, (u32)drop_reason);
>                                 goto discard_and_relse;
>                         }
>                         sock_put(sk);
> @@ -1940,7 +1940,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
>  bad_packet:
>                 __TCP_INC_STATS(net, TCP_MIB_INERRS);
>         } else {
> -               tcp_v6_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
> +               tcp_v6_send_reset(NULL, skb, (u32)drop_reason);
>         }
>
>  discard_it:
> @@ -1995,7 +1995,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
>                 tcp_v6_timewait_ack(sk, skb);
>                 break;
>         case TCP_TW_RST:
> -               tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +               tcp_v6_send_reset(sk, skb, (u32)drop_reason);
>                 inet_twsk_deschedule_put(inet_twsk(sk));
>                 goto discard_it;
>         case TCP_TW_SUCCESS:
> --
> 2.37.3
>

Re: [PATCH net-next v4 4/6] tcp: support rstreason for passive reset

[Jason Xing]

On Tue, Apr 16, 2024 at 2:34 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Apr 11, 2024 at 1:57 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Reuse the dropreason logic to show the exact reason of tcp reset,
> > so we don't need to implement those duplicated reset reasons.
> > This patch replaces all the prior NOT_SPECIFIED reasons.
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  net/ipv4/tcp_ipv4.c | 8 ++++----
> >  net/ipv6/tcp_ipv6.c | 8 ++++----
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index 441134aebc51..863397c2a47b 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -1935,7 +1935,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
> >         return 0;
> >
> >  reset:
> > -       tcp_v4_send_reset(rsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +       tcp_v4_send_reset(rsk, skb, (u32)reason);
> >  discard:
> >         kfree_skb_reason(skb, reason);
> >         /* Be careful here. If this function gets more complicated and
> > @@ -2278,7 +2278,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> >                 } else {
> >                         drop_reason = tcp_child_process(sk, nsk, skb);
> >                         if (drop_reason) {
> > -                               tcp_v4_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +                               tcp_v4_send_reset(nsk, skb, (u32)drop_reason);
>
> Are all these casts really needed ?

Not really. If without, the compiler wouldn't complain about it.

>
> enum sk_rst_reason is not the same as u32 anyway ?

I will remove the cast in the next version.

Thanks,
Jason

>
>
>
> >                                 goto discard_and_relse;
> >                         }
> >                         sock_put(sk);
> > @@ -2356,7 +2356,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> >  bad_packet:
> >                 __TCP_INC_STATS(net, TCP_MIB_INERRS);
> >         } else {
> > -               tcp_v4_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +               tcp_v4_send_reset(NULL, skb, (u32)drop_reason);
> >         }
> >
> >  discard_it:
> > @@ -2407,7 +2407,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> >                 tcp_v4_timewait_ack(sk, skb);
> >                 break;
> >         case TCP_TW_RST:
> > -               tcp_v4_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +               tcp_v4_send_reset(sk, skb, (u32)drop_reason);
> >                 inet_twsk_deschedule_put(inet_twsk(sk));
> >                 goto discard_it;
> >         case TCP_TW_SUCCESS:;
> > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> > index 6cad32430a12..ba9d9ceb7e89 100644
> > --- a/net/ipv6/tcp_ipv6.c
> > +++ b/net/ipv6/tcp_ipv6.c
> > @@ -1678,7 +1678,7 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
> >         return 0;
> >
> >  reset:
> > -       tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +       tcp_v6_send_reset(sk, skb, (u32)reason);
> >  discard:
> >         if (opt_skb)
> >                 __kfree_skb(opt_skb);
> > @@ -1864,7 +1864,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
> >                 } else {
> >                         drop_reason = tcp_child_process(sk, nsk, skb);
> >                         if (drop_reason) {
> > -                               tcp_v6_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +                               tcp_v6_send_reset(nsk, skb, (u32)drop_reason);
> >                                 goto discard_and_relse;
> >                         }
> >                         sock_put(sk);
> > @@ -1940,7 +1940,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
> >  bad_packet:
> >                 __TCP_INC_STATS(net, TCP_MIB_INERRS);
> >         } else {
> > -               tcp_v6_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +               tcp_v6_send_reset(NULL, skb, (u32)drop_reason);
> >         }
> >
> >  discard_it:
> > @@ -1995,7 +1995,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
> >                 tcp_v6_timewait_ack(sk, skb);
> >                 break;
> >         case TCP_TW_RST:
> > -               tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +               tcp_v6_send_reset(sk, skb, (u32)drop_reason);
> >                 inet_twsk_deschedule_put(inet_twsk(sk));
> >                 goto discard_it;
> >         case TCP_TW_SUCCESS:
> > --
> > 2.37.3
> >

Re: [PATCH net-next v4 5/6] mptcp: support rstreason for passive reset

[Jason Xing]

On Tue, Apr 16, 2024 at 2:33 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Apr 11, 2024 at 1:57 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > It relys on what reset options in the skb are as rfc8684 says. Reusing
> > this logic can save us much energy. This patch replaces most of the prior
> > NOT_SPECIFIED reasons.
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  net/mptcp/subflow.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index ba0a252c113f..25eaad94cb79 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -308,8 +308,12 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
> >                 return dst;
> >
> >         dst_release(dst);
> > -       if (!req->syncookie)
> > -               tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +       if (!req->syncookie) {
> > +               struct mptcp_ext *mpext = mptcp_get_ext(skb);
> > +               enum sk_rst_reason reason = convert_mptcp_reason(mpext->reset_reason);
> > +
> > +               tcp_request_sock_ops.send_reset(sk, skb, reason);
> > +       }
> >         return NULL;
> >  }
> >
> > @@ -375,8 +379,12 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
> >                 return dst;
> >
> >         dst_release(dst);
> > -       if (!req->syncookie)
> > -               tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +       if (!req->syncookie) {
> > +               struct mptcp_ext *mpext = mptcp_get_ext(skb);
> > +               enum sk_rst_reason reason = convert_mptcp_reason(mpext->reset_reason);
> > +
> > +               tcp6_request_sock_ops.send_reset(sk, skb, reason);
> > +       }
> >         return NULL;
> >  }
> >  #endif
> > @@ -783,6 +791,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> >         bool fallback, fallback_is_fatal;
> >         struct mptcp_sock *owner;
> >         struct sock *child;
> > +       enum sk_rst_reason reason;
>
> reverse xmas tree ?

Got it. Thanks!

>
> >
> >         pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
> >
> > @@ -911,7 +920,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> >         tcp_rsk(req)->drop_req = true;
> >         inet_csk_prepare_for_destroy_sock(child);
> >         tcp_done(child);
> > -       req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +       reason = convert_mptcp_reason(mptcp_get_ext(skb)->reset_reason);
> > +       req->rsk_ops->send_reset(sk, skb, reason);
> >
> >         /* The last child reference will be released by the caller */
> >         return child;
> > --
> > 2.37.3
> >

Re: [PATCH net-next v4 3/6] rstreason: prepare for active reset

[Paolo Abeni]

On Thu, 2024-04-11 at 19:56 +0800, Jason Xing wrote:
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 744c87b6d5a4..ba0a252c113f 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -412,7 +412,7 @@ void mptcp_subflow_reset(struct sock *ssk)
>  	/* must hold: tcp_done() could drop last reference on parent */
>  	sock_hold(sk);
>  
> -	tcp_send_active_reset(ssk, GFP_ATOMIC);
> +	tcp_send_active_reset(ssk, GFP_ATOMIC, SK_RST_REASON_NOT_SPECIFIED);

I'm sorry for the late feedback.

Some of the caller can set subflow->reset_reason, so probably something
alike the following:

	tcp_send_active_reset(ssk, GFP_ATOMIC,
			      subflow->reset_reason?: SK_RST_REASON_NOT_SPECIFIED);


would work - with an helper even better.

Could be a follow-up patch.

Thanks,

Paolo

Re: [PATCH net-next v4 5/6] mptcp: support rstreason for passive reset

[Paolo Abeni]

On Thu, 2024-04-11 at 19:56 +0800, Jason Xing wrote:
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index ba0a252c113f..25eaad94cb79 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -308,8 +308,12 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
>  		return dst;
>  
>  	dst_release(dst);
> -	if (!req->syncookie)
> -		tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> +	if (!req->syncookie) {
> +		struct mptcp_ext *mpext = mptcp_get_ext(skb);
> +		enum sk_rst_reason reason = convert_mptcp_reason(mpext->reset_reason);

Since you already have to repost, very minor nit: even the above
strictly speaking does not respect the reverse xmass tree.

		enum sk_rst_reason reason;

		reason = convert_mptcp_reason(mpext->reset_reason);
> +		tcp_request_sock_ops.send_reset(sk, skb, reason);

Cheers,

Paolo

Re: [PATCH net-next v4 5/6] mptcp: support rstreason for passive reset

[Jason Xing]

On Tue, Apr 16, 2024 at 4:04 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Thu, 2024-04-11 at 19:56 +0800, Jason Xing wrote:
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index ba0a252c113f..25eaad94cb79 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -308,8 +308,12 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
> >               return dst;
> >
> >       dst_release(dst);
> > -     if (!req->syncookie)
> > -             tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > +     if (!req->syncookie) {
> > +             struct mptcp_ext *mpext = mptcp_get_ext(skb);
> > +             enum sk_rst_reason reason = convert_mptcp_reason(mpext->reset_reason);
>
> Since you already have to repost, very minor nit: even the above
> strictly speaking does not respect the reverse xmass tree.
>
>                 enum sk_rst_reason reason;
>
>                 reason = convert_mptcp_reason(mpext->reset_reason);
> > +             tcp_request_sock_ops.send_reset(sk, skb, reason);

Thanks, I will update this too.

>
> Cheers,
>
> Paolo
>

Re: [PATCH net-next v4 3/6] rstreason: prepare for active reset

[Jason Xing]

On Tue, Apr 16, 2024 at 4:01 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Thu, 2024-04-11 at 19:56 +0800, Jason Xing wrote:
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 744c87b6d5a4..ba0a252c113f 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -412,7 +412,7 @@ void mptcp_subflow_reset(struct sock *ssk)
> >       /* must hold: tcp_done() could drop last reference on parent */
> >       sock_hold(sk);
> >
> > -     tcp_send_active_reset(ssk, GFP_ATOMIC);
> > +     tcp_send_active_reset(ssk, GFP_ATOMIC, SK_RST_REASON_NOT_SPECIFIED);
>
> I'm sorry for the late feedback.

Thanks for the review, Paolo :)

>
> Some of the caller can set subflow->reset_reason, so probably something
> alike the following:
>
>         tcp_send_active_reset(ssk, GFP_ATOMIC,
>                               subflow->reset_reason?: SK_RST_REASON_NOT_SPECIFIED);
>
>
> would work - with an helper even better.
>
> Could be a follow-up patch.

I see. I can add this as a separate patch when I submit soon with your
suggested-by tag because I also want to make every patch clean and
simple.

Thanks,
Jason

>
> Thanks,
>
> Paolo
>

Re: [PATCH net-next v4 4/6] tcp: support rstreason for passive reset

[Jason Xing]

On Tue, Apr 16, 2024 at 3:45 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Tue, Apr 16, 2024 at 2:34 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Apr 11, 2024 at 1:57 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Reuse the dropreason logic to show the exact reason of tcp reset,
> > > so we don't need to implement those duplicated reset reasons.
> > > This patch replaces all the prior NOT_SPECIFIED reasons.
> > >
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > ---
> > >  net/ipv4/tcp_ipv4.c | 8 ++++----
> > >  net/ipv6/tcp_ipv6.c | 8 ++++----
> > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > > index 441134aebc51..863397c2a47b 100644
> > > --- a/net/ipv4/tcp_ipv4.c
> > > +++ b/net/ipv4/tcp_ipv4.c
> > > @@ -1935,7 +1935,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
> > >         return 0;
> > >
> > >  reset:
> > > -       tcp_v4_send_reset(rsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > > +       tcp_v4_send_reset(rsk, skb, (u32)reason);
> > >  discard:
> > >         kfree_skb_reason(skb, reason);
> > >         /* Be careful here. If this function gets more complicated and
> > > @@ -2278,7 +2278,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > >                 } else {
> > >                         drop_reason = tcp_child_process(sk, nsk, skb);
> > >                         if (drop_reason) {
> > > -                               tcp_v4_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > > +                               tcp_v4_send_reset(nsk, skb, (u32)drop_reason);
> >
> > Are all these casts really needed ?
>
> Not really. If without, the compiler wouldn't complain about it.

The truth is mptcp CI treats it as an error (see link[1]) when I
submitted the V5 patchset but my machine works well. I wonder whether
I should not remove all the casts or ignore the warnings?

[1]: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8705084717/job/23874718134
net/ipv6/tcp_ipv6.c: In function 'tcp_v6_do_rcv':
4148 net/ipv6/tcp_ipv6.c:1683:36: error: implicit conversion from
'enum skb_drop_reason' to 'enum sk_rst_reason'
[-Werror=enum-conversion]
4149 1683 | tcp_v6_send_reset(sk, skb, reason);
4150 | ^~~~~~
4151 net/ipv6/tcp_ipv6.c: In function 'tcp_v6_rcv':
4152 net/ipv6/tcp_ipv6.c:1868:61: error: implicit conversion from
'enum skb_drop_reason' to 'enum sk_rst_reason'
[-Werror=enum-conversion]
4153 1868 | tcp_v6_send_reset(nsk, skb, drop_reason);
4154 | ^~~~~~~~~~~
4155 net/ipv6/tcp_ipv6.c:1945:46: error: implicit conversion from
'enum skb_drop_reason' to 'enum sk_rst_reason'
[-Werror=enum-conversion]
4156 1945 | tcp_v6_send_reset(NULL, skb, drop_reason);
4157 | ^~~~~~~~~~~
4158 net/ipv6/tcp_ipv6.c:2001:44: error: implicit conversion from
'enum skb_drop_reason' to 'enum sk_rst_reason'
[-Werror=enum-conversion]
4159 2001 | tcp_v6_send_reset(sk, skb, drop_reason);

Thanks,
Jason

>
> >
> > enum sk_rst_reason is not the same as u32 anyway ?
>
> I will remove the cast in the next version.
>
> Thanks,
> Jason
>
> >
> >
> >
> > >                                 goto discard_and_relse;
> > >                         }
> > >                         sock_put(sk);
> > > @@ -2356,7 +2356,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > >  bad_packet:
> > >                 __TCP_INC_STATS(net, TCP_MIB_INERRS);
> > >         } else {
> > > -               tcp_v4_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
> > > +               tcp_v4_send_reset(NULL, skb, (u32)drop_reason);
> > >         }
> > >
> > >  discard_it:
> > > @@ -2407,7 +2407,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> > >                 tcp_v4_timewait_ack(sk, skb);
> > >                 break;
> > >         case TCP_TW_RST:
> > > -               tcp_v4_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > > +               tcp_v4_send_reset(sk, skb, (u32)drop_reason);
> > >                 inet_twsk_deschedule_put(inet_twsk(sk));
> > >                 goto discard_it;
> > >         case TCP_TW_SUCCESS:;
> > > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> > > index 6cad32430a12..ba9d9ceb7e89 100644
> > > --- a/net/ipv6/tcp_ipv6.c
> > > +++ b/net/ipv6/tcp_ipv6.c
> > > @@ -1678,7 +1678,7 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
> > >         return 0;
> > >
> > >  reset:
> > > -       tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > > +       tcp_v6_send_reset(sk, skb, (u32)reason);
> > >  discard:
> > >         if (opt_skb)
> > >                 __kfree_skb(opt_skb);
> > > @@ -1864,7 +1864,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
> > >                 } else {
> > >                         drop_reason = tcp_child_process(sk, nsk, skb);
> > >                         if (drop_reason) {
> > > -                               tcp_v6_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > > +                               tcp_v6_send_reset(nsk, skb, (u32)drop_reason);
> > >                                 goto discard_and_relse;
> > >                         }
> > >                         sock_put(sk);
> > > @@ -1940,7 +1940,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
> > >  bad_packet:
> > >                 __TCP_INC_STATS(net, TCP_MIB_INERRS);
> > >         } else {
> > > -               tcp_v6_send_reset(NULL, skb, SK_RST_REASON_NOT_SPECIFIED);
> > > +               tcp_v6_send_reset(NULL, skb, (u32)drop_reason);
> > >         }
> > >
> > >  discard_it:
> > > @@ -1995,7 +1995,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
> > >                 tcp_v6_timewait_ack(sk, skb);
> > >                 break;
> > >         case TCP_TW_RST:
> > > -               tcp_v6_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
> > > +               tcp_v6_send_reset(sk, skb, (u32)drop_reason);
> > >                 inet_twsk_deschedule_put(inet_twsk(sk));
> > >                 goto discard_it;
> > >         case TCP_TW_SUCCESS:
> > > --
> > > 2.37.3
> > >

Re: [PATCH net-next v4 4/6] tcp: support rstreason for passive reset

[Matthieu Baerts]

Hi Jason,

16 Apr 2024 14:25:13 Jason Xing <kerneljasonxing@gmail.com>:

> On Tue, Apr 16, 2024 at 3:45 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>>
>> On Tue, Apr 16, 2024 at 2:34 PM Eric Dumazet <edumazet@google.com> wrote:
>>>
>>> On Thu, Apr 11, 2024 at 1:57 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>>>>
>>>> From: Jason Xing <kernelxing@tencent.com>
>>>>
>>>> Reuse the dropreason logic to show the exact reason of tcp reset,
>>>> so we don't need to implement those duplicated reset reasons.
>>>> This patch replaces all the prior NOT_SPECIFIED reasons.
>>>>
>>>> Signed-off-by: Jason Xing <kernelxing@tencent.com>
>>>> ---
>>>> net/ipv4/tcp_ipv4.c | 8 ++++----
>>>> net/ipv6/tcp_ipv6.c | 8 ++++----
>>>> 2 files changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
>>>> index 441134aebc51..863397c2a47b 100644
>>>> --- a/net/ipv4/tcp_ipv4.c
>>>> +++ b/net/ipv4/tcp_ipv4.c
>>>> @@ -1935,7 +1935,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
>>>>         return 0;
>>>>
>>>> reset:
>>>> -       tcp_v4_send_reset(rsk, skb, SK_RST_REASON_NOT_SPECIFIED);
>>>> +       tcp_v4_send_reset(rsk, skb, (u32)reason);
>>>> discard:
>>>>         kfree_skb_reason(skb, reason);
>>>>         /* Be careful here. If this function gets more complicated and
>>>> @@ -2278,7 +2278,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>>>                 } else {
>>>>                         drop_reason = tcp_child_process(sk, nsk, skb);
>>>>                         if (drop_reason) {
>>>> -                               tcp_v4_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
>>>> +                               tcp_v4_send_reset(nsk, skb, (u32)drop_reason);
>>>
>>> Are all these casts really needed ?
>>
>> Not really. If without, the compiler wouldn't complain about it.
>
> The truth is mptcp CI treats it as an error (see link[1]) when I
> submitted the V5 patchset but my machine works well. I wonder whether
> I should not remove all the casts or ignore the warnings?

Please do not ignore the warnings, they are not specific to the
MPTCP CI, they are also visible on the Netdev CI, and avoidable:

https://patchwork.kernel.org/project/netdevbpf/patch/20240416114003.62110-5-kerneljasonxing@gmail.com/

Cheers,
Matt

Re: [PATCH net-next v4 4/6] tcp: support rstreason for passive reset

[Jason Xing]

Hello Matthieu,

On Wed, Apr 17, 2024 at 4:23 AM Matthieu Baerts <matttbe@kernel.org> wrote:
>
> Hi Jason,
>
> 16 Apr 2024 14:25:13 Jason Xing <kerneljasonxing@gmail.com>:
>
> > On Tue, Apr 16, 2024 at 3:45 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >>
> >> On Tue, Apr 16, 2024 at 2:34 PM Eric Dumazet <edumazet@google.com> wrote:
> >>>
> >>> On Thu, Apr 11, 2024 at 1:57 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >>>>
> >>>> From: Jason Xing <kernelxing@tencent.com>
> >>>>
> >>>> Reuse the dropreason logic to show the exact reason of tcp reset,
> >>>> so we don't need to implement those duplicated reset reasons.
> >>>> This patch replaces all the prior NOT_SPECIFIED reasons.
> >>>>
> >>>> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> >>>> ---
> >>>> net/ipv4/tcp_ipv4.c | 8 ++++----
> >>>> net/ipv6/tcp_ipv6.c | 8 ++++----
> >>>> 2 files changed, 8 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> >>>> index 441134aebc51..863397c2a47b 100644
> >>>> --- a/net/ipv4/tcp_ipv4.c
> >>>> +++ b/net/ipv4/tcp_ipv4.c
> >>>> @@ -1935,7 +1935,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
> >>>>         return 0;
> >>>>
> >>>> reset:
> >>>> -       tcp_v4_send_reset(rsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> >>>> +       tcp_v4_send_reset(rsk, skb, (u32)reason);
> >>>> discard:
> >>>>         kfree_skb_reason(skb, reason);
> >>>>         /* Be careful here. If this function gets more complicated and
> >>>> @@ -2278,7 +2278,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> >>>>                 } else {
> >>>>                         drop_reason = tcp_child_process(sk, nsk, skb);
> >>>>                         if (drop_reason) {
> >>>> -                               tcp_v4_send_reset(nsk, skb, SK_RST_REASON_NOT_SPECIFIED);
> >>>> +                               tcp_v4_send_reset(nsk, skb, (u32)drop_reason);
> >>>
> >>> Are all these casts really needed ?
> >>
> >> Not really. If without, the compiler wouldn't complain about it.
> >
> > The truth is mptcp CI treats it as an error (see link[1]) when I
> > submitted the V5 patchset but my machine works well. I wonder whether
> > I should not remove all the casts or ignore the warnings?
>
> Please do not ignore the warnings, they are not specific to the
> MPTCP CI, they are also visible on the Netdev CI, and avoidable:
>
> https://patchwork.kernel.org/project/netdevbpf/patch/20240416114003.62110-5-kerneljasonxing@gmail.com/

Thanks for the information. Will add back the casts in V6 soon :)

Thanks,
Jason