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.
cheers,
jamal
> static inline void
> __tcf_bind_filter(struct Qdisc *q, struct tcf_result *r, unsigned long base)
> {
> diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
> index 1b92c33b5f81..d7ead3fc3c45 100644
> --- a/net/sched/cls_basic.c
> +++ b/net/sched/cls_basic.c
> @@ -50,7 +50,7 @@ TC_INDIRECT_SCOPE int basic_classify(struct sk_buff *skb,
> if (!tcf_em_tree_match(skb, &f->ematches, NULL))
> continue;
> __this_cpu_inc(f->pf->rhit);
> - *res = f->res;
> + tcf_set_result(res, &f->res);
> r = tcf_exts_exec(skb, &f->exts, res);
> if (r < 0)
> continue;
> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index 382c7a71f81f..e4620a462bc3 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -124,7 +124,7 @@ TC_INDIRECT_SCOPE int cls_bpf_classify(struct sk_buff *skb,
> res->class = 0;
> res->classid = filter_res;
> } else {
> - *res = prog->res;
> + tcf_set_result(res, &prog->res);
> }
>
> ret = tcf_exts_exec(skb, &prog->exts, res);
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index e5314a31f75a..eb94090fb26c 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -341,7 +341,7 @@ TC_INDIRECT_SCOPE int fl_classify(struct sk_buff *skb,
>
> f = fl_mask_lookup(mask, &skb_key);
> if (f && !tc_skip_sw(f->flags)) {
> - *res = f->res;
> + tcf_set_result(res, &f->res);
> return tcf_exts_exec(skb, &f->exts, res);
> }
> }
> diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
> index c49d6af0e048..70b873f8771f 100644
> --- a/net/sched/cls_fw.c
> +++ b/net/sched/cls_fw.c
> @@ -63,7 +63,7 @@ TC_INDIRECT_SCOPE int fw_classify(struct sk_buff *skb,
> for (f = rcu_dereference_bh(head->ht[fw_hash(id)]); f;
> f = rcu_dereference_bh(f->next)) {
> if (f->id == id) {
> - *res = f->res;
> + tcf_set_result(res, &f->res);
> if (!tcf_match_indev(skb, f->ifindex))
> continue;
> r = tcf_exts_exec(skb, &f->exts, res);
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index c4ed11df6254..a4018db80a60 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -37,7 +37,7 @@ TC_INDIRECT_SCOPE int mall_classify(struct sk_buff *skb,
> if (tc_skip_sw(head->flags))
> return -1;
>
> - *res = head->res;
> + tcf_set_result(res, &head->res);
> __this_cpu_inc(head->pf->rhit);
> return tcf_exts_exec(skb, &head->exts, res);
> }
> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 1424bfeaca73..cbfaa1d1820f 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
> @@ -109,7 +109,7 @@ static inline int route4_hash_wild(void)
>
> #define ROUTE4_APPLY_RESULT() \
> { \
> - *res = f->res; \
> + tcf_set_result(res, &f->res); \
> if (tcf_exts_has_actions(&f->exts)) { \
> int r = tcf_exts_exec(skb, &f->exts, res); \
> if (r < 0) { \
> @@ -152,7 +152,7 @@ TC_INDIRECT_SCOPE int route4_classify(struct sk_buff *skb,
> goto failure;
> }
>
> - *res = f->res;
> + tcf_set_result(res, &f->res);
> spin_unlock(&fastmap_lock);
> return 0;
> }
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 6663e971a13e..f50ae40a29d5 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -172,7 +172,7 @@ TC_INDIRECT_SCOPE int u32_classify(struct sk_buff *skb,
> check_terminal:
> if (n->sel.flags & TC_U32_TERMINAL) {
>
> - *res = n->res;
> + tcf_set_result(res, &n->res);
> if (!tcf_match_indev(skb, n->ifindex)) {
> n = rcu_dereference_bh(n->next);
> goto next_knode;
> --
> 2.34.1
>
