| 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
| ||
|
Message-ID: <CAM0EoM=4FyGjjXdT=3f8FE18o+b2=_TZEbaure63MrU96szzAQ@mail.gmail.com>
Date: Sun, 12 Oct 2025 11:22:53 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
Kuniyuki Iwashima <kuniyu@...gle.com>, Willem de Bruijn <willemb@...gle.com>, netdev@...r.kernel.org,
eric.dumazet@...il.com
Subject: Re: [PATCH RFC net-next 2/5] net/sched: act_mirred: add loop detection
On Mon, Oct 6, 2025 at 3:31 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> We want to revert commit 0f022d32c3ec ("net/sched: Fix mirred deadlock
> on device recursion") because it adds code in the fast path, even when
> act_mirred is not used.
>
> Use an additional device pointers array in struct netdev_xmit
> and implement loop detection in tcf_mirred_is_act_redirect().
Patch series looks good!
This has the potential of (later on) fixing issue that are currently
broken after the TTL bits were taken away.
Small suggestion, the commit message was a bit confusing to me. How about:
Commit 0f022d32c3ec ("net/sched: Fix mirred deadlock on device
recursion") it adds code in the fast path, even when act_mirred is not
used. We revert in the next patch.
Prepare by adding an additional device pointers array in struct
netdev_xmit and implement loop detection in
tcf_mirred_is_act_redirect().
Please give us time to run tests on this set!
cheers,
jamal
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
> include/linux/netdevice_xmit.h | 9 ++++-
> net/sched/act_mirred.c | 62 +++++++++++++---------------------
> 2 files changed, 31 insertions(+), 40 deletions(-)
>
> diff --git a/include/linux/netdevice_xmit.h b/include/linux/netdevice_xmit.h
> index 813a19122ebbb2c6a04176330b1055b7c2b9c902..cc232508e695eefe95ea6e55a21978be11d5da83 100644
> --- a/include/linux/netdevice_xmit.h
> +++ b/include/linux/netdevice_xmit.h
> @@ -2,6 +2,12 @@
> #ifndef _LINUX_NETDEVICE_XMIT_H
> #define _LINUX_NETDEVICE_XMIT_H
>
> +#if IS_ENABLED(CONFIG_NET_ACT_MIRRED)
> +#define MIRRED_NEST_LIMIT 4
> +#endif
> +
> +struct net_device;
> +
> struct netdev_xmit {
> u16 recursion;
> u8 more;
> @@ -9,7 +15,8 @@ struct netdev_xmit {
> u8 skip_txqueue;
> #endif
> #if IS_ENABLED(CONFIG_NET_ACT_MIRRED)
> - u8 sched_mirred_nest;
> + u8 sched_mirred_nest;
> + struct net_device *sched_mirred_dev[MIRRED_NEST_LIMIT];
> #endif
> #if IS_ENABLED(CONFIG_NF_DUP_NETDEV)
> u8 nf_dup_skb_recursion;
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 5f01f567c934d3669d9a3058cff861a8fe5f88b6..f27b583def78e4afecc7112854b93d59c2520201 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -29,31 +29,6 @@
> static LIST_HEAD(mirred_list);
> static DEFINE_SPINLOCK(mirred_list_lock);
>
> -#define MIRRED_NEST_LIMIT 4
> -
> -#ifndef CONFIG_PREEMPT_RT
> -static u8 tcf_mirred_nest_level_inc_return(void)
> -{
> - return __this_cpu_inc_return(softnet_data.xmit.sched_mirred_nest);
> -}
> -
> -static void tcf_mirred_nest_level_dec(void)
> -{
> - __this_cpu_dec(softnet_data.xmit.sched_mirred_nest);
> -}
> -
> -#else
> -static u8 tcf_mirred_nest_level_inc_return(void)
> -{
> - return current->net_xmit.sched_mirred_nest++;
> -}
> -
> -static void tcf_mirred_nest_level_dec(void)
> -{
> - current->net_xmit.sched_mirred_nest--;
> -}
> -#endif
> -
> static bool tcf_mirred_is_act_redirect(int action)
> {
> return action == TCA_EGRESS_REDIR || action == TCA_INGRESS_REDIR;
> @@ -439,44 +414,53 @@ TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
> {
> struct tcf_mirred *m = to_mirred(a);
> int retval = READ_ONCE(m->tcf_action);
> - unsigned int nest_level;
> + struct netdev_xmit *xmit;
> bool m_mac_header_xmit;
> struct net_device *dev;
> - int m_eaction;
> + int i, m_eaction;
> u32 blockid;
>
> - nest_level = tcf_mirred_nest_level_inc_return();
> - if (unlikely(nest_level > MIRRED_NEST_LIMIT)) {
> +#ifdef CONFIG_PREEMPT_RT
> + xmit = ¤t->net_xmit;
> +#else
> + xmit = this_cpu_ptr(&softnet_data.xmit);
> +#endif
> + if (unlikely(xmit->sched_mirred_nest >= MIRRED_NEST_LIMIT)) {
> net_warn_ratelimited("Packet exceeded mirred recursion limit on dev %s\n",
> netdev_name(skb->dev));
> - retval = TC_ACT_SHOT;
> - goto dec_nest_level;
> + return TC_ACT_SHOT;
> }
>
> tcf_lastuse_update(&m->tcf_tm);
> tcf_action_update_bstats(&m->common, skb);
>
> blockid = READ_ONCE(m->tcfm_blockid);
> - if (blockid) {
> - retval = tcf_blockcast(skb, m, blockid, res, retval);
> - goto dec_nest_level;
> - }
> + if (blockid)
> + return tcf_blockcast(skb, m, blockid, res, retval);
>
> dev = rcu_dereference_bh(m->tcfm_dev);
> if (unlikely(!dev)) {
> pr_notice_once("tc mirred: target device is gone\n");
> tcf_action_inc_overlimit_qstats(&m->common);
> - goto dec_nest_level;
> + return retval;
> }
> + for (i = 0; i < xmit->sched_mirred_nest; i++) {
> + if (xmit->sched_mirred_dev[i] != dev)
> + continue;
> + pr_notice_once("tc mirred: loop on device %s\n",
> + netdev_name(dev));
> + tcf_action_inc_overlimit_qstats(&m->common);
> + return retval;
> + }
> +
> + xmit->sched_mirred_dev[xmit->sched_mirred_nest++] = dev;
>
> m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
> m_eaction = READ_ONCE(m->tcfm_eaction);
>
> retval = tcf_mirred_to_dev(skb, m, dev, m_mac_header_xmit, m_eaction,
> retval);
> -
> -dec_nest_level:
> - tcf_mirred_nest_level_dec();
> + xmit->sched_mirred_nest--;
>
> return retval;
> }
> --
> 2.51.0.618.g983fd99d29-goog
>
Powered by blists - more mailing lists