Re: [PATCH] net/sched: act_mirred: Fix blockcast recursion bypass leading to stack overflow

[Jamal Hadi Salim]

On Wed, Apr 15, 2026 at 10:19 AM Simon Horman <horms@xxxxxxxxxx> wrote:
>
> From: 'Simon Horman' <horms@xxxxxxxxxx>
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> net/sched: act_mirred: Fix blockcast recursion bypass leading to stack overflow
>
> This patch fixes a stack overflow bug where mirred blockcast actions would
> bypass the recursion limit check because the recursion counter was not
> incremented for blockcast actions.
>
> > diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> > index 05e0b14b57731..5928fcf3e6514 100644
> > --- a/net/sched/act_mirred.c
> > +++ b/net/sched/act_mirred.c
> > @@ -444,8 +444,12 @@ TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
> >       tcf_action_update_bstats(&m->common, skb);
> >
> >       blockid = READ_ONCE(m->tcfm_blockid);
> > -     if (blockid)
> > -             return tcf_blockcast(skb, m, blockid, res, retval);
> > +     if (blockid) {
> > +             xmit->sched_mirred_nest++;
>
> Could this leave a stale pointer in sched_mirred_dev since netdev_xmit is
> persistent per-CPU (or per-task in PREEMPT_RT) memory?
>
> If a nested mirred action later iterates over sched_mirred_dev up to the new
> nesting depth to detect routing loops, could it match a stale pointer from
> whatever unrelated packet previously reached this nesting level?
>
> This might cause a false-positive loop detection and incorrectly drop a valid
> packet. Should we set xmit->sched_mirred_dev[xmit->sched_mirred_nest] = NULL
> before incrementing the counter?
>
> > +             retval = tcf_blockcast(skb, m, blockid, res, retval);
>
> This isn't a regression introduced by this patch, but it exists in the early
> error returns of tcf_mirred_act() (e.g., when the target device is gone, or a
> routing loop is detected) and in tcf_blockcast() (e.g., when
> xa_empty(&block->ports) is true).
>
> In these error paths, the functions simply return retval. For mirred redirect
> actions, retval is initialized to m->tcf_action, which is typically
> TC_ACT_STOLEN.
>
> When TC_ACT_STOLEN is returned, the TC framework assumes the action has taken
> ownership of the packet and will not free the sk_buff. Since the code does
> not free the sk_buff before returning in these error paths, does this leak
> the sk_buff?
>
> Could an unprivileged user inside a network namespace trigger this
> intentionally by creating a routing loop with mirred redirect rules, leading
> to continuous sk_buff leaks and system memory exhaustion?
>
> Should these error paths ensure the sk_buff is freed or force the return value
> to TC_ACT_SHOT if the action is a redirect, mirroring the existing behavior in
> tcf_mirred_to_dev()?
>
> > +             xmit->sched_mirred_nest--;
> > +             return retval;
> > +     }

There's a dependency with the skb bits patch[1] with this one.
Stephen, I will take this set and consider what Sashiko is saying here
[2]. I just became aware of this - but it seems to have some merit.
I will also review this patch and sashiko's input from that
perspective. If it merits it, I will resend the set incorporating this
fix and Sashiko's suggestions.

cheers,
jamal

[1]https://lore.kernel.org/netdev/20260326181701.308275-1-stephen@xxxxxxxxxxxxxxxxxx/
[2] https://sashiko.dev/#/patchset/20260326181701.308275-1-stephen%40networkplumber.org