| 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: <a9f0f57f-a73c-72dd-3fc6-fe22e2d3e466@engleder-embedded.com>
Date: Sun, 2 Apr 2023 20:23:02 +0200
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: Maxim Georgiev <glipus@...il.com>, kory.maincent@...tlin.com
Cc: kuba@...nel.org, netdev@...r.kernel.org,
maxime.chevallier@...tlin.com, vladimir.oltean@....com
Subject: Re: [PATCH net-next RFC v2] Add NDOs for hardware timestamp get/set
On 02.04.23 16:24, Maxim Georgiev wrote:
> Current NIC driver API demands drivers supporting hardware timestamping
> to support SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs. Handling these IOCTLs
> requires dirivers to implement request parameter structure translation
dirivers -> drivers
> between user and kernel address spaces, handling possible
> translation failures, etc. This translation code is pretty much
> identical across most of the NIC drivers that support SIOCGHWTSTAMP/
> SIOCSHWTSTAMP.
> This patch extends NDO functiuon set with ndo_hwtstamp_get/set
functiuon -> function
> functions, implements SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTL translation
> to ndo_hwtstamp_get/set function calls including parameter structure
> translation and translation error handling.
>
> This patch is sent out as RFC.
> It still pending on basic testing. Implementing ndo_hwtstamp_get/set
> in netdevsim driver should allow manual testing of the request
> translation logic. Also is there a way to automate this testing?
>
> Suggested-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Maxim Georgiev <glipus@...il.com>
> ---
> Changes in v2:
> - Introduced kernel_hwtstamp_config structure
> - Added netlink_ext_ack* and kernel_hwtstamp_config* as NDO hw timestamp
> function parameters
> - Reodered function variable declarations in dev_hwtstamp()
> - Refactored error handling logic in dev_hwtstamp()
> - Split dev_hwtstamp() into GET and SET versions
> - Changed net_hwtstamp_validate() to accept struct hwtstamp_config *
> as a parameter
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 39 ++++++-----
> drivers/net/netdevsim/netdev.c | 26 +++++++
> drivers/net/netdevsim/netdevsim.h | 1 +
> include/linux/netdevice.h | 21 ++++++
> include/uapi/linux/net_tstamp.h | 15 ++++
> net/core/dev_ioctl.c | 81 ++++++++++++++++++----
> 6 files changed, 149 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 6f5c16aebcbf..5b98f7257c77 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -6161,7 +6161,9 @@ static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
> /**
> * e1000e_hwtstamp_set - control hardware time stamping
> * @netdev: network interface device structure
> - * @ifr: interface request
> + * @config: hwtstamp_config structure containing request parameters
> + * @kernel_config: kernel version of config parameter structure.
> + * @extack: netlink request parameters.
'.' at end of parameter line or not? You mixed it.
> *
> * Outgoing time stamping can be enabled and disabled. Play nice and
> * disable it when requested, although it shouldn't cause any overhead
> @@ -6174,20 +6176,19 @@ static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
> * specified. Matching the kind of event packet is not supported, with the
> * exception of "all V2 events regardless of level 2 or 4".
> **/
> -static int e1000e_hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
> +static int e1000e_hwtstamp_set(struct net_device *netdev,
> + struct hwtstamp_config *config,
> + struct kernel_hwtstamp_config *kernel_config,
> + struct netlink_ext_ack *extack)
> {
> struct e1000_adapter *adapter = netdev_priv(netdev);
> - struct hwtstamp_config config;
> int ret_val;
>
> - if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> - return -EFAULT;
> -
> - ret_val = e1000e_config_hwtstamp(adapter, &config);
> + ret_val = e1000e_config_hwtstamp(adapter, config);
> if (ret_val)
> return ret_val;
>
> - switch (config.rx_filter) {
> + switch (config->rx_filter) {
> case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> case HWTSTAMP_FILTER_PTP_V2_SYNC:
> @@ -6199,22 +6200,24 @@ static int e1000e_hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
> * by hardware so notify the caller the requested packets plus
> * some others are time stamped.
> */
> - config.rx_filter = HWTSTAMP_FILTER_SOME;
> + config->rx_filter = HWTSTAMP_FILTER_SOME;
> break;
> default:
> break;
> }
> -
> - return copy_to_user(ifr->ifr_data, &config,
> - sizeof(config)) ? -EFAULT : 0;
> + return ret_val;
I would expect 'return 0;'.
> }
>
> -static int e1000e_hwtstamp_get(struct net_device *netdev, struct ifreq *ifr)
> +static int e1000e_hwtstamp_get(struct net_device *netdev,
> + struct hwtstamp_config *config,
> + struct kernel_hwtstamp_config *kernel_config,
> + struct netlink_ext_ack *extack)
> {
> struct e1000_adapter *adapter = netdev_priv(netdev);
>
> - return copy_to_user(ifr->ifr_data, &adapter->hwtstamp_config,
> - sizeof(adapter->hwtstamp_config)) ? -EFAULT : 0;
> + memcpy(config, &adapter->hwtstamp_config,
> + sizeof(adapter->hwtstamp_config));
> + return 0;
> }
>
> static int e1000_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> @@ -6224,10 +6227,6 @@ static int e1000_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> case SIOCGMIIREG:
> case SIOCSMIIREG:
> return e1000_mii_ioctl(netdev, ifr, cmd);
> - case SIOCSHWTSTAMP:
> - return e1000e_hwtstamp_set(netdev, ifr);
> - case SIOCGHWTSTAMP:
> - return e1000e_hwtstamp_get(netdev, ifr);
> default:
> return -EOPNOTSUPP;
> }
> @@ -7365,6 +7364,8 @@ static const struct net_device_ops e1000e_netdev_ops = {
> .ndo_set_features = e1000_set_features,
> .ndo_fix_features = e1000_fix_features,
> .ndo_features_check = passthru_features_check,
> + .ndo_hwtstamp_get = e1000e_hwtstamp_get,
> + .ndo_hwtstamp_set = e1000e_hwtstamp_set,
> };
>
> /**
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 35fa1ca98671..502715c6e9e1 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -238,6 +238,30 @@ nsim_set_features(struct net_device *dev, netdev_features_t features)
> return 0;
> }
>
> +static int
> +nsim_hwtstamp_get(struct net_device *dev,
> + struct hwtstamp_config *config,
> + struct kernel_hwtstamp_config *kernel_config,
> + struct netlink_ext_ack *extack)
> +{
> + struct netdevsim *ns = netdev_priv(dev);
> +
> + memcpy(config, &ns->hw_tstamp_config, sizeof(ns->hw_tstamp_config));
> + return 0;
> +}
> +
> +static int
> +nsim_hwtstamp_ges(struct net_device *dev,
nsim_hwtstamp_ges -> nsim_hwtstamp_get
> + struct hwtstamp_config *config,
> + struct kernel_hwtstamp_config *kernel_config,
> + struct netlink_ext_ack *extack)
> +{
> + struct netdevsim *ns = netdev_priv(dev);
> +
> + memcpy(&ns->hw_tstamp_config, config, sizeof(ns->hw_tstamp_config));
> + return 0;
> +}
> +
> static const struct net_device_ops nsim_netdev_ops = {
> .ndo_start_xmit = nsim_start_xmit,
> .ndo_set_rx_mode = nsim_set_rx_mode,
> @@ -256,6 +280,8 @@ static const struct net_device_ops nsim_netdev_ops = {
> .ndo_setup_tc = nsim_setup_tc,
> .ndo_set_features = nsim_set_features,
> .ndo_bpf = nsim_bpf,
> + .ndo_hwtstamp_get = nsim_hwtstamp_get,
> + .ndo_hwtstamp_set = nsim_hwtstamp_get,
nsim_hwtstamp_get -> nsim_hwtstamp_set
> };
>
> static const struct net_device_ops nsim_vf_netdev_ops = {
> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> index 7d8ed8d8df5c..c6efd2383552 100644
> --- a/drivers/net/netdevsim/netdevsim.h
> +++ b/drivers/net/netdevsim/netdevsim.h
> @@ -102,6 +102,7 @@ struct netdevsim {
> } udp_ports;
>
> struct nsim_ethtool ethtool;
> + struct hwtstamp_config hw_tstamp_config;
> };
>
> struct netdevsim *
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index c8c634091a65..078c9284930a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -57,6 +57,8 @@
> struct netpoll_info;
> struct device;
> struct ethtool_ops;
> +struct hwtstamp_config;
> +struct kernel_hwtstamp_config;
> struct phy_device;
> struct dsa_port;
> struct ip_tunnel_parm;
> @@ -1411,6 +1413,17 @@ struct netdev_net_notifier {
> * Get hardware timestamp based on normal/adjustable time or free running
> * cycle counter. This function is required if physical clock supports a
> * free running cycle counter.
> + * int (*ndo_hwtstamp_get)(struct net_device *dev,
> + * struct hwtstamp_config *config,
> + * struct kernel_hwtstamp_config *kernel_config,
> + * struct netlink_ext_ack *extack);
> + * Get hardware timestamping parameters currently configured for NIC
'configured for' -> 'configured for'
> + * device.
> + * int (*ndo_hwtstamp_set)(struct net_device *dev,
> + * struct hwtstamp_config *config,
> + * struct kernel_hwtstamp_config *kernel_config,
> + * struct netlink_ext_ack *extack);
> + * Set hardware timestamping parameters for NIC device.
> */
> struct net_device_ops {
> int (*ndo_init)(struct net_device *dev);
> @@ -1645,6 +1658,14 @@ struct net_device_ops {
> ktime_t (*ndo_get_tstamp)(struct net_device *dev,
> const struct skb_shared_hwtstamps *hwtstamps,
> bool cycles);
> + int (*ndo_hwtstamp_get)(struct net_device *dev,
> + struct hwtstamp_config *config,
> + struct kernel_hwtstamp_config *kernel_config,
> + struct netlink_ext_ack *extack);
> + int (*ndo_hwtstamp_set)(struct net_device *dev,
> + struct hwtstamp_config *config,
> + struct kernel_hwtstamp_config *kernel_config,
> + struct netlink_ext_ack *extack);
> };
>
> struct xdp_metadata_ops {
> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index a2c66b3d7f0f..547f73beb479 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -79,6 +79,21 @@ struct hwtstamp_config {
> int rx_filter;
> };
>
> +/**
> + * struct kernel_hwtstamp_config - %SIOCGHWTSTAMP and %SIOCSHWTSTAMP parameter
> + *
> + * @dummy: a placeholder field added to work around empty struct language
> + * restriction
> + *
> + * This structure passed as a parameter to NDO methods called in
> + * response to SIOCGHWTSTAMP and SIOCSHWTSTAMP IOCTLs.
> + * The structure is effectively empty for now. Before adding new fields
> + * to the structure "dummy" placeholder field should be removed.
> + */
> +struct kernel_hwtstamp_config {
> + u8 dummy;
> +};
> +
> /* possible values for hwtstamp_config->flags */
> enum hwtstamp_flags {
> /*
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 846669426236..c145afb3f77e 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(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:
> @@ -277,6 +273,63 @@ static int dev_siocbond(struct net_device *dev,
> return -EOPNOTSUPP;
> }
>
> +static int dev_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
> +{
> + const struct net_device_ops *ops = dev->netdev_ops;
> + struct hwtstamp_config config;
> + int err;
> +
> + err = dsa_ndo_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
> + if (err == 0 || err != -EOPNOTSUPP)
> + return err;
> +
> + if (!ops->ndo_hwtstamp_get)
> + return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
> +
> + if (!netif_device_present(dev))
> + return -ENODEV;
> +
> + err = ops->ndo_hwtstamp_get(dev, &config, NULL, NULL);
> + if (err)
> + return err;
> +
> + if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> + return -EFAULT;
> + return 0;
> +}
> +
> +static int dev_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
> +{
> + const struct net_device_ops *ops = dev->netdev_ops;
> + struct hwtstamp_config config;
> + int err;
> +
> + if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> + return -EFAULT;
> +
> + err = net_hwtstamp_validate(&config);
> + if (err)
> + return err;
> +
> + err = dsa_ndo_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
> + if (err == 0 || err != -EOPNOTSUPP)
> + return err;
> +
> + if (!ops->ndo_hwtstamp_set)
> + return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
> +
> + if (!netif_device_present(dev))
> + return -ENODEV;
> +
> + err = ops->ndo_hwtstamp_set(dev, &config, NULL, NULL);
> + if (err)
> + return err;
> +
> + if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> + return -EFAULT;
> + return 0;
> +}
> +
> static int dev_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
> void __user *data, unsigned int cmd)
> {
> @@ -391,11 +444,11 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
> rtnl_lock();
> return err;
>
> + case SIOCGHWTSTAMP:
> + return dev_hwtstamp_get(dev, ifr);
> +
> case SIOCSHWTSTAMP:
> - err = net_hwtstamp_validate(ifr);
> - if (err)
> - return err;
> - fallthrough;
> + return dev_hwtstamp_set(dev, ifr);
>
> /*
> * Unknown or private ioctl
> @@ -407,9 +460,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
>
> if (cmd == SIOCGMIIPHY ||
> cmd == SIOCGMIIREG ||
> - cmd == SIOCSMIIREG ||
> - cmd == SIOCSHWTSTAMP ||
> - cmd == SIOCGHWTSTAMP) {
> + cmd == SIOCSMIIREG) {
> err = dev_eth_ioctl(dev, ifr, cmd);
> } else if (cmd == SIOCBONDENSLAVE ||
> cmd == SIOCBONDRELEASE ||
Powered by blists - more mailing lists