| 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
| ||
|
Date: Sun, 2 Apr 2023 19:10:52 -0600
From: Max Georgiev <glipus@...il.com>
To: Gerhard Engleder <gerhard@...leder-embedded.com>
Cc: kory.maincent@...tlin.com, 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 Sun, Apr 2, 2023 at 12:23 PM Gerhard Engleder
<gerhard@...leder-embedded.com> wrote:
>
> 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
Thank you for catching it!
>
> > 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
My bad. Thank you for catching it!
>
> > 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.
What was I thinking!
Let me remove the periods since the original parameter descriptions
didn't have them.
>
> > *
> > * 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;'.
Makes perfect sense. Let me fix that.
>
> > }
> >
> > -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
This was intended to be nsim_hwtstamp_set(). But nevertheless I got it wrong.
Fixing it...
>
> > + 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
Aha, this one masked the previous error you caught.
Thank you for pointing it out!
>
> > };
> >
> > 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'
Removing this double whitespace. Thank you!
>
> > + * 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 ||
Gerhard, thank you for reviewing the patch and highlighting all these errors!
I'll incorporate the fixes to ver 3 of the patch.
Powered by blists - more mailing lists