[PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

When doing multiport mirroring we dont detect infinite loops.

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

Example 2 (see the second accompanying tdc test)
port0 egress --> port1 ingress --> port0 egress

Fix this by remembering the source dev where mirred ran as opposed to
destination/target dev

Fixes: fe946a751d9b ("net/sched: act_mirred: add loop detection")
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_mirred.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index f27b583def78..7315179197fd 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -453,7 +453,7 @@ TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
 		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);
-- 
2.34.1

[PATCH net-next 2/2] selftests/tc-testing: Add test cases exercising mirred redirects (with loops)

[Jamal Hadi Salim]

From: Victor Nogueira <victor@mojatatu.com>

Add two tdc test cases for mirred which cause loops.
The first one does a redirect dev1 ingress -> dummy egress -> dev1 ingress
The second one does a redirect dummy egress -> dev1 ingress -> dummy egress

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 .../tc-testing/tc-tests/actions/mirred.json   | 103 ++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/mirred.json b/tools/testing/selftests/tc-testing/tc-tests/actions/mirred.json
index b73bd255ea36..97800f2e26f2 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/mirred.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/mirred.json
@@ -1052,5 +1052,108 @@
             "$TC qdisc del dev $DEV1 ingress_block 21 clsact",
             "$TC actions flush action mirred"
         ]
+    },
+    {
+        "id": "6941",
+        "name": "Redirect multiport: dev1 ingress -> dummy egress -> dev1 ingress (Loop)",
+        "category": [
+            "filter",
+            "mirred"
+        ],
+        "plugins": {
+            "requires": [
+                "nsPlugin",
+                "scapyPlugin"
+            ]
+        },
+        "scapy": {
+            "iface": "$DEV0",
+            "count": 1,
+            "packet": "Ether()/IP(dst='10.10.10.1', src='10.10.10.10')/ICMP()"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$TC qdisc add dev $DEV1 clsact",
+            "$TC filter add dev $DEV1 ingress protocol ip prio 10 matchall action mirred egress redirect dev $DUMMY index 1",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY clsact",
+            "$TC filter add dev $DUMMY egress protocol ip prio 10 matchall action mirred ingress redirect dev $DEV1 index 2"
+        ],
+        "cmdUnderTest": "$TC -j -s actions get action mirred index 1",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -j -s actions get action mirred index 1",
+        "matchJSON": [
+            {
+                "total acts": 0
+            },
+            {
+                "actions": [
+                    {
+                        "order": 1,
+                        "kind": "mirred",
+                        "mirred_action": "redirect",
+                        "direction": "egress",
+                        "index": 1,
+                        "stats": {
+                            "packets": 1
+                        },
+                        "not_in_hw": true
+                    }
+                ]
+            }
+        ],
+        "teardown": [
+            "$TC qdisc del dev $DEV1 clsact",
+            "$TC qdisc del dev $DUMMY clsact"
+        ]
+    },
+    {
+        "id": "ceb4",
+        "name": "Redirect multiport: dummy egress -> dev1 ingress -> dummy egress (Loop)",
+        "category": [
+            "filter",
+            "mirred"
+        ],
+        "plugins": {
+            "requires": [
+                "nsPlugin"
+            ]
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY clsact",
+            "$TC filter add dev $DUMMY egress protocol ip prio 10 matchall action mirred ingress redirect dev $DEV1 index 1",
+            "$TC qdisc add dev $DEV1 clsact",
+            "$TC filter add dev $DEV1 ingress protocol ip prio 10 matchall action mirred egress redirect dev $DUMMY index 2"
+        ],
+        "cmdUnderTest": "ping -c1 -W0.01 -I $DUMMY 10.10.10.1",
+        "expExitCode": "1",
+        "verifyCmd": "$TC -j -s actions get action mirred index 1",
+        "matchJSON": [
+            {
+                "total acts": 0
+            },
+            {
+                "actions": [
+                    {
+                        "order": 1,
+                        "kind": "mirred",
+                        "mirred_action": "redirect",
+                        "direction": "ingress",
+                        "index": 1,
+                        "stats": {
+                            "packets": 2,
+                            "overlimits": 1
+                        },
+                        "not_in_hw": true
+                    }
+                ]
+            }
+        ],
+        "teardown": [
+            "$TC qdisc del dev $DUMMY clsact",
+            "$TC qdisc del dev $DEV1 clsact"
+        ]
     }
 ]
-- 
2.34.1

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jakub Kicinski]

On Mon, 24 Nov 2025 15:08:24 -0500 Jamal Hadi Salim wrote:
> When doing multiport mirroring we dont detect infinite loops.
> 
> 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
> 
> Example 2 (see the second accompanying tdc test)
> port0 egress --> port1 ingress --> port0 egress
> 
> Fix this by remembering the source dev where mirred ran as opposed to
> destination/target dev
> 
> Fixes: fe946a751d9b ("net/sched: act_mirred: add loop detection")
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Hm, this breaks net/fib_tests.sh:

# 23.80 [+0.00] IPv4 rp_filter tests
# 25.63 [+1.84]     TEST: rp_filter passes local packets                                [FAIL]
# 26.65 [+1.02]     TEST: rp_filter passes loopback packets                             [FAIL]

https://netdev-3.bots.linux.dev/vmksft-net/results/400301/10-fib-tests-sh/stdout

Not making a statement on whether the fix itself is acceptable
but if it is we gotta fix that test too..
-- 
pw-bot: cr

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

On Mon, Nov 24, 2025 at 5:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 24 Nov 2025 15:08:24 -0500 Jamal Hadi Salim wrote:
> > When doing multiport mirroring we dont detect infinite loops.
> >
> > 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
> >
> > Example 2 (see the second accompanying tdc test)
> > port0 egress --> port1 ingress --> port0 egress
> >
> > Fix this by remembering the source dev where mirred ran as opposed to
> > destination/target dev
> >
> > Fixes: fe946a751d9b ("net/sched: act_mirred: add loop detection")
> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Hm, this breaks net/fib_tests.sh:
>
> # 23.80 [+0.00] IPv4 rp_filter tests
> # 25.63 [+1.84]     TEST: rp_filter passes local packets                                [FAIL]
> # 26.65 [+1.02]     TEST: rp_filter passes loopback packets                             [FAIL]
>
> https://netdev-3.bots.linux.dev/vmksft-net/results/400301/10-fib-tests-sh/stdout
>
> Not making a statement on whether the fix itself is acceptable
> but if it is we gotta fix that test too..

Sigh. I will look into it later.
Note: Fixing this (and the netem loop issue) would have been trivial
if we had those two skb ttl fields that were taken away.
The human hours spent trying to detect and prevent infinite loops!

cheers,
jamal

> --
> pw-bot: cr

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

On Tue, Nov 25, 2025 at 11:20 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Mon, Nov 24, 2025 at 5:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Mon, 24 Nov 2025 15:08:24 -0500 Jamal Hadi Salim wrote:
> > > When doing multiport mirroring we dont detect infinite loops.
> > >
> > > 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
> > >
> > > Example 2 (see the second accompanying tdc test)
> > > port0 egress --> port1 ingress --> port0 egress
> > >
> > > Fix this by remembering the source dev where mirred ran as opposed to
> > > destination/target dev
> > >
> > > Fixes: fe946a751d9b ("net/sched: act_mirred: add loop detection")
> > > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> >
> > Hm, this breaks net/fib_tests.sh:
> >
> > # 23.80 [+0.00] IPv4 rp_filter tests
> > # 25.63 [+1.84]     TEST: rp_filter passes local packets                                [FAIL]
> > # 26.65 [+1.02]     TEST: rp_filter passes loopback packets                             [FAIL]
> >
> > https://netdev-3.bots.linux.dev/vmksft-net/results/400301/10-fib-tests-sh/stdout
> >
> > Not making a statement on whether the fix itself is acceptable
> > but if it is we gotta fix that test too..
>
> Sigh. I will look into it later.
> Note: Fixing this (and the netem loop issue) would have been trivial
> if we had those two skb ttl fields that were taken away.
> The human hours spent trying to detect and prevent infinite loops!
>

Ok, I spent time on this and frankly cant find a way to fix the
infinite loop that avoids adding _a lot more_ complexity.
We need loop state to be associated with the skb. I will restore the
two skb bits and test. From inspection, i see one bit free but i may
be able to steal a bit from somewhere. I will post an RFC and at
minimal that will start a discussion and maybe someone will come up
with a better way of solving this.

Eric, there's another issue as well involving example
port0:egress->port0:egress  - I have a patch but will post it later
after some testing.

cheers,
jamal

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Eric Dumazet]

On Wed, Nov 26, 2025 at 8:26 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Tue, Nov 25, 2025 at 11:20 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Mon, Nov 24, 2025 at 5:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Mon, 24 Nov 2025 15:08:24 -0500 Jamal Hadi Salim wrote:
> > > > When doing multiport mirroring we dont detect infinite loops.
> > > >
> > > > 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
> > > >
> > > > Example 2 (see the second accompanying tdc test)
> > > > port0 egress --> port1 ingress --> port0 egress
> > > >
> > > > Fix this by remembering the source dev where mirred ran as opposed to
> > > > destination/target dev
> > > >
> > > > Fixes: fe946a751d9b ("net/sched: act_mirred: add loop detection")
> > > > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > >
> > > Hm, this breaks net/fib_tests.sh:
> > >
> > > # 23.80 [+0.00] IPv4 rp_filter tests
> > > # 25.63 [+1.84]     TEST: rp_filter passes local packets                                [FAIL]
> > > # 26.65 [+1.02]     TEST: rp_filter passes loopback packets                             [FAIL]
> > >
> > > https://netdev-3.bots.linux.dev/vmksft-net/results/400301/10-fib-tests-sh/stdout
> > >
> > > Not making a statement on whether the fix itself is acceptable
> > > but if it is we gotta fix that test too..
> >
> > Sigh. I will look into it later.
> > Note: Fixing this (and the netem loop issue) would have been trivial
> > if we had those two skb ttl fields that were taken away.
> > The human hours spent trying to detect and prevent infinite loops!
> >
>
> Ok, I spent time on this and frankly cant find a way to fix the
> infinite loop that avoids adding _a lot more_ complexity.
> We need loop state to be associated with the skb. I will restore the
> two skb bits and test. From inspection, i see one bit free but i may
> be able to steal a bit from somewhere. I will post an RFC and at
> minimal that will start a discussion and maybe someone will come up
> with a better way of solving this.
>
> Eric, there's another issue as well involving example
> port0:egress->port0:egress  - I have a patch but will post it later
> after some testing.

Adding bits for mirred in skb would be quite unfortunate.
Argument of 'we have available space, let's use/waste it' is not very appealing.

Do we really need to accept more than one mirred ?

What is a legitimate/realistic use for MIRRED_NEST_LIMIT == 4 ?

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

On Wed, Nov 26, 2025 at 11:41 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Nov 26, 2025 at 8:26 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Tue, Nov 25, 2025 at 11:20 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Mon, Nov 24, 2025 at 5:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > >
> > > > On Mon, 24 Nov 2025 15:08:24 -0500 Jamal Hadi Salim wrote:
> > > > > When doing multiport mirroring we dont detect infinite loops.
> > > > >
> > > > > 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
> > > > >
> > > > > Example 2 (see the second accompanying tdc test)
> > > > > port0 egress --> port1 ingress --> port0 egress
> > > > >
> > > > > Fix this by remembering the source dev where mirred ran as opposed to
> > > > > destination/target dev
> > > > >
> > > > > Fixes: fe946a751d9b ("net/sched: act_mirred: add loop detection")
> > > > > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > > >
> > > > Hm, this breaks net/fib_tests.sh:
> > > >
> > > > # 23.80 [+0.00] IPv4 rp_filter tests
> > > > # 25.63 [+1.84]     TEST: rp_filter passes local packets                                [FAIL]
> > > > # 26.65 [+1.02]     TEST: rp_filter passes loopback packets                             [FAIL]
> > > >
> > > > https://netdev-3.bots.linux.dev/vmksft-net/results/400301/10-fib-tests-sh/stdout
> > > >
> > > > Not making a statement on whether the fix itself is acceptable
> > > > but if it is we gotta fix that test too..
> > >
> > > Sigh. I will look into it later.
> > > Note: Fixing this (and the netem loop issue) would have been trivial
> > > if we had those two skb ttl fields that were taken away.
> > > The human hours spent trying to detect and prevent infinite loops!
> > >
> >
> > Ok, I spent time on this and frankly cant find a way to fix the
> > infinite loop that avoids adding _a lot more_ complexity.
> > We need loop state to be associated with the skb. I will restore the
> > two skb bits and test. From inspection, i see one bit free but i may
> > be able to steal a bit from somewhere. I will post an RFC and at
> > minimal that will start a discussion and maybe someone will come up
> > with a better way of solving this.
> >
> > Eric, there's another issue as well involving example
> > port0:egress->port0:egress  - I have a patch but will post it later
> > after some testing.
>
> Adding bits for mirred in skb would be quite unfortunate.
> Argument of 'we have available space, let's use/waste it' is not very appealing.
>
> Do we really need to accept more than one mirred ?
>
> What is a legitimate/realistic use for MIRRED_NEST_LIMIT == 4 ?

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

cheers,
jamal

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Eric Dumazet]

On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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);

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]

On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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);

Will try this in a bit. From outset i dont think it will help.
But here's what i tested and it works. Of course needs a lot more
scrutiny and testing but should be able to explain the idea.

cheers,
jamal

[-- Attachment #2: p-ttl --]
[-- Type: application/octet-stream, Size: 4204 bytes --]

diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index d3dc0914450a..65155b532a1c 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -123,8 +123,7 @@ static void ifb_ri_tasklet(struct tasklet_struct *t)
 		}
 		rcu_read_unlock();
 		skb->skb_iif = txp->dev->ifindex;
-
-		if (!skb->from_ingress) {
+		if (!tc_skb_cb(skb)->from_ingress) {
 			dev_queue_xmit(skb);
 		} else {
 			skb_pull_rcsum(skb, skb->mac_len);
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ff90281ddf90..118f0c2a249c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1000,6 +1000,8 @@ struct sk_buff {
 	/* Indicates the inner headers are valid in the skbuff. */
 	__u8			encapsulation:1;
 	__u8			encap_hdr_csum:1;
+	/*XXX: find reasonable ifded? other than  mirred, netem could use it*/
+	__u8			ttl:2;
 	__u8			csum_valid:1;
 #ifdef CONFIG_IPV6_NDISC_NODETYPE
 	__u8			ndisc_nodetype:2;
@@ -1016,9 +1018,6 @@ struct sk_buff {
 	__u8			offload_l3_fwd_mark:1;
 #endif
 	__u8			redirected:1;
-#ifdef CONFIG_NET_REDIRECT
-	__u8			from_ingress:1;
-#endif
 #ifdef CONFIG_NETFILTER_SKIP_EGRESS
 	__u8			nf_skip_egress:1;
 #endif
@@ -5347,35 +5346,6 @@ static inline __wsum lco_csum(struct sk_buff *skb)
 	return csum_partial(l4_hdr, csum_start - l4_hdr, partial);
 }
 
-static inline bool skb_is_redirected(const struct sk_buff *skb)
-{
-	return skb->redirected;
-}
-
-static inline void skb_set_redirected(struct sk_buff *skb, bool from_ingress)
-{
-	skb->redirected = 1;
-#ifdef CONFIG_NET_REDIRECT
-	skb->from_ingress = from_ingress;
-	if (skb->from_ingress)
-		skb_clear_tstamp(skb);
-#endif
-}
-
-static inline void skb_reset_redirect(struct sk_buff *skb)
-{
-	skb->redirected = 0;
-}
-
-static inline void skb_set_redirected_noclear(struct sk_buff *skb,
-					      bool from_ingress)
-{
-	skb->redirected = 1;
-#ifdef CONFIG_NET_REDIRECT
-	skb->from_ingress = from_ingress;
-#endif
-}
-
 static inline bool skb_csum_is_sctp(struct sk_buff *skb)
 {
 #if IS_ENABLED(CONFIG_IP_SCTP)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 94966692ccdf..ed80f7ae5059 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -1069,6 +1069,7 @@ struct tc_skb_cb {
 	u8 post_ct:1;
 	u8 post_ct_snat:1;
 	u8 post_ct_dnat:1;
+	u8 from_ingress:1;
 };
 
 static inline struct tc_skb_cb *tc_skb_cb(const struct sk_buff *skb)
@@ -1091,6 +1092,35 @@ static inline void tcf_set_drop_reason(const struct sk_buff *skb,
 	tc_skb_cb(skb)->drop_reason = reason;
 }
 
+static inline bool skb_is_redirected(const struct sk_buff *skb)
+{
+	return skb->redirected;
+}
+
+static inline void skb_set_redirected(struct sk_buff *skb, bool from_ingress)
+{
+	skb->redirected = 1;
+#ifdef CONFIG_NET_REDIRECT
+	tc_skb_cb(skb)->from_ingress = from_ingress;
+	if (tc_skb_cb(skb)->from_ingress)
+		skb_clear_tstamp(skb);
+#endif
+}
+
+static inline void skb_reset_redirect(struct sk_buff *skb)
+{
+	skb->redirected = 0;
+}
+
+static inline void skb_set_redirected_noclear(struct sk_buff *skb,
+					      bool from_ingress)
+{
+	skb->redirected = 1;
+#ifdef CONFIG_NET_REDIRECT
+	tc_skb_cb(skb)->from_ingress = from_ingress;
+#endif
+}
+
 /* Instead of calling kfree_skb() while root qdisc lock is held,
  * queue the skb for future freeing at end of __dev_xmit_skb()
  */
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index f27b583def78..a083ba63ae9e 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -309,8 +309,10 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
 
 		skb_set_redirected(skb_to_send, skb_to_send->tc_at_ingress);
 
+		skb_to_send->ttl++;
 		err = tcf_mirred_forward(at_ingress, want_ingress, skb_to_send);
 	} else {
+		skb_to_send->ttl++;
 		err = tcf_mirred_forward(at_ingress, want_ingress, skb_to_send);
 	}
 	if (err)
@@ -425,7 +427,8 @@ TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
 #else
 	xmit = this_cpu_ptr(&softnet_data.xmit);
 #endif
-	if (unlikely(xmit->sched_mirred_nest >= MIRRED_NEST_LIMIT)) {
+
+	if (skb->ttl >= MIRRED_NEST_LIMIT - 1) {
 		net_warn_ratelimited("Packet exceeded mirred recursion limit on dev %s\n",
 				     netdev_name(skb->dev));
 		return TC_ACT_SHOT;

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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?
I dont have time today but will continue early AM.

cheers,
jamal

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Eric Dumazet]

On Wed, Nov 26, 2025 at 12:20 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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.

> I dont have time today but will continue early AM.
>
> cheers,
> jamal

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

On Wed, Nov 26, 2025 at 3:30 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Nov 26, 2025 at 12:20 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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.

cheers,
jamal

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Eric Dumazet]

On Thu, Nov 27, 2025 at 6:45 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Wed, Nov 26, 2025 at 3:30 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Wed, Nov 26, 2025 at 12:20 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > > On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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 ?

Really, anything but adding new bits in sk_buff.

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

On Thu, Nov 27, 2025 at 10:11 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Nov 27, 2025 at 6:45 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Wed, Nov 26, 2025 at 3:30 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Wed, Nov 26, 2025 at 12:20 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > >
> > > > On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@google.com> wrote:
> > > > >
> > > > > On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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.

cheers,
jamal

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

On Thu, Nov 27, 2025 at 10:23 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Thu, Nov 27, 2025 at 10:11 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Nov 27, 2025 at 6:45 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Wed, Nov 26, 2025 at 3:30 PM Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > > On Wed, Nov 26, 2025 at 12:20 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > > >
> > > > > On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@google.com> wrote:
> > > > > >
> > > > > > On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

On Thu, Nov 27, 2025 at 11:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Thu, Nov 27, 2025 at 10:23 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Thu, Nov 27, 2025 at 10:11 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Thu, Nov 27, 2025 at 6:45 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > >
> > > > On Wed, Nov 26, 2025 at 3:30 PM Eric Dumazet <edumazet@google.com> wrote:
> > > > >
> > > > > On Wed, Nov 26, 2025 at 12:20 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > > > >
> > > > > > On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@google.com> wrote:
> > > > > > >
> > > > > > > On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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 */
>

Eric,
Are you still looking at this?

cheers,
jamal

Re: [PATCH net-next 1/2] net/sched: act_mirred: Fix infinite loop

[Jamal Hadi Salim]

On Wed, Dec 3, 2025 at 9:27 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Thu, Nov 27, 2025 at 11:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Thu, Nov 27, 2025 at 10:23 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Thu, Nov 27, 2025 at 10:11 AM Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > > On Thu, Nov 27, 2025 at 6:45 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > > >
> > > > > On Wed, Nov 26, 2025 at 3:30 PM Eric Dumazet <edumazet@google.com> wrote:
> > > > > >
> > > > > > On Wed, Nov 26, 2025 at 12:20 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > > > > >
> > > > > > > On Wed, Nov 26, 2025 at 1:20 PM Eric Dumazet <edumazet@google.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Nov 26, 2025 at 10:14 AM Jamal Hadi Salim <jhs@mojatatu.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 */
> >
>
> Eric,
> Are you still looking at this?
>

i will send a bunch of RFC patches. Maybe one is a clear "easy fix",
so i will send that one out against net. Just a bit of testing needed.

cheers,
jamal
> cheers,
> jamal