netdev - Re: [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path

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] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 22 Jul 2022 18:27:32 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Davide Caratti <dcaratti@...hat.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net/sched: act_mirred: avoid printout in the
 traffic path

You dont want to use the target device if it is operationally/admin down.
But if that happens momentarily then it comes back up - what happens then?

cheers,
jamal



On Thu, Jul 21, 2022 at 12:19 PM Davide Caratti <dcaratti@...hat.com> wrote:
>
> when tc-mirred outputs to a device that's not up, dmesg is cluttered with
> messages like:
>
>  tc mirred to Houston: device br-int is down
>
> we can't completely remove this printout: users might be relying on it to
> detect setups where tc-mirred drops everything, as discussed earlier [1].
> however, we can at least reduce the amount of these messages, and improve
> their content as follows:
>  - add a pr_notice(...) in the .init() function, to warn users of missing
>    IFF_UP flag on the target of a newly added tc-mirred action
>  - check for NETDEV_DOWN in the .notifier_call() function, and add proper
>    pr_notice(...) to warn users of missing/down target devices
>
> [1] https://lore.kernel.org/netdev/CAM_iQpUvn+ijyZtLmca3n+nZmHY9cMmPYwZMp5BTv10bLUhg3Q@mail.gmail.com/
>
> Suggested-by: Cong Wang <xiyou.wangcong@...il.com>
> CC: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> Signed-off-by: Davide Caratti <dcaratti@...hat.com>
> ---
>  net/sched/act_mirred.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index a1d70cf86843..4af6073e472b 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -178,6 +178,13 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>                         err = -ENODEV;
>                         goto put_chain;
>                 }
> +               if (!(ndev->flags & IFF_UP))
> +                       pr_notice("tc mirred: action %i %s on %s while device is down",
> +                                 m->tcf_index,
> +                                 tcf_mirred_is_act_redirect(parm->eaction) ?
> +                                       "redirects" : "mirrors",
> +                                 ndev->name);
> +
>                 mac_header_xmit = dev_is_mac_header_xmit(ndev);
>                 odev = rcu_replace_pointer(m->tcfm_dev, ndev,
>                                           lockdep_is_held(&m->tcf_lock));
> @@ -251,16 +258,8 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
>         m_eaction = READ_ONCE(m->tcfm_eaction);
>         retval = READ_ONCE(m->tcf_action);
>         dev = rcu_dereference_bh(m->tcfm_dev);
> -       if (unlikely(!dev)) {
> -               pr_notice_once("tc mirred: target device is gone\n");
> +       if (unlikely(!dev || !(dev->flags & IFF_UP)))
>                 goto out;
> -       }
> -
> -       if (unlikely(!(dev->flags & IFF_UP))) {
> -               net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
> -                                      dev->name);
> -               goto out;
> -       }
>
>         /* we could easily avoid the clone only if called by ingress and clsact;
>          * since we can't easily detect the clsact caller, skip clone only for
> @@ -397,16 +396,21 @@ static int mirred_device_event(struct notifier_block *unused,
>         struct tcf_mirred *m;
>
>         ASSERT_RTNL();
> -       if (event == NETDEV_UNREGISTER) {
> +       if (event == NETDEV_UNREGISTER || event == NETDEV_DOWN) {
>                 spin_lock(&mirred_list_lock);
>                 list_for_each_entry(m, &mirred_list, tcfm_list) {
>                         spin_lock_bh(&m->tcf_lock);
>                         if (tcf_mirred_dev_dereference(m) == dev) {
> -                               netdev_put(dev, &m->tcfm_dev_tracker);
> -                               /* Note : no rcu grace period necessary, as
> -                                * net_device are already rcu protected.
> -                                */
> -                               RCU_INIT_POINTER(m->tcfm_dev, NULL);
> +                               pr_notice("tc mirred: target device %s is %s\n",
> +                                         dev->name,
> +                                         event == NETDEV_UNREGISTER ? "gone" : "down");
> +                               if (event == NETDEV_UNREGISTER) {
> +                                       netdev_put(dev, &m->tcfm_dev_tracker);
> +                                       /* Note : no rcu grace period necessary, as
> +                                        * net_device are already rcu protected.
> +                                        */
> +                                       RCU_INIT_POINTER(m->tcfm_dev, NULL);
> +                               }
>                         }
>                         spin_unlock_bh(&m->tcf_lock);
>                 }
> --
> 2.35.3
>