From: Menglong Dong <[email protected]>
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
From: Menglong Dong <[email protected]>
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 <[email protected]>
Reviewed-by: Hao Peng <[email protected]>
Signed-off-by: Menglong Dong <[email protected]>
---
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
[email protected] writes:
> From: Menglong Dong <[email protected]>
>
> 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
On Thu, Apr 28, 2022 at 9:22 PM Toke Høiland-Jørgensen <[email protected]> wrote:
>
> [email protected] writes:
>
> > From: Menglong Dong <[email protected]>
> >
> > 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
>