From e5b42483ccce50d5b957f474fd332afd4ef0c27b Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Fri, 24 Mar 2023 23:11:49 +0100 Subject: dev_ioctl: fix a W=1 warning This fixes the following warning when compiled with GCC 12.2.0 and W=1. net/core/dev_ioctl.c:475: warning: Function parameter or member 'data' not described in 'dev_ioctl' Signed-off-by: Heiner Kallweit Reviewed-by: Simon Horman Signed-off-by: David S. Miller --- net/core/dev_ioctl.c | 1 + 1 file changed, 1 insertion(+) (limited to 'net/core/dev_ioctl.c') diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 5cdbfbf9a7dc..846669426236 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -462,6 +462,7 @@ EXPORT_SYMBOL(dev_load); * @net: the applicable net namespace * @cmd: command to issue * @ifr: pointer to a struct ifreq in user space + * @data: data exchanged with userspace * @need_copyout: whether or not copy_to_user() should be called * * Issue ioctl functions to devices. This is normally called by the -- cgit v1.2.3 From 00d521b39307c3232bdc685c2e9fd82ed973ac24 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Sun, 2 Apr 2023 15:37:49 +0300 Subject: net: don't abuse "default" case for unknown ioctl in dev_ifsioc() The "switch (cmd)" block from dev_ifsioc() gained a bit too much unnecessary manual handling of "cmd" in the "default" case, starting with the private ioctls. Clean that up by using the "ellipsis" gcc extension, adding separate cases for the rest of the ioctls, and letting the default case only return -EINVAL. Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: David S. Miller --- net/core/dev_ioctl.c | 42 +++++++++++++++++++----------------------- 1 file changed, 19 insertions(+), 23 deletions(-) (limited to 'net/core/dev_ioctl.c') diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 846669426236..1c0256ea522f 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -391,36 +391,32 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data, rtnl_lock(); return err; + case SIOCDEVPRIVATE ... SIOCDEVPRIVATE + 15: + return dev_siocdevprivate(dev, ifr, data, cmd); + case SIOCSHWTSTAMP: err = net_hwtstamp_validate(ifr); if (err) return err; fallthrough; - /* - * Unknown or private ioctl - */ - default: - if (cmd >= SIOCDEVPRIVATE && - cmd <= SIOCDEVPRIVATE + 15) - return dev_siocdevprivate(dev, ifr, data, cmd); - - if (cmd == SIOCGMIIPHY || - cmd == SIOCGMIIREG || - cmd == SIOCSMIIREG || - cmd == SIOCSHWTSTAMP || - cmd == SIOCGHWTSTAMP) { - err = dev_eth_ioctl(dev, ifr, cmd); - } else if (cmd == SIOCBONDENSLAVE || - cmd == SIOCBONDRELEASE || - cmd == SIOCBONDSETHWADDR || - cmd == SIOCBONDSLAVEINFOQUERY || - cmd == SIOCBONDINFOQUERY || - cmd == SIOCBONDCHANGEACTIVE) { - err = dev_siocbond(dev, ifr, cmd); - } else - err = -EINVAL; + case SIOCGHWTSTAMP: + case SIOCGMIIPHY: + case SIOCGMIIREG: + case SIOCSMIIREG: + return dev_eth_ioctl(dev, ifr, cmd); + + case SIOCBONDENSLAVE: + case SIOCBONDRELEASE: + case SIOCBONDSETHWADDR: + case SIOCBONDSLAVEINFOQUERY: + case SIOCBONDINFOQUERY: + case SIOCBONDCHANGEACTIVE: + return dev_siocbond(dev, ifr, cmd); + /* Unknown ioctl */ + default: + err = -EINVAL; } return err; } -- cgit v1.2.3 From 1193db2a55b6b04f296f03affdfa80c16ecc3814 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Sun, 2 Apr 2023 15:37:50 +0300 Subject: net: simplify handling of dsa_ndo_eth_ioctl() return code In the expression "x == 0 || x != -95", the term "x == 0" does not change the expression's logical value, because 0 != -95, and so, if x is 0, the expression would still be true by virtue of the second term. If x is non-zero, the expression depends on the truth value of the second term anyway. As such, the first term is redundant and can be deleted. Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: David S. Miller --- net/core/dev_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'net/core/dev_ioctl.c') diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 1c0256ea522f..b299fb23fcfa 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -249,7 +249,7 @@ static int dev_eth_ioctl(struct net_device *dev, int err; err = dsa_ndo_eth_ioctl(dev, ifr, cmd); - if (err == 0 || err != -EOPNOTSUPP) + if (err != -EOPNOTSUPP) return err; if (ops->ndo_eth_ioctl) { -- cgit v1.2.3 From 4ee58e1e56800b589afe31c34547e2bc0c59f586 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Sun, 2 Apr 2023 15:37:51 +0300 Subject: net: promote SIOCSHWTSTAMP and SIOCGHWTSTAMP ioctls to dedicated handlers DSA does not want to intercept all ioctls handled by dev_eth_ioctl(), only SIOCSHWTSTAMP. This can be seen from commit f685e609a301 ("net: dsa: Deny PTP on master if switch supports it"). However, the way in which the dsa_ndo_eth_ioctl() is called would suggest otherwise. Split the handling of SIOCSHWTSTAMP and SIOCGHWTSTAMP ioctls into separate case statements of dev_ifsioc(), and make each one call its own sub-function. This also removes the dsa_ndo_eth_ioctl() call from dev_eth_ioctl(), which from now on exclusively handles PHY ioctls. Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: David S. Miller --- net/core/dev_ioctl.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) (limited to 'net/core/dev_ioctl.c') diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index b299fb23fcfa..3b1402f6897c 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -246,20 +246,34 @@ static int dev_eth_ioctl(struct net_device *dev, struct ifreq *ifr, unsigned int cmd) { const struct net_device_ops *ops = dev->netdev_ops; + + if (!ops->ndo_eth_ioctl) + return -EOPNOTSUPP; + + if (!netif_device_present(dev)) + return -ENODEV; + + return ops->ndo_eth_ioctl(dev, ifr, cmd); +} + +static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr) +{ + return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP); +} + +static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) +{ int err; - err = dsa_ndo_eth_ioctl(dev, ifr, cmd); - if (err != -EOPNOTSUPP) + err = net_hwtstamp_validate(ifr); + if (err) return err; - if (ops->ndo_eth_ioctl) { - if (netif_device_present(dev)) - err = ops->ndo_eth_ioctl(dev, ifr, cmd); - else - err = -ENODEV; - } + err = dsa_ndo_eth_ioctl(dev, ifr, SIOCSHWTSTAMP); + if (err != -EOPNOTSUPP) + return err; - return err; + return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP); } static int dev_siocbond(struct net_device *dev, @@ -395,12 +409,11 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data, return dev_siocdevprivate(dev, ifr, data, cmd); case SIOCSHWTSTAMP: - err = net_hwtstamp_validate(ifr); - if (err) - return err; - fallthrough; + return dev_set_hwtstamp(dev, ifr); case SIOCGHWTSTAMP: + return dev_get_hwtstamp(dev, ifr); + case SIOCGMIIPHY: case SIOCGMIIREG: case SIOCSMIIREG: -- cgit v1.2.3 From d5d5fd8f2552f6b9c50d3937c77b6783f99fbe83 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Sun, 2 Apr 2023 15:37:52 +0300 Subject: net: move copy_from_user() out of net_hwtstamp_validate() The kernel will want to start using the more meaningful struct hwtstamp_config pointer in more places, so move the copy_from_user() at the beginning of dev_set_hwtstamp() in order to get to that, and pass this argument to net_hwtstamp_validate(). Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: David S. Miller --- net/core/dev_ioctl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'net/core/dev_ioctl.c') diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 3b1402f6897c..34a0da5fbcfc 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -183,22 +183,18 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm return err; } -static int net_hwtstamp_validate(struct ifreq *ifr) +static int net_hwtstamp_validate(const struct hwtstamp_config *cfg) { - struct hwtstamp_config cfg; enum hwtstamp_tx_types tx_type; enum hwtstamp_rx_filters rx_filter; int tx_type_valid = 0; int rx_filter_valid = 0; - if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg))) - return -EFAULT; - - if (cfg.flags & ~HWTSTAMP_FLAG_MASK) + if (cfg->flags & ~HWTSTAMP_FLAG_MASK) return -EINVAL; - tx_type = cfg.tx_type; - rx_filter = cfg.rx_filter; + tx_type = cfg->tx_type; + rx_filter = cfg->rx_filter; switch (tx_type) { case HWTSTAMP_TX_OFF: @@ -263,9 +259,13 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr) static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) { + struct hwtstamp_config cfg; int err; - err = net_hwtstamp_validate(ifr); + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg))) + return -EFAULT; + + err = net_hwtstamp_validate(&cfg); if (err) return err; -- cgit v1.2.3 From c4bffeaa8d50b7279c5a76597efa4b06e709df63 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Sun, 2 Apr 2023 15:37:53 +0300 Subject: net: add struct kernel_hwtstamp_config and make net_hwtstamp_validate() use it Jakub Kicinski suggested that we may want to add new UAPI for controlling hardware timestamping through netlink in the future, and in that case, we will be limited to the struct hwtstamp_config that is currently passed in fixed binary format through the SIOCGHWTSTAMP and SIOCSHWTSTAMP ioctls. It would be good if new kernel code already started operating on an extensible kernel variant of that structure, similar in concept to struct kernel_ethtool_coalesce vs struct ethtool_coalesce. Since struct hwtstamp_config is in include/uapi/linux/net_tstamp.h, here we introduce include/linux/net_tstamp.h which shadows that other header, but also includes it, so that existing includers of this header work as before. In addition to that, we add the definition for the kernel-only structure, and a helper which translates all fields by manual copying. I am doing a manual copy in order to not force the alignment (or type) of the fields of struct kernel_hwtstamp_config to be the same as of struct hwtstamp_config, even though now, they are the same. Link: https://lore.kernel.org/netdev/20230330223519.36ce7d23@kernel.org/ Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: David S. Miller --- net/core/dev_ioctl.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'net/core/dev_ioctl.c') diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 34a0da5fbcfc..c532ef4d5dff 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -183,7 +183,7 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm return err; } -static int net_hwtstamp_validate(const struct hwtstamp_config *cfg) +static int net_hwtstamp_validate(const struct kernel_hwtstamp_config *cfg) { enum hwtstamp_tx_types tx_type; enum hwtstamp_rx_filters rx_filter; @@ -259,13 +259,16 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr) static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) { + struct kernel_hwtstamp_config kernel_cfg; struct hwtstamp_config cfg; int err; if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg))) return -EFAULT; - err = net_hwtstamp_validate(&cfg); + hwtstamp_config_to_kernel(&kernel_cfg, &cfg); + + err = net_hwtstamp_validate(&kernel_cfg); if (err) return err; -- cgit v1.2.3 From 88c0a6b503b7f4fffb68a8d49c3987870c5b1d6b Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Sun, 2 Apr 2023 15:37:55 +0300 Subject: net: create a netdev notifier for DSA to reject PTP on DSA master The fact that PTP 2-step TX timestamping is broken on DSA switches if the master also timestamps the same packets is 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. There is an ongoing effort to migrate driver-visible hardware timestamping control from the ndo_eth_ioctl() based API to a new ndo_hwtstamp_set() model, but DSA actively prevents that migration, since dsa_master_ioctl() is currently coded to manually call the master's legacy ndo_eth_ioctl(), and so, whenever a network device driver would be converted to the new API, DSA's restrictions would be circumvented, because any device could be used as a DSA master. The established way for unrelated modules to react on a net device event is via netdevice notifiers. So we create a new notifier which gets called whenever there is an attempt to change hardware timestamping settings on a device. Finally, there is another reason why a netdev notifier will be a good idea, besides strictly DSA, and this has to do with PHY timestamping. With ndo_eth_ioctl(), all MAC drivers must manually call phy_has_hwtstamp() before deciding whether to act upon SIOCSHWTSTAMP, otherwise they must pass this ioctl to the PHY driver via phy_mii_ioctl(). With the new ndo_hwtstamp_set() API, it will be desirable to simply not make any calls into the MAC device driver when timestamping should be performed at the PHY level. But there exist drivers, such as the lan966x switch, which need to install packet traps for PTP regardless of whether they are the layer that provides the hardware timestamps, or the PHY is. That would be impossible to support with the new API. The proposal there, too, is to introduce a netdev notifier which acts as a better cue for switching drivers to add or remove PTP packet traps, than ndo_hwtstamp_set(). The one introduced here "almost" works there as well, except for the fact that packet traps should only be installed if the PHY driver succeeded to enable hardware timestamping, whereas here, we need to deny hardware timestamping on the DSA master before it actually gets enabled. This is why this notifier is called "PRE_", and the notifier that would get used for PHY timestamping and packet traps would be called NETDEV_CHANGE_HWTSTAMP. This isn't a new concept, for example NETDEV_CHANGEUPPER and NETDEV_PRECHANGEUPPER do the same thing. In expectation of future netlink UAPI, we also pass a non-NULL extack pointer to the netdev notifier, and we make DSA populate it with an informative reason for the rejection. To avoid making it go to waste, we make the ioctl-based dev_set_hwtstamp() create a fake extack and print the message to the kernel log. Link: https://lore.kernel.org/netdev/20230401191215.tvveoi3lkawgg6g4@skbuf/ Link: https://lore.kernel.org/netdev/20230310164451.ls7bbs6pdzs4m6pw@skbuf/ Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: David S. Miller --- net/core/dev_ioctl.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) (limited to 'net/core/dev_ioctl.c') diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index c532ef4d5dff..6d772837eb3f 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -259,7 +259,11 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr) static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) { + struct netdev_notifier_hwtstamp_info info = { + .info.dev = dev, + }; struct kernel_hwtstamp_config kernel_cfg; + struct netlink_ext_ack extack = {}; struct hwtstamp_config cfg; int err; @@ -272,9 +276,17 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) if (err) return err; - err = dsa_ndo_eth_ioctl(dev, ifr, SIOCSHWTSTAMP); - if (err != -EOPNOTSUPP) + info.info.extack = &extack; + info.config = &kernel_cfg; + + err = call_netdevice_notifiers_info(NETDEV_PRE_CHANGE_HWTSTAMP, + &info.info); + err = notifier_to_errno(err); + if (err) { + if (extack._msg) + netdev_err(dev, "%s\n", extack._msg); return err; + } return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP); } -- cgit v1.2.3 From 5a17818682cf43ad0fdd6035945f3b7a8c9dc5e9 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Thu, 6 Apr 2023 14:42:46 +0300 Subject: net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub There was a sort of rush surrounding commit 88c0a6b503b7 ("net: create a netdev notifier for DSA to reject PTP on DSA master"), due to a desire to convert DSA's attempt to deny TX timestamping on a DSA master to something that doesn't block the kernel-wide API conversion from ndo_eth_ioctl() to ndo_hwtstamp_set(). What was required was a mechanism that did not depend on ndo_eth_ioctl(), and what was provided was a mechanism that did not depend on ndo_eth_ioctl(), while at the same time introducing something that wasn't absolutely necessary - a new netdev notifier. There have been objections from Jakub Kicinski that using notifiers in general when they are not absolutely necessary creates complications to the control flow and difficulties to maintainers who look at the code. So there is a desire to not use notifiers. In addition to that, the notifier chain gets called even if there is no DSA in the system and no one is interested in applying any restriction. Take the model of udp_tunnel_nic_ops and introduce a stub mechanism, through which net/core/dev_ioctl.c can call into DSA even when CONFIG_NET_DSA=m. Compared to the code that existed prior to the notifier conversion, aka what was added in commits: - 4cfab3566710 ("net: dsa: Add wrappers for overloaded ndo_ops") - 3369afba1e46 ("net: Call into DSA netdevice_ops wrappers") this is different because we are not overloading any struct net_device_ops of the DSA master anymore, but rather, we are exposing a rather specific functionality which is orthogonal to which API is used to enable it - ndo_eth_ioctl() or ndo_hwtstamp_set(). Also, what is similar is that both approaches use function pointers to get from built-in code to DSA. There is no point in replicating the function pointers towards __dsa_master_hwtstamp_validate() once for every CPU port (dev->dsa_ptr). Instead, it is sufficient to introduce a singleton struct dsa_stubs, built into the kernel, which contains a single function pointer to __dsa_master_hwtstamp_validate(). I find this approach preferable to what we had originally, because dev->dsa_ptr->netdev_ops->ndo_do_ioctl() used to require going through struct dsa_port (dev->dsa_ptr), and so, this was incompatible with any attempts to add any data encapsulation and hide DSA data structures from the outside world. Link: https://lore.kernel.org/netdev/20230403083019.120b72fd@kernel.org/ Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: David S. Miller --- net/core/dev_ioctl.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) (limited to 'net/core/dev_ioctl.c') diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index 6d772837eb3f..3730945ee294 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -7,7 +7,7 @@ #include #include #include -#include +#include #include #include "dev.h" @@ -259,9 +259,6 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr) static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) { - struct netdev_notifier_hwtstamp_info info = { - .info.dev = dev, - }; struct kernel_hwtstamp_config kernel_cfg; struct netlink_ext_ack extack = {}; struct hwtstamp_config cfg; @@ -276,12 +273,7 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) if (err) return err; - info.info.extack = &extack; - info.config = &kernel_cfg; - - err = call_netdevice_notifiers_info(NETDEV_PRE_CHANGE_HWTSTAMP, - &info.info); - err = notifier_to_errno(err); + err = dsa_master_hwtstamp_validate(dev, &kernel_cfg, &extack); if (err) { if (extack._msg) netdev_err(dev, "%s\n", extack._msg); -- cgit v1.2.3