Re: [PATCH net-next RFC] Add NDOs for hardware timestamp get/set

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 30 Mar 2023 22:56:19 -0600 Maxim Georgiev wrote:
> @@ -1642,6 +1650,10 @@ 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);
> +	int			(*ndo_hwtstamp_set)(struct net_device *dev,
> +						    struct hwtstamp_config *config);

I wonder if we should pass in 

	struct netlink_ext_ack *extack

and maybe another structure for future extensions?
So we don't have to change the drivers again when we extend uAPI.

>  };
>  
>  struct xdp_metadata_ops {
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 5cdbfbf9a7dc..c90fac9a9b2e 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -277,6 +277,39 @@ static int dev_siocbond(struct net_device *dev,
>  	return -EOPNOTSUPP;
>  }
>  
> +static int dev_hwtstamp(struct net_device *dev, struct ifreq *ifr,
> +			unsigned int cmd)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	int err;
> +	struct hwtstamp_config config;

nit: reorder int err after config we like lines longest to shortest

> +
> +	if ((cmd == SIOCGHWTSTAMP && !ops->ndo_hwtstamp_get) ||
> +	    (cmd == SIOCSHWTSTAMP && !ops->ndo_hwtstamp_set))
> +		return dev_eth_ioctl(dev, ifr, cmd);
> +
> +	err = dsa_ndo_eth_ioctl(dev, ifr, cmd);
> +	if (err == 0 || err != -EOPNOTSUPP)
> +		return err;
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	if (cmd == SIOCSHWTSTAMP) {
> +		if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> +			err = -EFAULT;
> +		else
> +			err = ops->ndo_hwtstamp_set(dev, &config);
> +	} else if (cmd == SIOCGHWTSTAMP) {
> +		err = ops->ndo_hwtstamp_get(dev, &config);
> +	}
> +
> +	if (err == 0)
> +		err = copy_to_user(ifr->ifr_data, &config,
> +				   sizeof(config)) ? -EFAULT : 0;

nit: just error check each return value, don't try to save LoC

> +	return err;
> +}
> +
>  static int dev_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
>  			      void __user *data, unsigned int cmd)
>  {
> @@ -391,11 +424,14 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
>  		rtnl_lock();
>  		return err;
>  
> +	case SIOCGHWTSTAMP:
> +		return dev_hwtstamp(dev, ifr, cmd);
> +
>  	case SIOCSHWTSTAMP:
>  		err = net_hwtstamp_validate(ifr);
>  		if (err)
>  			return err;
> -		fallthrough;
> +		return dev_hwtstamp(dev, ifr, cmd);

Let's refactor this differently, we need net_hwtstamp_validate()
to run on the same in-kernel copy as we'll send down to the driver.
If we copy_from_user() twice we may validate a different thing
than the driver will end up seeing (ToCToU).

TBH I'm not sure if keeping GET and SET in a common dev_hwtstamp()
ends up being beneficial. If we fold in the validation check half 
of the code will be under and if (GET) or if (SET)..