Hi Jamal,
On 10/3/23 11:36 PM, Jamal Hadi Salim wrote:
[...]
I did look, Daniel. You are lumping all the error codes into one -
which doesnt change my view on disambiguation. If i was to debug
closely and run kprobe now i am seeing only one error code
TC_ACT_ABORT instead of -EINVAL vs -ENOMEM, etc. Easier for me to find
the source manually (and possibly even better with Andrii's tool i saw
once if it would work in the datapath - iirc, i think it prints return
codes on the code paths).
That should be possible with some work this way, agree. I've been toying a bit
more on this issue, and actually there is an even better way which would cleanly
solve all use cases and we likely would utilize it for bpf as well in future.
I wasn't aware of it before, but the drop reason actually has per-subsystem infra
already which so far only mac80211 and ovs makes use of.
I wrote up below patch as a starting point to get the base infra up and with
TC_DROP_MAX_RECLASSIFY as the initial example on how to utilize it. Then you can
simply just use regular tooling and get more detailed kfree_skb_reason() codes,
which would also remove the need for kprobes/kretprobes to gather the error.
Let me know if this looks like a good path forward, then I'll cook a proper one
and you or Victor can extend it further with more drop reasons. The nice thing is
also that this can be extended successively with more reasons whenever needed.
Best & thanks,
Daniel
From d62b4a52f9c725d4a63d5c76a576d4e3bbbea4ef Mon Sep 17 00:00:00 2001
From: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
Date: Fri, 6 Oct 2023 08:42:19 +0000
Subject: [PATCH net-next] net, tc: Add extensible drop reason subsystem codes
[ commit msg tbd ]
Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
---
include/net/dropreason-core.h | 4 ++--
include/net/dropreason.h | 6 ++++++
include/net/sch_drop.h | 35 +++++++++++++++++++++++++++++++++++
include/net/sch_generic.h | 11 +++++++++--
net/core/dev.c | 15 ++++++++++-----
net/sched/cls_api.c | 1 +
6 files changed, 63 insertions(+), 9 deletions(-)
create mode 100644 include/net/sch_drop.h
diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index a587e83fc169..670eac9923aa 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -235,7 +235,7 @@ enum skb_drop_reason {
SKB_DROP_REASON_NEIGH_QUEUEFULL,
/** @SKB_DROP_REASON_NEIGH_DEAD: neigh entry is dead */
SKB_DROP_REASON_NEIGH_DEAD,
- /** @SKB_DROP_REASON_TC_EGRESS: dropped in TC egress HOOK */
+ /** @SKB_DROP_REASON_TC_EGRESS: Unused, see TC_DROP_EGRESS instead */
SKB_DROP_REASON_TC_EGRESS,
/**
* @SKB_DROP_REASON_QDISC_DROP: dropped by qdisc when packet outputting (
@@ -250,7 +250,7 @@ enum skb_drop_reason {
SKB_DROP_REASON_CPU_BACKLOG,
/** @SKB_DROP_REASON_XDP: dropped by XDP in input path */
SKB_DROP_REASON_XDP,
- /** @SKB_DROP_REASON_TC_INGRESS: dropped in TC ingress HOOK */
+ /** @SKB_DROP_REASON_TC_INGRESS: Unused, see TC_DROP_INGRESS instead */
SKB_DROP_REASON_TC_INGRESS,
/** @SKB_DROP_REASON_UNHANDLED_PROTO: protocol not implemented or not supported */
SKB_DROP_REASON_UNHANDLED_PROTO,
diff --git a/include/net/dropreason.h b/include/net/dropreason.h
index 56cb7be92244..434ed2124836 100644
--- a/include/net/dropreason.h
+++ b/include/net/dropreason.h
@@ -29,6 +29,12 @@ enum skb_drop_reason_subsys {
*/
SKB_DROP_REASON_SUBSYS_OPENVSWITCH,
+ /**
+ * @SKB_DROP_REASON_SUBSYS_TC: traffic control (tc) drop reasons,
+ * see include/net/sch_drop.h
+ */
+ SKB_DROP_REASON_SUBSYS_TC,
+
/** @SKB_DROP_REASON_SUBSYS_NUM: number of subsystems defined */
SKB_DROP_REASON_SUBSYS_NUM
};
diff --git a/include/net/sch_drop.h b/include/net/sch_drop.h
new file mode 100644
index 000000000000..c2471a62c10b
--- /dev/null
+++ b/include/net/sch_drop.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Traffic control drop reason list. */
+
+#ifndef NET_SCH_DROP_H
+#define NET_SCH_DROP_H
+
+#include <linux/skbuff.h>
+#include <net/dropreason.h>
+
+/**
+ * @TC_DROP_INGRESS: dropped in tc ingress hook (generic, catch-all code)
+ * @TC_DROP_EGRESS: dropped in tc egress hook (generic, catch-all code)
+ * @TC_DROP_MAX_RECLASSIFY: dropped due to hitting maximum reclassify limit
+ */
+#define TC_DROP_REASONS(R) \
+ R(TC_DROP_INGRESS) \
+ R(TC_DROP_EGRESS) \
+ R(TC_DROP_MAX_RECLASSIFY) \
+ /* deliberate comment for trailing \ */
+
+enum tc_drop_reason {
+ __TC_DROP_REASON = SKB_DROP_REASON_SUBSYS_TC <<
+ SKB_DROP_REASON_SUBSYS_SHIFT,
+#define ENUM(x) x,
+ TC_DROP_REASONS(ENUM)
+#undef ENUM
+ TC_DROP_MAX,
+};
+
+static inline void
+tc_kfree_skb_reason(struct sk_buff *skb, enum tc_drop_reason reason)
+{
+ kfree_skb_reason(skb, (u32)reason);
+}
+#endif /* NET_SCH_DROP_H */
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c7318c73cfd6..e50a281ff1af 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -16,9 +16,11 @@
#include <linux/rwsem.h>
#include <linux/atomic.h>
#include <linux/hashtable.h>
+
#include <net/gen_stats.h>
#include <net/rtnetlink.h>
#include <net/flow_offload.h>
+#include <net/sch_drop.h>
struct Qdisc_ops;
struct qdisc_walker;
@@ -324,15 +326,14 @@ struct Qdisc_ops {
struct module *owner;
};
-
struct tcf_result {
+ enum tc_drop_reason drop_reason;
union {
struct {
unsigned long class;
u32 classid;
};
const struct tcf_proto *goto_tp;
-
};
};
@@ -667,6 +668,12 @@ static inline int tc_classid_to_hwtc(struct net_device *dev, u32 classid)
return (hwtc < netdev_get_num_tc(dev)) ? hwtc : -EINVAL;
}
+static inline void tc_set_drop_reason(struct tcf_result *res,
+ enum tc_drop_reason reason)
+{
+ res->drop_reason = reason;
+}
+
int qdisc_class_hash_init(struct Qdisc_class_hash *);
void qdisc_class_hash_insert(struct Qdisc_class_hash *,
struct Qdisc_class_common *);
diff --git a/net/core/dev.c b/net/core/dev.c
index 606a366cc209..93cebe374082 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3910,7 +3910,8 @@ EXPORT_SYMBOL_GPL(netdev_xmit_skip_txqueue);
#endif /* CONFIG_NET_EGRESS */
#ifdef CONFIG_NET_XGRESS
-static int tc_run(struct tcx_entry *entry, struct sk_buff *skb)
+static int tc_run(struct tcx_entry *entry, struct sk_buff *skb,
+ enum tc_drop_reason *drop_reason)
{
int ret = TC_ACT_UNSPEC;
#ifdef CONFIG_NET_CLS_ACT
@@ -3922,12 +3923,14 @@ static int tc_run(struct tcx_entry *entry, struct sk_buff *skb)
tc_skb_cb(skb)->mru = 0;
tc_skb_cb(skb)->post_ct = false;
+ res.drop_reason = *drop_reason;
mini_qdisc_bstats_cpu_update(miniq, skb);
ret = tcf_classify(skb, miniq->block, miniq->filter_list, &res, false);
/* Only tcf related quirks below. */
switch (ret) {
case TC_ACT_SHOT:
+ *drop_reason = res.drop_reason;
mini_qdisc_qstats_cpu_drop(miniq);
break;
case TC_ACT_OK:
@@ -3977,6 +3980,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
struct net_device *orig_dev, bool *another)
{
struct bpf_mprog_entry *entry = rcu_dereference_bh(skb->dev->tcx_ingress);
+ enum tc_drop_reason drop_reason = TC_DROP_INGRESS;
int sch_ret;
if (!entry)
@@ -3994,7 +3998,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
if (sch_ret != TC_ACT_UNSPEC)
goto ingress_verdict;
}
- sch_ret = tc_run(tcx_entry(entry), skb);
+ sch_ret = tc_run(tcx_entry(entry), skb, &drop_reason);
ingress_verdict:
switch (sch_ret) {
case TC_ACT_REDIRECT:
@@ -4011,7 +4015,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
*ret = NET_RX_SUCCESS;
return NULL;
case TC_ACT_SHOT:
- kfree_skb_reason(skb, SKB_DROP_REASON_TC_INGRESS);
+ tc_kfree_skb_reason(skb, drop_reason);
*ret = NET_RX_DROP;
return NULL;
/* used by tc_run */
@@ -4032,6 +4036,7 @@ static __always_inline struct sk_buff *
sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
{
struct bpf_mprog_entry *entry = rcu_dereference_bh(dev->tcx_egress);
+ enum tc_drop_reason drop_reason = TC_DROP_EGRESS;
int sch_ret;
if (!entry)
@@ -4045,7 +4050,7 @@ sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
if (sch_ret != TC_ACT_UNSPEC)
goto egress_verdict;
}
- sch_ret = tc_run(tcx_entry(entry), skb);
+ sch_ret = tc_run(tcx_entry(entry), skb, &drop_reason);
egress_verdict:
switch (sch_ret) {
case TC_ACT_REDIRECT:
@@ -4054,7 +4059,7 @@ sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
*ret = NET_XMIT_SUCCESS;
return NULL;
case TC_ACT_SHOT:
- kfree_skb_reason(skb, SKB_DROP_REASON_TC_EGRESS);
+ tc_kfree_skb_reason(skb, drop_reason);
*ret = NET_XMIT_DROP;
return NULL;
/* used by tc_run */
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a193cc7b3241..5d56ddb1462f 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1723,6 +1723,7 @@ static inline int __tcf_classify(struct sk_buff *skb,
tp->chain->block->index,
tp->prio & 0xffff,
ntohs(tp->protocol));
+ tc_set_drop_reason(res, TC_DROP_MAX_RECLASSIFY);
return TC_ACT_SHOT;
}
--
2.34.1
