Re: [PATCH net 2/3] net/sched: Handle TC_ACT_REDIRECT from qdisc filter chains

[Paolo Abeni <pabeni@redhat.com>]

Hi,

On 7/1/26 5:35 PM, Jamal Hadi Salim wrote:
> On Tue, Jun 30, 2026 at 11:23 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 6/30/26 5:16 PM, Jamal Hadi Salim wrote:
>>> On Tue, Jun 30, 2026 at 8:33 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>>>
>>>> When a TC filter attached to a qdisc filter chain returns
>>>> TC_ACT_REDIRECT (ex: via an eBPF program calling bpf_redirect() or an
>>>> act_bpf action), the redirect was silently lost i.e no qdisc classify
>>>> function handled TC_ACT_REDIRECT, so the packet fell through the
>>>> switch and was enqueued normally instead of being redirected.
>>>>
>>>> This has been broken since bpf_redirect() was introduced for TC in
>>>> commit 27b29f63058d ("bpf: add bpf_redirect() helper"). We got lucky
>>>> for a long time because bpf_net_context was a per-CPU variable that
>>>> was always available.
>>>>
>>>> commit 401cb7dae813 ("net: Reference bpf_redirect_info via task_struct
>>>> on PREEMPT_RT.") turned bpf_net_context into a task_struct member that
>>>> is only set up by explicit callers. Without a caller setting it up,
>>>> bpf_redirect() itself crashes with a NULL pointer dereference in
>>>> bpf_net_ctx_get_ri(). However, even with bpf_net_context available,
>>>> TC_ACT_REDIRECT from qdisc filter chains cannot be honored without
>>>> adding skb_do_redirect() calls to every qdisc classify function, which
>>>> would require changes across net/sched/. Isolate it to ebpf core where
>>>> it belongs.
>>>>
>>>> Instead, add a tcf_classify_qdisc() inline helper in pkt_cls.h, as a
>>>> wrapper around tcf_classify() for use by qdisc classify functions and
>>>> tcf_qevent_handle(). When the classify verdict is TC_ACT_REDIRECT,
>>>> the wrapper converts it to TC_ACT_SHOT, dropping the packet rather
>>>> than letting it continue silently. Dropping is preferred over
>>>> letting the packet through because the user immediately sees packet
>>>> loss. Silently passing the packet through would hide the problem and
>>>> leave the user wondering why their redirect is not working.
>>>>
>>>> The clsact fast path, tc_run() continues to call tcf_classify() directly
>>>> and is unaffected: TC_ACT_REDIRECT is returned as-is and handled by
>>>> sch_handle_egress/ingress() calling skb_do_redirect() as before.
>>>>
>>>> Fixes: 27b29f63058d ("bpf: add bpf_redirect() helper")
>>>> Fixes: 401cb7dae813 ("net: Reference bpf_redirect_info via task_struct on PREEMPT_RT.")
>>>> Tested-by: Victor Nogueira <victor@mojatatu.com>
>>>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>>> ---
>>>>   include/net/pkt_cls.h    | 14 +++++++++++++-
>>>>   net/sched/cls_api.c      |  4 +---
>>>>   net/sched/sch_cake.c     |  2 +-
>>>>   net/sched/sch_drr.c      |  2 +-
>>>>   net/sched/sch_dualpi2.c  |  2 +-
>>>>   net/sched/sch_ets.c      |  2 +-
>>>>   net/sched/sch_fq_codel.c |  2 +-
>>>>   net/sched/sch_fq_pie.c   |  2 +-
>>>>   net/sched/sch_hfsc.c     |  2 +-
>>>>   net/sched/sch_htb.c      |  2 +-
>>>>   net/sched/sch_multiq.c   |  2 +-
>>>>   net/sched/sch_prio.c     |  2 +-
>>>>   net/sched/sch_qfq.c      |  2 +-
>>>>   net/sched/sch_sfb.c      |  2 +-
>>>>   net/sched/sch_sfq.c      |  2 +-
>>>>   15 files changed, 27 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>>>> index 3bd08d7f39c1..5f5cb36439fe 100644
>>>> --- a/include/net/pkt_cls.h
>>>> +++ b/include/net/pkt_cls.h
>>>> @@ -156,8 +156,20 @@ static inline int tcf_classify(struct sk_buff *skb,
>>>>   {
>>>>          return TC_ACT_UNSPEC;
>>>>   }
>>>> -
>>>>   #endif
>>>> +static inline int tcf_classify_qdisc(struct sk_buff *skb,
>>>> +                                    const struct tcf_proto *tp,
>>>> +                                    struct tcf_result *res, bool compat_mode)
>>>> +{
>>>> +       int ret = tcf_classify(skb, NULL, tp, res, compat_mode);
>>>> +
>>>> +       /* TC_ACT_REDIRECT from qdisc filter chains is not supported.
>>>> +        * Use BPF via tcx or mirred redirect instead.
>>>> +        */
>>>> +       if (unlikely(ret == TC_ACT_REDIRECT))
>>>> +               ret = TC_ACT_SHOT;
>>>> +       return ret;
>>>> +}
>>>
>>> Why did you remove the warning?
>>> A lesser issue is that you introduced a space above #endif
>>
>> I don't think we need to put out an extra warn to spam the log, this
>> can be easily traced either via bpftrace or qdisc stats etc; plus the
>> guidance to eBPF with clsact is also obsolete. Given noone has run
>> into this the last 10y, I don't think it really matters tbh.
> 
> It's a bit entitled for you to change someone else's patch to fit your
> philosophy without a mention.
> It's only one message per qdisc. But fine, let's keep it that way.
> Sashiko is hallucinating on TC_ACT_CONSUMED for patch 1. Let's ignore it.

@Jamal: I read the above as if you are ok with the series in the current
form as a whole. Please LMK.

FTR, I have a slight preference towards the extra warn message in case
of misconfiguration, but big deal either way.

/P