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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Removal of TC_ACT_REDIRECT support from qevents (tcf_qevent_handle) breaks existing eBPF redirect functionality.
- [Low] pr_warn_once() used inside a static inline function in a header file will print multiple times.
--

commit d96e142c03f9c1a1e9ac2e1d8c8fe26605053b10
Author: Jamal Hadi Salim <jhs@mojatatu.com>

net/sched: Handle TC_ACT_REDIRECT from qdisc filter chains

When a TC filter attached to a qdisc filter chain returns TC_ACT_REDIRECT,
the redirect is silently lost. This patch adds a tcf_classify_qdisc()
wrapper to convert TC_ACT_REDIRECT to TC_ACT_SHOT and drops the packet.
It also removes the TC_ACT_REDIRECT case from tcf_qevent_handle() since
it is handled by the wrapper.

> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 3bd08d7f39c18..3a542a72e9a55 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
[ ... ]
> @@ -159,6 +159,19 @@ static inline int tcf_classify(struct sk_buff *skb,
>  
>  #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);
> +
> +	if (unlikely(ret == TC_ACT_REDIRECT)) {
> +		pr_warn_once("TC_ACT_REDIRECT from qdisc filter chains is not supported; use eBPF with clsact or mirred redirect instead\n");

[Severity: Low]
Is this going to print multiple times instead of strictly once globally?

Because tcf_classify_qdisc() is a static inline function in a header file,
each C file including it (like the various qdisc modules) will create its own
local instance of the static __already_done variable used by pr_warn_once().

> +		ret = TC_ACT_SHOT;
> +	}
> +	return ret;
> +}
> +
>  static inline unsigned long
>  __cls_set_class(unsigned long *clp, unsigned long cl)
>  {
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 3e67600a4a1a1..3ca56d060e28f 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -4033,7 +4033,7 @@ struct sk_buff *tcf_qevent_handle(struct tcf_qevent *qe, struct Qdisc *sch, stru
>  
>  	fl = rcu_dereference_bh(qe->filter_chain);
>  
> -	switch (tcf_classify(skb, NULL, fl, &cl_res, false)) {
> +	switch (tcf_classify_qdisc(skb, fl, &cl_res, false)) {

[Severity: High]
Does this break existing eBPF redirect functionality for qevents?

Prior to this patch, tcf_qevent_handle() correctly handled TC_ACT_REDIRECT
by calling skb_do_redirect(skb). By using the tcf_classify_qdisc() wrapper
here, TC_ACT_REDIRECT is unconditionally converted to TC_ACT_SHOT. This
inadvertently drops packets that were legitimately redirecting from qevents.

>  	case TC_ACT_SHOT:
>  		qdisc_qstats_drop(sch);
>  		__qdisc_drop(skb, to_free);
> @@ -4045,10 +4045,6 @@ struct sk_buff *tcf_qevent_handle(struct tcf_qevent *qe, struct Qdisc *sch, stru
>  		__qdisc_drop(skb, to_free);
>  		*ret = __NET_XMIT_STOLEN;
>  		return NULL;
> -	case TC_ACT_REDIRECT:
> -		skb_do_redirect(skb);
> -		*ret = __NET_XMIT_STOLEN;
> -		return NULL;
>  	case TC_ACT_CONSUMED:
>  		*ret = __NET_XMIT_STOLEN;
>  		return NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626165156.169012-1-jhs@mojatatu.com?part=2