netdev - [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-next>] [day] [month] [year] [list]

Date:   Thu, 21 Jul 2022 18:19:22 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        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: [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path

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