Date:   Fri, 18 Mar 2022 00:50:35 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     Paolo Abeni <pabeni@...hat.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Richard Cochran <richardcochran@...il.com>
Subject: [RFC PATCH net-next] net: create a NETDEV_ETH_IOCTL notifier for DSA to reject PTP on DSA master

The fact that PTP 2-step TX timestamping is deeply broken on DSA
switches if the master also timestamps the same packets is well
documented by commit f685e609a301 ("net: dsa: Deny PTP on master if
switch supports it"). We attempt to help the users avoid shooting
themselves in the foot by making DSA reject the timestamping ioctls on
an interface that is a DSA master, and the switch tree beneath it
contains switches which are aware of PTP.

The only problem is that there isn't an established way of intercepting
ndo_eth_ioctl calls, so DSA creates avoidable burden upon the network
stack by creating a struct dsa_netdevice_ops with overlaid function
pointers that are manually checked from the relevant call sites. There
used to be 2 such dsa_netdevice_ops, but now, ndo_eth_ioctl is the only
one left.

In fact, the underlying reason which is prompting me to make this change
is that I'd like to hide as many DSA data structures from public API as
I can. But struct net_device :: dsa_ptr is a struct dsa_port (which is a
huge structure), and I'd like to create a smaller structure. I'd like
struct dsa_netdevice_ops to not be a part of this, so this is how the
need to delete it arose.

The established way for unrelated modules to react on a net device event
is via netdevice notifiers. These have the advantage of loose coupling,
i.e. they work even when DSA is built as module, without resorting to
static inline functions (which cannot offer the desired data structure
encapsulation).

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
I'd mostly like to take this opportunity to raise a discussion about how
to handle this. It's clear that calling the notifier chain is less
efficient than having some dev->dsa_ptr checks, but I'm not sure if the
ndo_eth_ioctl can tolerate the extra performance hit at the expense of
some code cleanliness.

Of course, what would be great is if we didn't have the limitation to
begin with, but the effort to add UAPI for multiple TX timestamps per
packet isn't proportional to the stated goal here, which is to hide some
DSA data structures.

 include/linux/netdevice.h | 10 +++++++-
 include/net/dsa.h         | 51 ---------------------------------------
 net/core/dev.c            |  7 +++---
 net/core/dev_ioctl.c      | 10 ++++++--
 net/dsa/dsa_priv.h        |  1 +
 net/dsa/master.c          | 26 +++-----------------
 net/dsa/slave.c           | 10 ++++++++
 7 files changed, 35 insertions(+), 80 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8cbe96ce0a2c..4b3f22b87193 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2736,6 +2736,7 @@ enum netdev_cmd {
 	NETDEV_OFFLOAD_XSTATS_DISABLE,
 	NETDEV_OFFLOAD_XSTATS_REPORT_USED,
 	NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
+	NETDEV_ETH_IOCTL,
 };
 const char *netdev_cmd_to_name(enum netdev_cmd cmd);
 
@@ -2786,6 +2787,12 @@ struct netdev_notifier_pre_changeaddr_info {
 	const unsigned char *dev_addr;
 };
 
+struct netdev_notifier_eth_ioctl_info {
+	struct netdev_notifier_info info; /* must be first */
+	struct ifreq *ifr;
+	unsigned int cmd;
+};
+
 enum netdev_offload_xstats_type {
 	NETDEV_OFFLOAD_XSTATS_TYPE_L3 = 1,
 };
@@ -2842,7 +2849,8 @@ netdev_notifier_info_to_extack(const struct netdev_notifier_info *info)
 }
 
 int call_netdevice_notifiers(unsigned long val, struct net_device *dev);
-
+int call_netdevice_notifiers_info(unsigned long val,
+				  struct netdev_notifier_info *info);
 
 extern rwlock_t				dev_base_lock;		/* Device list lock */
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index a2a68f532f59..d80dd68ae5d8 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -103,16 +103,6 @@ struct dsa_device_ops {
 	bool promisc_on_master;
 };
 
-/* This structure defines the control interfaces that are overlayed by the
- * DSA layer on top of the DSA CPU/management net_device instance. This is
- * used by the core net_device layer while calling various net_device_ops
- * function pointers.
- */
-struct dsa_netdevice_ops {
-	int (*ndo_eth_ioctl)(struct net_device *dev, struct ifreq *ifr,
-			     int cmd);
-};
-
 #define DSA_TAG_DRIVER_ALIAS "dsa_tag-"
 #define MODULE_ALIAS_DSA_TAG_DRIVER(__proto)				\
 	MODULE_ALIAS(DSA_TAG_DRIVER_ALIAS __stringify(__proto##_VALUE))
@@ -314,11 +304,6 @@ struct dsa_port {
 	 */
 	const struct ethtool_ops *orig_ethtool_ops;
 
-	/*
-	 * Original copy of the master netdev net_device_ops
-	 */
-	const struct dsa_netdevice_ops *netdev_ops;
-
 	/* List of MAC addresses that must be forwarded on this port.
 	 * These are only valid on CPU ports and DSA links.
 	 */
@@ -1278,42 +1263,6 @@ static inline void dsa_tag_generic_flow_dissect(const struct sk_buff *skb,
 #endif
 }
 
-#if IS_ENABLED(CONFIG_NET_DSA)
-static inline int __dsa_netdevice_ops_check(struct net_device *dev)
-{
-	int err = -EOPNOTSUPP;
-
-	if (!dev->dsa_ptr)
-		return err;
-
-	if (!dev->dsa_ptr->netdev_ops)
-		return err;
-
-	return 0;
-}
-
-static inline int dsa_ndo_eth_ioctl(struct net_device *dev, struct ifreq *ifr,
-				    int cmd)
-{
-	const struct dsa_netdevice_ops *ops;
-	int err;
-
-	err = __dsa_netdevice_ops_check(dev);
-	if (err)
-		return err;
-
-	ops = dev->dsa_ptr->netdev_ops;
-
-	return ops->ndo_eth_ioctl(dev, ifr, cmd);
-}
-#else
-static inline int dsa_ndo_eth_ioctl(struct net_device *dev, struct ifreq *ifr,
-				    int cmd)
-{
-	return -EOPNOTSUPP;
-}
-#endif
-
 void dsa_unregister_switch(struct dsa_switch *ds);
 int dsa_register_switch(struct dsa_switch *ds);
 void dsa_switch_shutdown(struct dsa_switch *ds);
diff --git a/net/core/dev.c b/net/core/dev.c
index 75bab5b0dbae..49ab1895a319 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -159,8 +159,6 @@ struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;
 struct list_head ptype_all __read_mostly;	/* Taps */
 
 static int netif_rx_internal(struct sk_buff *skb);
-static int call_netdevice_notifiers_info(unsigned long val,
-					 struct netdev_notifier_info *info);
 static int call_netdevice_notifiers_extack(unsigned long val,
 					   struct net_device *dev,
 					   struct netlink_ext_ack *extack);
@@ -1622,6 +1620,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
 	N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
 	N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)
 	N(OFFLOAD_XSTATS_REPORT_USED) N(OFFLOAD_XSTATS_REPORT_DELTA)
+	N(ETH_IOCTL)
 	}
 #undef N
 	return "UNKNOWN_NETDEV_EVENT";
@@ -1920,8 +1919,8 @@ static void move_netdevice_notifiers_dev_net(struct net_device *dev,
  *	are as for raw_notifier_call_chain().
  */
 
-static int call_netdevice_notifiers_info(unsigned long val,
-					 struct netdev_notifier_info *info)
+int call_netdevice_notifiers_info(unsigned long val,
+				  struct netdev_notifier_info *info)
 {
 	struct net *net = dev_net(info->dev);
 	int ret;
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 1b807d119da5..c6f3d5e22ee4 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -244,10 +244,16 @@ static int dev_eth_ioctl(struct net_device *dev,
 			 struct ifreq *ifr, unsigned int cmd)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
+	struct netdev_notifier_eth_ioctl_info info = {
+		.info.dev = dev,
+		.ifr = ifr,
+		.cmd = cmd,
+	};
 	int err;
 
-	err = dsa_ndo_eth_ioctl(dev, ifr, cmd);
-	if (err == 0 || err != -EOPNOTSUPP)
+	err = call_netdevice_notifiers_info(NETDEV_ETH_IOCTL, &info.info);
+	err = notifier_to_errno(err);
+	if (err)
 		return err;
 
 	if (ops->ndo_eth_ioctl) {
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index f20bdd8ea0a8..04b8723c23bb 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -195,6 +195,7 @@ static inline int dsa_tag_protocol_overhead(const struct dsa_device_ops *ops)
 /* master.c */
 int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp);
 void dsa_master_teardown(struct net_device *dev);
+int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd);
 
 static inline struct net_device *dsa_master_find_slave(struct net_device *dev,
 						       int device, int port)
diff --git a/net/dsa/master.c b/net/dsa/master.c
index 991c2930d631..e84d5d35bbd8 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -187,12 +187,11 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
 	}
 }
 
-static int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct dsa_port *cpu_dp = dev->dsa_ptr;
 	struct dsa_switch *ds = cpu_dp->ds;
 	struct dsa_switch_tree *dst;
-	int err = -EOPNOTSUPP;
 	struct dsa_port *dp;
 
 	dst = ds->dst;
@@ -210,16 +209,9 @@ static int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		break;
 	}
 
-	if (dev->netdev_ops->ndo_eth_ioctl)
-		err = dev->netdev_ops->ndo_eth_ioctl(dev, ifr, cmd);
-
-	return err;
+	return 0;
 }
 
-static const struct dsa_netdevice_ops dsa_netdev_ops = {
-	.ndo_eth_ioctl = dsa_master_ioctl,
-};
-
 static int dsa_master_ethtool_setup(struct net_device *dev)
 {
 	struct dsa_port *cpu_dp = dev->dsa_ptr;
@@ -254,12 +246,6 @@ static void dsa_master_ethtool_teardown(struct net_device *dev)
 	cpu_dp->orig_ethtool_ops = NULL;
 }
 
-static void dsa_netdev_ops_set(struct net_device *dev,
-			       const struct dsa_netdevice_ops *ops)
-{
-	dev->dsa_ptr->netdev_ops = ops;
-}
-
 /* Keep the master always promiscuous if the tagging protocol requires that
  * (garbles MAC DA) or if it doesn't support unicast filtering, case in which
  * it would revert to promiscuous mode as soon as we call dev_uc_add() on it
@@ -363,16 +349,13 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 	if (ret)
 		goto out_err_reset_promisc;
 
-	dsa_netdev_ops_set(dev, &dsa_netdev_ops);
-
 	ret = sysfs_create_group(&dev->dev.kobj, &dsa_group);
 	if (ret)
-		goto out_err_ndo_teardown;
+		goto out_err_ethtool_teardown;
 
 	return ret;
 
-out_err_ndo_teardown:
-	dsa_netdev_ops_set(dev, NULL);
+out_err_ethtool_teardown:
 	dsa_master_ethtool_teardown(dev);
 out_err_reset_promisc:
 	dsa_master_set_promiscuity(dev, -1);
@@ -382,7 +365,6 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 void dsa_master_teardown(struct net_device *dev)
 {
 	sysfs_remove_group(&dev->dev.kobj, &dsa_group);
-	dsa_netdev_ops_set(dev, NULL);
 	dsa_master_ethtool_teardown(dev);
 	dsa_master_set_promiscuity(dev, -1);
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d24b6bf845c1..7e4186f523c8 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2749,6 +2749,16 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 
 		return NOTIFY_OK;
 	}
+	case NETDEV_ETH_IOCTL: {
+		struct netdev_notifier_eth_ioctl_info *info = ptr;
+		int err;
+
+		if (!netdev_uses_dsa(dev))
+			return NOTIFY_DONE;
+
+		err = dsa_master_ioctl(dev, info->ifr, info->cmd);
+		return notifier_from_errno(err);
+	}
 	default:
 		break;
 	}
-- 
2.25.1

Powered by blists - more mailing lists