[PATCH net-next v2 0/2] net: tcp: add skb drop reasons to connect request

[PATCH net-next v2 0/2] net: tcp: add skb drop reasons to connect request

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Seems now the reasons of skb drop in TCP layer are almost supported,
except the path of connect requesting. So let's just finish it.

The TCP connect requesting is processed by
'inet_csk(sk)->icsk_af_ops->conn_request()'. Yeah, it's a function
pointer, so it's not easy to add function param to it. Luckily, it's
return value can be reused. For now, 0 means a call of 'consume_skb()'
and -1 means 'kfree_skb()', with a RESET be send. Therefore, we can
return the drop reasons directly, as the positive is not used yet. With
drop reasons returned, kfree_skb_reason() will be called instead of
consume_skb() in tcp_rcv_state_process().

And in the 2th patch, skb drop reasons are added to route_req() in
struct tcp_request_sock_ops by adding a function param to it.

Following new skb drop reasons are added:

  SKB_DROP_REASON_LISTENOVERFLOWS
  SKB_DROP_REASON_TCP_REQQFULLDROP
  SKB_DROP_REASON_SECURITY

Changes since v1:
- don't free skb in conn_request, as Eric suggested, and use it's
  return value to pass drop reasons in the 1th patch.

Menglong Dong (2):
  net: tcp: add skb drop reasons to tcp connect request
  net: tcp: add skb drop reasons to route_req()

 include/linux/skbuff.h     |  5 +++++
 include/net/tcp.h          |  3 ++-
 include/trace/events/skb.h |  3 +++
 net/ipv4/tcp_input.c       | 22 +++++++++++++++-------
 net/ipv4/tcp_ipv4.c        | 16 ++++++++++++----
 net/ipv6/tcp_ipv6.c        | 18 +++++++++++++-----
 net/mptcp/subflow.c        | 10 ++++++----
 7 files changed, 56 insertions(+), 21 deletions(-)

-- 
2.36.0

[PATCH net-next v2 1/2] net: tcp: add skb drop reasons to tcp connect request

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

For now, the return value of tcp_v4_conn_request() has the following
means:

  >=0: the skb is acceptable, free it with consume_skb()
  <0 : the skb is unacceptable, free it with kfree_skb() and send a
       RESET

In order to get the drop reasons from tcp_v4_conn_request(), we make
some changes to its return value:

  ==0: the skb is acceptable, free it with consume_skb()
  >0:  the return value is exactly the skb drop reasons, free it with
       kfree_skb_reason() without sendind RESET
  <0:  the same as what we do before

( As a negative value can be returned by
struct inet_connection_sock_af_ops.conn_request(), so we can't make
the return value of tcp_v4_conn_request() as num skb_drop_reason
directly. )

Therefore, previous logic is not changed, as tcp_v4_conn_request()
never return a positive before.

With drop reasons returned, the caller of tcp_v4_conn_request(), which
is tcp_rcv_state_process(), will call kfree_skb_reason() instead of
consume_skb().

Following new drop reasons are added:

  SKB_DROP_REASON_LISTENOVERFLOWS
  SKB_DROP_REASON_TCP_REQQFULLDROP

Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v2:
- don't free skb in conn_request, as Eric suggested, and use it's
  return value to pass drop reasons.
---
 include/linux/skbuff.h     |  4 ++++
 include/trace/events/skb.h |  2 ++
 net/ipv4/tcp_input.c       | 20 ++++++++++++++------
 net/ipv4/tcp_ipv4.c        |  2 +-
 net/ipv6/tcp_ipv6.c        |  4 ++--
 5 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 84d78df60453..f33b3636bbce 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -469,6 +469,10 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_PKT_TOO_BIG,	/* packet size is too big (maybe exceed
 					 * the MTU)
 					 */
+	SKB_DROP_REASON_LISTENOVERFLOWS, /* accept queue of the listen socket is full */
+	SKB_DROP_REASON_TCP_REQQFULLDROP, /* request queue of the listen
+					   * socket is full
+					   */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index a477bf907498..de6c93670437 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -80,6 +80,8 @@
 	EM(SKB_DROP_REASON_IP_INADDRERRORS, IP_INADDRERRORS)	\
 	EM(SKB_DROP_REASON_IP_INNOROUTES, IP_INNOROUTES)	\
 	EM(SKB_DROP_REASON_PKT_TOO_BIG, PKT_TOO_BIG)		\
+	EM(SKB_DROP_REASON_LISTENOVERFLOWS, LISTENOVERFLOWS)	\
+	EM(SKB_DROP_REASON_TCP_REQQFULLDROP, TCP_REQQFULLDROP)	\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index daff631b9486..412367b7dfd6 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6411,7 +6411,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	const struct tcphdr *th = tcp_hdr(skb);
 	struct request_sock *req;
-	int queued = 0;
+	int err, queued = 0;
 	bool acceptable;
 	SKB_DR(reason);
 
@@ -6438,13 +6438,16 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 			 */
 			rcu_read_lock();
 			local_bh_disable();
-			acceptable = icsk->icsk_af_ops->conn_request(sk, skb) >= 0;
+			err = icsk->icsk_af_ops->conn_request(sk, skb);
 			local_bh_enable();
 			rcu_read_unlock();
 
-			if (!acceptable)
+			if (err < 0)
 				return 1;
-			consume_skb(skb);
+			if (err)
+				kfree_skb_reason(skb, err);
+			else
+				consume_skb(skb);
 			return 0;
 		}
 		SKB_DR_SET(reason, TCP_FLAGS);
@@ -6878,6 +6881,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	bool want_cookie = false;
 	struct dst_entry *dst;
 	struct flowi fl;
+	SKB_DR(reason);
 
 	/* TW buckets are converted to open requests without
 	 * limitations, they conserve resources and peer is
@@ -6886,12 +6890,15 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	if ((net->ipv4.sysctl_tcp_syncookies == 2 ||
 	     inet_csk_reqsk_queue_is_full(sk)) && !isn) {
 		want_cookie = tcp_syn_flood_action(sk, rsk_ops->slab_name);
-		if (!want_cookie)
+		if (!want_cookie) {
+			SKB_DR_SET(reason, TCP_REQQFULLDROP);
 			goto drop;
+		}
 	}
 
 	if (sk_acceptq_is_full(sk)) {
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
+		SKB_DR_SET(reason, LISTENOVERFLOWS);
 		goto drop;
 	}
 
@@ -6947,6 +6954,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 			 */
 			pr_drop_req(req, ntohs(tcp_hdr(skb)->source),
 				    rsk_ops->family);
+			SKB_DR_SET(reason, TCP_REQQFULLDROP);
 			goto drop_and_release;
 		}
 
@@ -7007,6 +7015,6 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	__reqsk_free(req);
 drop:
 	tcp_listendrop(sk);
-	return 0;
+	return reason;
 }
 EXPORT_SYMBOL(tcp_conn_request);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 157265aecbed..6a49470d30db 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1470,7 +1470,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 
 drop:
 	tcp_listendrop(sk);
-	return 0;
+	return SKB_DROP_REASON_IP_INADDRERRORS;
 }
 EXPORT_SYMBOL(tcp_v4_conn_request);
 
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 782df529ff69..92f4a58fdc2c 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1158,7 +1158,7 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 
 	if (ipv6_addr_v4mapped(&ipv6_hdr(skb)->saddr)) {
 		__IP6_INC_STATS(sock_net(sk), NULL, IPSTATS_MIB_INHDRERRORS);
-		return 0;
+		return SKB_DROP_REASON_IP_INADDRERRORS;
 	}
 
 	return tcp_conn_request(&tcp6_request_sock_ops,
@@ -1166,7 +1166,7 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 
 drop:
 	tcp_listendrop(sk);
-	return 0; /* don't send reset */
+	return SKB_DROP_REASON_IP_INADDRERRORS; /* don't send reset */
 }
 
 static void tcp_v6_restore_cb(struct sk_buff *skb)
-- 
2.36.0

[PATCH net-next v2 2/2] net: tcp: add skb drop reasons to route_req()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Add skb drop reasons to the route_req() in struct tcp_request_sock_ops.
Following functions are involved:

  tcp_v4_route_req()
  tcp_v6_route_req()
  subflow_v4_route_req()
  subflow_v6_route_req()

And the new reason SKB_DROP_REASON_SECURITY is added, which is used when
skb is dropped by LSM.

Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     |  1 +
 include/net/tcp.h          |  3 ++-
 include/trace/events/skb.h |  1 +
 net/ipv4/tcp_input.c       |  2 +-
 net/ipv4/tcp_ipv4.c        | 14 +++++++++++---
 net/ipv6/tcp_ipv6.c        | 14 +++++++++++---
 net/mptcp/subflow.c        | 10 ++++++----
 7 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f33b3636bbce..5909759e1b95 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -473,6 +473,7 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_TCP_REQQFULLDROP, /* request queue of the listen
 					   * socket is full
 					   */
+	SKB_DROP_REASON_SECURITY,	/* dropped by LSM */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 679b1964d494..01f841611895 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2075,7 +2075,8 @@ struct tcp_request_sock_ops {
 	struct dst_entry *(*route_req)(const struct sock *sk,
 				       struct sk_buff *skb,
 				       struct flowi *fl,
-				       struct request_sock *req);
+				       struct request_sock *req,
+				       enum skb_drop_reason *reason);
 	u32 (*init_seq)(const struct sk_buff *skb);
 	u32 (*init_ts_off)(const struct net *net, const struct sk_buff *skb);
 	int (*send_synack)(const struct sock *sk, struct dst_entry *dst,
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index de6c93670437..aff57cd43e85 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -82,6 +82,7 @@
 	EM(SKB_DROP_REASON_PKT_TOO_BIG, PKT_TOO_BIG)		\
 	EM(SKB_DROP_REASON_LISTENOVERFLOWS, LISTENOVERFLOWS)	\
 	EM(SKB_DROP_REASON_TCP_REQQFULLDROP, TCP_REQQFULLDROP)	\
+	EM(SKB_DROP_REASON_SECURITY, SECURITY)			\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 412367b7dfd6..e55340059c19 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6932,7 +6932,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	/* Note: tcp_v6_init_req() might override ir_iif for link locals */
 	inet_rsk(req)->ir_iif = inet_request_bound_dev_if(sk, skb);
 
-	dst = af_ops->route_req(sk, skb, &fl, req);
+	dst = af_ops->route_req(sk, skb, &fl, req, &reason);
 	if (!dst)
 		goto drop_and_free;
 
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 6a49470d30db..0ae55c9e8428 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1424,14 +1424,22 @@ static void tcp_v4_init_req(struct request_sock *req,
 static struct dst_entry *tcp_v4_route_req(const struct sock *sk,
 					  struct sk_buff *skb,
 					  struct flowi *fl,
-					  struct request_sock *req)
+					  struct request_sock *req,
+					  enum skb_drop_reason *reason)
 {
+	struct dst_entry *dst;
+
 	tcp_v4_init_req(req, sk, skb);
 
-	if (security_inet_conn_request(sk, skb, req))
+	if (security_inet_conn_request(sk, skb, req)) {
+		SKB_DR_SET(*reason, SECURITY);
 		return NULL;
+	}
 
-	return inet_csk_route_req(sk, &fl->u.ip4, req);
+	dst = inet_csk_route_req(sk, &fl->u.ip4, req);
+	if (!dst)
+		SKB_DR_SET(*reason, IP_OUTNOROUTES);
+	return dst;
 }
 
 struct request_sock_ops tcp_request_sock_ops __read_mostly = {
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 92f4a58fdc2c..d1f9266a81ed 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -802,14 +802,22 @@ static void tcp_v6_init_req(struct request_sock *req,
 static struct dst_entry *tcp_v6_route_req(const struct sock *sk,
 					  struct sk_buff *skb,
 					  struct flowi *fl,
-					  struct request_sock *req)
+					  struct request_sock *req,
+					  enum skb_drop_reason *reason)
 {
+	struct dst_entry *dst;
+
 	tcp_v6_init_req(req, sk, skb);
 
-	if (security_inet_conn_request(sk, skb, req))
+	if (security_inet_conn_request(sk, skb, req)) {
+		SKB_DR_SET(*reason, SECURITY);
 		return NULL;
+	}
 
-	return inet6_csk_route_req(sk, &fl->u.ip6, req, IPPROTO_TCP);
+	dst = inet6_csk_route_req(sk, &fl->u.ip6, req, IPPROTO_TCP);
+	if (!dst)
+		SKB_DR_SET(*reason, IP_OUTNOROUTES);
+	return dst;
 }
 
 struct request_sock_ops tcp6_request_sock_ops __read_mostly = {
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index aba260f547da..03d07165cda6 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -283,7 +283,8 @@ EXPORT_SYMBOL_GPL(mptcp_subflow_init_cookie_req);
 static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 					      struct sk_buff *skb,
 					      struct flowi *fl,
-					      struct request_sock *req)
+					      struct request_sock *req,
+					      enum skb_drop_reason *reason)
 {
 	struct dst_entry *dst;
 	int err;
@@ -291,7 +292,7 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 	tcp_rsk(req)->is_mptcp = 1;
 	subflow_init_req(req, sk);
 
-	dst = tcp_request_sock_ipv4_ops.route_req(sk, skb, fl, req);
+	dst = tcp_request_sock_ipv4_ops.route_req(sk, skb, fl, req, reason);
 	if (!dst)
 		return NULL;
 
@@ -309,7 +310,8 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 					      struct sk_buff *skb,
 					      struct flowi *fl,
-					      struct request_sock *req)
+					      struct request_sock *req,
+					      enum skb_drop_reason *reason)
 {
 	struct dst_entry *dst;
 	int err;
@@ -317,7 +319,7 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 	tcp_rsk(req)->is_mptcp = 1;
 	subflow_init_req(req, sk);
 
-	dst = tcp_request_sock_ipv6_ops.route_req(sk, skb, fl, req);
+	dst = tcp_request_sock_ipv6_ops.route_req(sk, skb, fl, req, reason);
 	if (!dst)
 		return NULL;
 
-- 
2.36.0

Re: [PATCH net-next v2 2/2] net: tcp: add skb drop reasons to route_req()

[Toke Høiland-Jørgensen]

menglong8.dong@gmail.com writes:

> From: Menglong Dong <imagedong@tencent.com>
>
> Add skb drop reasons to the route_req() in struct tcp_request_sock_ops.
> Following functions are involved:
>
>   tcp_v4_route_req()
>   tcp_v6_route_req()
>   subflow_v4_route_req()
>   subflow_v6_route_req()
>
> And the new reason SKB_DROP_REASON_SECURITY is added, which is used when
> skb is dropped by LSM.

Could we maybe pick a slightly less generic name? If I saw
"SKB_DROP_REASON_SECURITY" my first thought would be something related
to *network* security, like a firewall. Maybe just SKB_DROP_REASON_LSM?

-Toke

Re: [PATCH net-next v2 2/2] net: tcp: add skb drop reasons to route_req()

[Menglong Dong]

On Thu, Apr 28, 2022 at 9:22 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> menglong8.dong@gmail.com writes:
>
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Add skb drop reasons to the route_req() in struct tcp_request_sock_ops.
> > Following functions are involved:
> >
> >   tcp_v4_route_req()
> >   tcp_v6_route_req()
> >   subflow_v4_route_req()
> >   subflow_v6_route_req()
> >
> > And the new reason SKB_DROP_REASON_SECURITY is added, which is used when
> > skb is dropped by LSM.
>
> Could we maybe pick a slightly less generic name? If I saw
> "SKB_DROP_REASON_SECURITY" my first thought would be something related
> to *network* security, like a firewall. Maybe just SKB_DROP_REASON_LSM?
>

Thanks for your suggestion, and I think SKB_DROP_REASON_LSM is fine.
I'll change it in the next version.

Menglong Dong

> -Toke
>