On Wed, Oct 25, 2023 at 9:46 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 10/25/23 3:21 PM, Jamal Hadi Salim wrote:
> > On Wed, Oct 25, 2023 at 7:52 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> >> On 10/25/23 1:05 PM, Jamal Hadi Salim wrote:
> >>> On Wed, Oct 25, 2023 at 6:01 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> >>>> On 10/25/23 10:59 AM, Ido Schimmel wrote:
> >>>>> On Mon, Oct 09, 2023 at 11:26:54AM +0200, Daniel Borkmann wrote:
> >>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
> >>>>>> index 606a366cc209..664426285fa3 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 skb_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;
> >>>>>
> >>>>> Daniel,
> >>>>>
> >>>>> Getting the following splat [1] with CONFIG_DEBUG_NET=y and this
> >>>>> reproducer [2]. Problem seems to be that classifiers clear 'struct
> >>>>> tcf_result::drop_reason', thereby triggering the warning in
> >>>>> __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0).
> >>>>>
> >>>>> Fixed by maintaining the original drop reason if the one returned from
> >>>>> tcf_classify() is 'SKB_NOT_DROPPED_YET' [3]. I can submit this fix
> >>>>> unless you have a better idea.
> >>>>
> >>>> Thanks for catching this, looks reasonable to me as a fix.
> >>>>
> >>>>> [1]
> >>>>> WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130
> >>>>> Modules linked in:
> >>>>> CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682
> >>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014
> >>>>> RIP: 0010:kfree_skb_reason+0x38/0x130
> >>>>> [...]
> >>>>> Call Trace:
> >>>>> <IRQ>
> >>>>> __netif_receive_skb_core.constprop.0+0x837/0xdb0
> >>>>> __netif_receive_skb_one_core+0x3c/0x70
> >>>>> process_backlog+0x95/0x130
> >>>>> __napi_poll+0x25/0x1b0
> >>>>> net_rx_action+0x29b/0x310
> >>>>> __do_softirq+0xc0/0x29b
> >>>>> do_softirq+0x43/0x60
> >>>>> </IRQ>
> >>>>>
> >>>>> [2]
> >>>>> #!/bin/bash
> >>>>>
> >>>>> ip link add name veth0 type veth peer name veth1
> >>>>> ip link set dev veth0 up
> >>>>> ip link set dev veth1 up
> >>>>> tc qdisc add dev veth1 clsact
> >>>>> tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop
> >>>>> mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1
> >>>>
> >>>> I didn't know you're using mausezahn, nice :)
> >>>>
> >>>>> [3]
> >>>>> diff --git a/net/core/dev.c b/net/core/dev.c
> >>>>> index a37a932a3e14..abd0b13f3f17 100644
> >>>>> --- a/net/core/dev.c
> >>>>> +++ b/net/core/dev.c
> >>>>> @@ -3929,7 +3929,8 @@ static int tc_run(struct tcx_entry *entry, struct sk_buff *skb,
> >>>>> /* Only tcf related quirks below. */
> >>>>> switch (ret) {
> >>>>> case TC_ACT_SHOT:
> >>>>> - *drop_reason = res.drop_reason;
> >>>>> + if (res.drop_reason != SKB_NOT_DROPPED_YET)
> >>>>> + *drop_reason = res.drop_reason;
> >>>>> mini_qdisc_qstats_cpu_drop(miniq);
> >>>>> break;
> >>>>> case TC_ACT_OK:
> >>>>>
> >>>
> >>> Out of curiosity - how does the policy say "drop" but drop_reason does
> >>> not reflect it?
> >>
> >> Ido, Jamal, wdyt about this alternative approach - these were the locations I could
> >> find from an initial glance (compile-tested) :
> >>
> >> From a3d46a55aac484372b60b783cb6a3c98a0fef75c Mon Sep 17 00:00:00 2001
> >> From: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> >> Date: Wed, 25 Oct 2023 11:43:44 +0000
> >> Subject: [PATCH] net, sched: fix..
> >>
> >> Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> >> ---
> >> include/net/pkt_cls.h | 12 ++++++++++++
> >> net/sched/cls_basic.c | 2 +-
> >> net/sched/cls_bpf.c | 2 +-
> >> net/sched/cls_flower.c | 2 +-
> >> net/sched/cls_fw.c | 2 +-
> >> net/sched/cls_matchall.c | 2 +-
> >> net/sched/cls_route.c | 4 ++--
> >> net/sched/cls_u32.c | 2 +-
> >> 8 files changed, 20 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> >> index a76c9171db0e..31d8e8587824 100644
> >> --- a/include/net/pkt_cls.h
> >> +++ b/include/net/pkt_cls.h
> >> @@ -160,6 +160,18 @@ static inline void tcf_set_drop_reason(struct tcf_result *res,
> >> res->drop_reason = reason;
> >> }
> >>
> >> +static inline void tcf_set_result(struct tcf_result *to,
> >> + const struct tcf_result *from)
> >> +{
> >> + /* tcf_result's drop_reason which is the last member must be
> >> + * preserved and cannot be copied from the cls'es tcf_result
> >> + * template given this is carried all the way and potentially
> >> + * set to a concrete tc drop reason upon error or intentional
> >> + * drop. See tcf_set_drop_reason() locations.
> >> + */
> >> + memcpy(to, from, offsetof(typeof(*to), drop_reason));
> >> +}
> >> +
> >
> > Daniel, IMO, doing this at cls_api is best instead (like what Victors
> > or my original patch did). Iam ~30K feet right now with a lousy
> > keyboard - you can either do it, or i or Victor can send the patch by
> > end of day today. There are missing cases which were covered by Victor
> > and possibly something else will pop up next.
>
> Sure, if you have sth clean and simple for today, go for it. Otherwise I
> can cook a proper one out of this as a fix and ship it tomorrow AM, so we
> have a fix for the splat in CONFIG_DEBUG_NET kernels and you can still
> refactor later.
>
I was thinking something along these lines:
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1662,6 +1662,7 @@ static inline int __tcf_classify(struct sk_buff *skb,
const int max_reclassify_loop = 16;
const struct tcf_proto *first_tp;
int limit = 0;
+ u32 reason_code = res->drop_reason;
reclassify:
#endif
@@ -1712,8 +1713,11 @@ static inline int __tcf_classify(struct sk_buff *skb,
goto reset;
}
#endif
- if (err >= 0)
+ if (err >= 0) {
+ if (err == TC_ACT_SHOT) /* policy drop */
+ tcf_set_drop_reason(res, orig_reason);
return err;
+ }
}
if (unlikely(n)) {
But tbh, i am struggling with the whole approach you took in the
earlier patch - i.e setting the drop reason from cls_act and then
expecting it not to be changed on policy drops; while it works for
clsact, it is not going to work for the rest of the qdiscs - unless we
change all the qdisc enqueues to take an extra param for drop_reason.
Thoughts?
cheers,
jamal
> Thanks,
> Daniel
