netdev - Re: [PATCH net-next 1/2] net/sched: act

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAM0EoMn4rW3VOmEaxz2VBxGeywoQ_TBDSz7qKsS6AHjo0HNJWA@mail.gmail.com>
Date: Thu, 27 Nov 2025 11:21:19 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net, pabeni@...hat.com, 
	jiri@...nulli.us, xiyou.wangcong@...il.com, netdev@...r.kernel.org, 
	dcaratti@...hat.com
Subject: Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

On Thu, Nov 27, 2025 at 10:23 AM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>
> On Thu, Nov 27, 2025 at 10:11 AM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Thu, Nov 27, 2025 at 6:45 AM Jamal Hadi Salim <jhs@...atatu.com> wrote:
> > >
> > > On Wed, Nov 26, 2025 at 3:30 PM Eric Dumazet <edumazet@...gle.com> wrote:
> > > >
> > > > On Wed, Nov 26, 2025 at 12:20 PM Jamal Hadi Salim <jhs@...atatu.com> wrote:
> > > > >
> > > > > On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@...gle.com> wrote:
> > > > > >
> > > > > > On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@...atatu.com> wrote:
> > > > > >
> > > > > > > It's the multiport redirection, particularly to ingress. When it get
> > > > > > > redirected to ingress it will get queued and then transitioned back.
> > > > > > > xmit struct wont catch this as a recursion, so MIRRED_NEST_LIMIT will
> > > > > > > not help you.
> > > > > > > Example (see the first accompanying tdc test):
> > > > > > > packet showing up on port0:ingress mirred redirect --> port1:egress
> > > > > > > packet showing up on port1:egress mirred redirect --> port0:ingress
> > > > > >
> > > > > > Have you tried recording both devices ?
> > > > > >
> > > > > > diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> > > > > > index f27b583def78e4afecc7112854b93d59c2520201..711fc2e31cb0451c07a39f9c94226357d5faec09
> > > > > > 100644
> > > > > > --- a/net/sched/act_mirred.c
> > > > > > +++ b/net/sched/act_mirred.c
> > > > > > @@ -445,15 +445,17 @@ TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
> > > > > >                 return retval;
> > > > > >         }
> > > > > >         for (i = 0; i < xmit->sched_mirred_nest; i++) {
> > > > > > -               if (xmit->sched_mirred_dev[i] != dev)
> > > > > > +               if (xmit->sched_mirred_dev[i] != dev &&
> > > > > > +                   xmit->sched_mirred_dev[i] != skb->dev)
> > > > > >                         continue;
> > > > > > -               pr_notice_once("tc mirred: loop on device %s\n",
> > > > > > -                              netdev_name(dev));
> > > > > > +               pr_notice_once("tc mirred: loop on device %s/%s\n",
> > > > > > +                              netdev_name(dev), netdev_name(skb->dev));
> > > > > >                 tcf_action_inc_overlimit_qstats(&m->common);
> > > > > >                 return retval;
> > > > > >         }
> > > > > >
> > > > > >         xmit->sched_mirred_dev[xmit->sched_mirred_nest++] = dev;
> > > > > > +       xmit->sched_mirred_dev[xmit->sched_mirred_nest++] = skb->dev;
> > > > > >
> > > > > >         m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
> > > > > >         m_eaction = READ_ONCE(m->tcfm_eaction);
> > > > >
> > > > > Did you mean not to decrement sched_mirred_nest twice?
> > > >
> > > > No, sorry, we should decrement twice of course.
> > > >
> > >
> > > Ok, I tested.
> > > While it "fixes" it - it's not really a fix. It works by ignoring direction.
> > > Example, this is not a loop but currently would be claimed to be a
> > > loop because port0 appears twice:
> > > port0 ingress --> port1 ingress --> port1 egress
> > > Note: port0 ingress and port0 egress cannot create a loop.
> >
> > I am not familiar with this stuff, can the direction be known and
> > taken into account ?
> >
>
> Yes, it can.
> To figure the target direction see tcf_mirred_act_wants_ingress() and
> to get what the current direction see code like:
> at_ingress = skb_at_tc_ingress(skb);
>
> > Really, anything but adding new bits in sk_buff.
>
> If we can fix it without restoring those two bits i will be happy.
> To repeat something i said earlier:
> The bigger challenge is somewhere along the way (after removing those
> two bits) we ended sending the egress->ingress to netif_rx() (see
> tcf_mirred_forward()), so any flow with that kind of setup will
> nullify your xmit count i.e when we come back for the next leg the
> xmit counter will be 0.
>

BTW, I have the attached patch but was waiting to see how this
discussion is heading.
It's when you have something like port0 egress --> port0 egress and
you use something like drr as root qdisc.

-- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -281,6 +281,14 @@ static int tcf_mirred_to_dev(struct sk_buff *skb,
struct tcf_mirred *m,

        want_ingress = tcf_mirred_act_wants_ingress(m_eaction);

+       if (dev == skb->dev && want_ingress == at_ingress) {
+               pr_notice_once("tc mirred: Loop (%s:%s --> %s:%s)\n",
+                              netdev_name(skb->dev),
+                              at_ingress?"ingress":"egress",
+                              netdev_name(dev),
+                              want_ingress?"ingress":"egress");
+               goto err_cant_do;
+       }
        /* All mirred/redirected skbs should clear previous ct info */
        nf_reset_ct(skb_to_send);
        if (want_ingress && !at_ingress) /* drop dst for egress -> ingress */

cheers,
jamal