Re: [RFC PATCH net-next v6 2/5] net: Add ifreq pointer field to kernel_hwtstamp_config structure

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Mon, May 01, 2023 at 10:31:47PM -0600, Maxim Georgiev wrote:
> Considering the stackable nature of drivers there will be situations
> where a driver implementing ndo_hwtstamp_get/set functions will have
> to translate requests back to SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs
> to pass them to lower level drivers that do not provide
> ndo_hwtstamp_get/set callbacks. To simplify request translation in
> such scenarios let's include a pointer to the original struct ifreq
> to kernel_hwtstamp_config structure.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Maxim Georgiev <glipus@gmail.com>
> 
> Notes:
>   Changes in v6:
>   - Patch title was updated. No code changes.
>   Changes in v5:
>   - kernel_hwtstamp_config kdoc is updated with the new field
>     descriptions.
>   Changes in V4:
>   - Introducing KERNEL_HWTSTAMP_FLAG_IFR_RESULT flag indicating that
>     the operation results are returned in the ifr referred by
>     struct kernel_hwtstamp_config instead of kernel_hwtstamp_config
>     glags/tx_type/rx_filter fields.
>   - Implementing generic_hwtstamp_set/set_lower() functions
>     which will be used by vlan, maxvlan, bond and potentially
>     other drivers translating ndo_hwtstamp_set/set calls to
>     lower level drivers.

Usually the notes go below the "---" line so that they are stripped once
the patch gets applied, but are otherwise visible to reviewers. It's
good having them nonetheless.

> ---
>  include/linux/net_tstamp.h |  9 +++++
>  include/linux/netdevice.h  |  6 +++
>  net/core/dev_ioctl.c       | 80 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
> index 7c59824f43f5..91d2738bf0a0 100644
> --- a/include/linux/net_tstamp.h
> +++ b/include/linux/net_tstamp.h
> @@ -11,6 +11,8 @@
>   * @flags: see struct hwtstamp_config
>   * @tx_type: see struct hwtstamp_config
>   * @rx_filter: see struct hwtstamp_config
> + * @ifr: pointer to ifreq structure from the original IOCTL request
> + * @kernel_flags: possible flags defined by kernel_hwtstamp_flags below
>   *
>   * Prefer using this structure for in-kernel processing of hardware
>   * timestamping configuration, over the inextensible struct hwtstamp_config
> @@ -20,6 +22,13 @@ struct kernel_hwtstamp_config {
>  	int flags;
>  	int tx_type;
>  	int rx_filter;
> +	struct ifreq *ifr;
> +	int kernel_flags;
> +};
> +
> +/* possible values for kernel_hwtstamp_config->kernel_flags */
> +enum kernel_hwtstamp_flags {
> +	KERNEL_HWTSTAMP_FLAG_IFR_RESULT = BIT(0),
>  };

I think "kernel_flags" and KERNEL_HWTSTAMP_FLAG_IFR_RESULT are slightly
overengineered and poorly named (I couldn't understand yesterday what
they do, tried again today, finally did).

I believe that a single "bool copied_to_user" would do the trick just
fine and also be more self-explanatory?

>  
>  static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kernel_cfg,
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7160135ca540..42e96b12fd21 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3942,6 +3942,12 @@ int put_user_ifreq(struct ifreq *ifr, void __user *arg);
>  int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
>  		void __user *data, bool *need_copyout);
>  int dev_ifconf(struct net *net, struct ifconf __user *ifc);
> +int generic_hwtstamp_set_lower(struct net_device *dev,
> +			       struct kernel_hwtstamp_config *kernel_cfg,
> +			       struct netlink_ext_ack *extack);
> +int generic_hwtstamp_get_lower(struct net_device *dev,
> +			       struct kernel_hwtstamp_config *kernel_cfg,
> +			       struct netlink_ext_ack *extack);
>  int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *userdata);
>  unsigned int dev_get_flags(const struct net_device *);
>  int __dev_change_flags(struct net_device *dev, unsigned int flags,
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index a157b9ab5237..da1d2391822f 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -265,14 +265,17 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  	if (!netif_device_present(dev))
>  		return -ENODEV;
>  
> +	kernel_cfg.ifr = ifr;
>  	err = ops->ndo_hwtstamp_get(dev, &kernel_cfg, NULL);
>  	if (err)
>  		return err;
>  
> -	hwtstamp_config_from_kernel(&config, &kernel_cfg);
> +	if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) {
> +		hwtstamp_config_from_kernel(&config, &kernel_cfg);
>  
> -	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> -		return -EFAULT;
> +		if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> +			return -EFAULT;
> +	}
>  
>  	return 0;
>  }
> @@ -289,6 +292,7 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  		return -EFAULT;
>  
>  	hwtstamp_config_to_kernel(&kernel_cfg, &cfg);
> +	kernel_cfg.ifr = ifr;
>  
>  	err = net_hwtstamp_validate(&kernel_cfg);
>  	if (err)
> @@ -311,14 +315,78 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  	if (err)
>  		return err;
>  
> -	hwtstamp_config_from_kernel(&cfg, &kernel_cfg);
> +	if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) {
> +		hwtstamp_config_from_kernel(&cfg, &kernel_cfg);
>  
> -	if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
> -		return -EFAULT;
> +		if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)))
> +			return -EFAULT;
> +	}
>  
>  	return 0;
>  }
>  
> +int generic_hwtstamp_set_lower(struct net_device *dev,
> +			       struct kernel_hwtstamp_config *kernel_cfg,
> +			       struct netlink_ext_ack *extack)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct ifreq ifrr;
> +	int err;
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	if (ops->ndo_hwtstamp_set) {
> +		kernel_cfg->kernel_flags &= ~KERNEL_HWTSTAMP_FLAG_IFR_RESULT;

I don't think you ever need to unset this flag, just set it? The structure
is initialized in dev_get_hwtstamp() and dev_set_hwtstamp() as:

	struct kernel_hwtstamp_config kernel_cfg = {};

which zero-fills it.

> +		err = ops->ndo_hwtstamp_set(dev, kernel_cfg, extack);
> +		return err;

		return ops->ndo_hwtstamp_set(...)

i.e. skip the "err" assignment

> +	}
> +
> +	if (!kernel_cfg->ifr)
> +		return -EOPNOTSUPP;
> +
> +	strscpy_pad(ifrr.ifr_name, dev->name, IFNAMSIZ);
> +	ifrr.ifr_ifru = kernel_cfg->ifr->ifr_ifru;
> +	err = dev_eth_ioctl(dev, &ifrr, SIOCSHWTSTAMP);
> +	if (!err) {
> +		kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru;
> +		kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT;
> +	}
> +	return err;
> +}
> +EXPORT_SYMBOL(generic_hwtstamp_set_lower);

EXPORT_SYMBOL_GPL()? We expect this to be used by core network stack
(vlan, macvlan, bonding), not by vendor drivers.

Same comments for "set".