| 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: Wed, 5 Apr 2023 15:31:30 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Maxim Georgiev <glipus@...il.com>
Cc: kory.maincent@...tlin.com, kuba@...nel.org, netdev@...r.kernel.org,
maxime.chevallier@...tlin.com, vadim.fedorenko@...ux.dev,
richardcochran@...il.com, gerhard@...leder-embedded.com
Subject: Re: [RFC PATCH v3 1/5] Add NDOs for hardware timestamp get/set
On Wed, Apr 05, 2023 at 12:31:44AM -0600, Maxim Georgiev wrote:
> Current NIC driver API demands drivers supporting hardware timestamping
> to implement handling logic for SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs.
> Handling these IOCTLs requires dirivers to implement request parameter
> structure translation 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
> 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.
>
> Suggested-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Maxim Georgiev <glipus@...il.com>
> ---
> Changes in v3:
> - Moved individual driver conversions to separate patches
>
> 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
> ---
> include/linux/net_tstamp.h | 8 ++++++++
> include/linux/netdevice.h | 16 ++++++++++++++++
> net/core/dev_ioctl.c | 36 ++++++++++++++++++++++++++++++++++--
> 3 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
> index fd67f3cc0c4b..063260475e77 100644
> --- a/include/linux/net_tstamp.h
> +++ b/include/linux/net_tstamp.h
> @@ -30,4 +30,12 @@ static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kern
> kernel_cfg->rx_filter = cfg->rx_filter;
> }
>
> +static inline void hwtstamp_kernel_to_config(struct hwtstamp_config *cfg,
> + const struct kernel_hwtstamp_config *kernel_cfg)
The reason why I suggested the name "hwtstamp_config_from_kernel()" was
to not break apart "hwtstamp" and "config", which together form the name
of one structure (hwtstamp_config).
> +{
> + cfg->flags = kernel_cfg->flags;
> + cfg->tx_type = kernel_cfg->tx_type;
> + cfg->rx_filter = kernel_cfg->rx_filter;
> +}
> +
> #endif /* _LINUX_NET_TIMESTAMPING_H_ */
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a740be3bb911..8356002d0ac0 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -57,6 +57,7 @@
> struct netpoll_info;
> struct device;
> struct ethtool_ops;
> +struct kernel_hwtstamp_config;
> struct phy_device;
> struct dsa_port;
> struct ip_tunnel_parm;
> @@ -1412,6 +1413,15 @@ 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 kernel_hwtstamp_config *kernel_config,
> + * struct netlink_ext_ack *extack);
> + * Get hardware timestamping parameters currently configured for NIC
> + * device.
> + * int (*ndo_hwtstamp_set)(struct net_device *dev,
> + * 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);
> @@ -1646,6 +1656,12 @@ 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 kernel_hwtstamp_config *kernel_config,
> + struct netlink_ext_ack *extack);
> + int (*ndo_hwtstamp_set)(struct net_device *dev,
> + struct kernel_hwtstamp_config *kernel_config,
> + struct netlink_ext_ack *extack);
> };
>
> struct xdp_metadata_ops {
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 6d772837eb3f..736f310a0661 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -254,11 +254,30 @@ static int dev_eth_ioctl(struct net_device *dev,
>
> static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> {
> - return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
> + const struct net_device_ops *ops = dev->netdev_ops;
> + struct kernel_hwtstamp_config kernel_cfg;
Should we zero-initialize kernel_cfg (" = {}"), in case the driver does
not bother to populate, say, "flags"?
> + struct hwtstamp_config config;
> + int 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, &kernel_cfg, NULL);
> + if (err)
> + return err;
> +
> + hwtstamp_kernel_to_config(&config, &kernel_cfg);
> + if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> + return -EFAULT;
> + return 0;
> }
>
> static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> {
> + const struct net_device_ops *ops = dev->netdev_ops;
> struct netdev_notifier_hwtstamp_info info = {
> .info.dev = dev,
> };
> @@ -288,7 +307,20 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> return err;
> }
>
> - return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
> + 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, &kernel_cfg, NULL);
> + if (err)
> + return err;
> +
> + hwtstamp_kernel_to_config(&cfg, &kernel_cfg);
Cosmetic blank line here
> + if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
> + return -EFAULT;
and here?
(and in equivalent positions in dev_get_hwtstamp())
> + return 0;
> }
>
> static int dev_siocbond(struct net_device *dev,
> --
> 2.39.2
>
Powered by blists - more mailing lists