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

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

[Maxim Georgiev]

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
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. 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@kernel.org>
Signed-off-by: Maxim Georgiev <glipus@gmail.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 33 ++++++++---------
 drivers/net/netdevsim/netdev.c             | 20 +++++++++++
 drivers/net/netdevsim/netdevsim.h          |  1 +
 include/linux/netdevice.h                  | 12 +++++++
 net/core/dev_ioctl.c                       | 42 +++++++++++++++++++---
 5 files changed, 85 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 6f5c16aebcbf..887be333349b 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6161,7 +6161,7 @@ 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
  *
  * Outgoing time stamping can be enabled and disabled. Play nice and
  * disable it when requested, although it shouldn't cause any overhead
@@ -6174,20 +6174,17 @@ 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 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 +6196,22 @@ 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;
 }
 
-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 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 +6221,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 +7358,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..c5c1835e3904 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -238,6 +238,24 @@ 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 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, struct hwtstamp_config *config)
+{
+	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 +274,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,
 };
 
 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 7621c512765f..aeab126baa22 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -57,6 +57,7 @@
 struct netpoll_info;
 struct device;
 struct ethtool_ops;
+struct hwtstamp_config;
 struct phy_device;
 struct dsa_port;
 struct ip_tunnel_parm;
@@ -1408,6 +1409,13 @@ 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);
+ *	Get hardware timestamping parameters currently configured  for NIC
+ *	device.
+ *	int (*ndo_hwtstamp_set)(struct net_device *dev,
+ *				struct hwtstamp_config *config);
+ *	Set hardware timestamping parameters for NIC device.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -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);
 };
 
 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;
+
+	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;
+	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);
 
 	/*
 	 *	Unknown or private ioctl
@@ -407,9 +443,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 ||
-- 
2.39.2

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

[Jakub Kicinski]

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)..

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

[Max Georgiev]

Jakub, thank you for taking a look!

On Thu, Mar 30, 2023 at 11:35 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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.

Would these two extra parameters be ignored by drivers in this initial
version of NDO hw timestamp API implementation?

>
> >  };
> >
> >  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

Will do. Thank you for pointing it out!

>
> > +
> > +     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

Will do!

>
> > +     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).

Got it, that would be a nice optimization for the NDO execution path!
We still will need a version of net_hwtstamp_validate(struct ifreq *ifr)
to do validation for drivers not implementing ndo_hwtstamp_set().
Also we'll need to implement validation for dsa_ndo_eth_ioctl() which
usually has an empty implementation, but can do something
meaningful depending on kernel configuration if I understand
it correctly. I'm not sure where to insert the validation code for
the DSA code path, would greatly appreciate some advice here.

>
> 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)..

I was on a fence about splitting dev_hwtstamp() into GET and SET versions.
If you believe separate implementations will provide a cleaner implementation
I'll be glad to split them.

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

[Jakub Kicinski]

On Fri, 31 Mar 2023 11:51:06 -0600 Max Georgiev wrote:
> On Thu, Mar 30, 2023 at 11:35 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > 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.  
> 
> Would these two extra parameters be ignored by drivers in this initial
> version of NDO hw timestamp API implementation?

Yup, and passed in as NULL.

See struct kernel_ethtool_coalesce for example of a kernel side
structure extending a fixed-size uAPI struct ethtool_coalesce.

So we would add a struct kernel_hwtstamp_config which would be 
empty for now, but we can make it not empty later.

Vladimir, does that sound reasonable or am I over-thinking?

> > > +     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).  
> 
> Got it, that would be a nice optimization for the NDO execution path!
> We still will need a version of net_hwtstamp_validate(struct ifreq *ifr)
> to do validation for drivers not implementing ndo_hwtstamp_set().
> Also we'll need to implement validation for dsa_ndo_eth_ioctl() which
> usually has an empty implementation, but can do something
> meaningful depending on kernel configuration if I understand
> it correctly. I'm not sure where to insert the validation code for
> the DSA code path, would greatly appreciate some advice here.

You can copy from user space onto stack at the start of the new
dev_set_hwtstamp(), make validation run on the already-copied
version, and then either proceed to call the NDO with the on-stack
config which was validated or the legacy and DSA path with ifr.

> > 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)..  
> 
> I was on a fence about splitting dev_hwtstamp() into GET and SET versions.
> If you believe separate implementations will provide a cleaner implementation
> I'll be glad to split them.

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

[Vladimir Oltean]

On Thu, Mar 30, 2023 at 10:35:19PM -0700, Jakub Kicinski wrote:
> >  	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).

I'm not sure I understand this. Since net_hwtstamp_validate() already
contains a copy_from_user() call, don't we already call copy_to_user()
twice (the second time being in all SIOCSHWTSTAMP handlers from drivers)?

Perhaps I don't understand what is it that can change the contents
of the ifreq structure, which would make this a potential issue for
ndo_hwtstamp_set() that isn't an issue for ndo_eth_ioctl()...

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

[Jakub Kicinski]

On Sat, 1 Apr 2023 19:08:29 +0300 Vladimir Oltean wrote:
> > 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).  
> 
> I'm not sure I understand this. Since net_hwtstamp_validate() already
> contains a copy_from_user() call, don't we already call copy_to_user()
> twice (the second time being in all SIOCSHWTSTAMP handlers from drivers)?

After this patch we'll be passing an in-kernel-space struct to drivers
rather than the ifr they have to copy themselves. I'm saying that we
should validate that exact copy, rather than copy, validate, copy, pass
to drivers, cause user space may change the values between the two
copies.

Unlikely to cause serious bugs but seems like a good code hygiene.

This is only for the drivers converted to the NDO, obviously, 
the legacy drivers will still have to copy themselves.

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

[Vladimir Oltean]

On Fri, Mar 31, 2023 at 11:10:41AM -0700, Jakub Kicinski wrote:
> On Fri, 31 Mar 2023 11:51:06 -0600 Max Georgiev wrote:
> > On Thu, Mar 30, 2023 at 11:35 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > 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.  
> > 
> > Would these two extra parameters be ignored by drivers in this initial
> > version of NDO hw timestamp API implementation?
> 
> Yup, and passed in as NULL.
> 
> See struct kernel_ethtool_coalesce for example of a kernel side
> structure extending a fixed-size uAPI struct ethtool_coalesce.
> 
> So we would add a struct kernel_hwtstamp_config which would be 
> empty for now, but we can make it not empty later.
> 
> Vladimir, does that sound reasonable or am I over-thinking?

So in principle I'm okay with the NULL extack (even though we could consider
doing something with the netlink message instead of letting it go to waste;
a suggestion may be to print the _msg to the kernel log, like store_bridge_parm()
does).

I missed the discussions around the creation of struct kernel_ethtool_coalesce,
but I imagine that ethtool_ops->set_coalesce() now takes both struct ethtool_coalesce,
as well as struct kernel_ethtool_coalesce, to avoid refactoring drivers even further
than just patching their function prototype? I don't think that argument would
apply here, for a completely new API?

I'm also okay, in principle, with having a struct kernel_hwtstamp_config, but
I have two problems with the way in which you've suggested it be implemented.
The first is not really only my problem, but rather, I don't think you
can have empty structures in C - I tried!

/* C doesn't allow empty structures, bah! */
struct sja1105_tas_data {
	u8 dummy;
};

The second is that I would actively dislike an ndo_hwtstamp_set() API
where half of the arguments are in one structure and half in the other.
I believe it's much easier, and cleaner, to make struct kernel_hwtstamp_config
duplicate the exact same fields from struct hwtstamp_config, and copy
those fields one by one from one structure to the other (to avoid issues
with UAPI field alignment mismatches). So we could pass only the extensible
kernel_hwtstamp_config to ndo_hwtstamp_set() and ndo_hwtstamp_get().

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

[Vladimir Oltean]

On Sat, Apr 01, 2023 at 10:55:33AM -0700, Jakub Kicinski wrote:
> On Sat, 1 Apr 2023 19:08:29 +0300 Vladimir Oltean wrote:
> > > 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).  
> > 
> > I'm not sure I understand this. Since net_hwtstamp_validate() already
> > contains a copy_from_user() call, don't we already call copy_to_user()
> > twice (the second time being in all SIOCSHWTSTAMP handlers from drivers)?
> 
> After this patch we'll be passing an in-kernel-space struct to drivers
> rather than the ifr they have to copy themselves. I'm saying that we
> should validate that exact copy, rather than copy, validate, copy, pass
> to drivers, cause user space may change the values between the two
> copies.
> 
> Unlikely to cause serious bugs but seems like a good code hygiene.
> 
> This is only for the drivers converted to the NDO, obviously, 
> the legacy drivers will still have to copy themselves.

Could you answer my second paragraph too, please?

| Perhaps I don't understand what is it that can change the contents
| of the ifreq structure, which would make this a potential issue for
| ndo_hwtstamp_set() that isn't an issue for ndo_eth_ioctl()...

I don't disagree with minimizing the number of copy_to_user() calls, but
I don't understand the ToCToU argument that you're bringing....

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

[Vladimir Oltean]

On Sat, Apr 01, 2023 at 09:20:58PM +0300, Vladimir Oltean wrote:
> I don't disagree with minimizing the number of copy_to_user() calls

from* user

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

[Vladimir Oltean]

On Fri, Mar 31, 2023 at 11:10:41AM -0700, Jakub Kicinski wrote:
> > We still will need a version of net_hwtstamp_validate(struct ifreq *ifr)
> > to do validation for drivers not implementing ndo_hwtstamp_set().
> > Also we'll need to implement validation for dsa_ndo_eth_ioctl() which
> > usually has an empty implementation, but can do something
> > meaningful depending on kernel configuration if I understand
> > it correctly. I'm not sure where to insert the validation code for
> > the DSA code path, would greatly appreciate some advice here.
> 
> You can copy from user space onto stack at the start of the new
> dev_set_hwtstamp(), make validation run on the already-copied
> version, and then either proceed to call the NDO with the on-stack
> config which was validated or the legacy and DSA path with ifr.

Actually, and here is the problem, DSA will want to see the timestamping
request with the new code path too, not just with the legacy one.
But, in this form, the dsa_ndo_eth_ioctl() -> dsa_master_ioctl() code
path wants to do one of two things: it either denies the configuration,
or passes it further, unchanged, to the master's netdev_ops->ndo_eth_ioctl().

By being written around the legacy ndo_eth_ioctl(), dsa_ndo_eth_ioctl()
places a requirement which conflicts with any attempt to convert any
kernel driver to the new API, because basically any net device can serve
as a DSA master, and simply put, DSA wants to see timestamping requests
to the DSA master, old or new API.

The only "useful" piece of logic from dsa_master_ioctl() is to deny the
hwtstamp_set operation in some cases, so it's clear that it's useless
for dsa_master_ioctl() to have to call the master's netdev_ops->ndo_eth_ioctl()
when dev_eth_ioctl() already would have done it anyway.

I can make dsa_ndo_eth_ioctl() disappear and replace it with a netdev
notifier as per this patch:
https://lore.kernel.org/netdev/20220317225035.3475538-1-vladimir.oltean@nxp.com/

My understanding of Jakub's objection is that the scope of the
NETDEV_ETH_IOCTL is too wide, and as such, it would need to change to
something like NETDEV_HWTSTAMP_SET. I can make that change if that is
the only objection, and resubmit that as preparation work for the
ndo_hwtstamp_set() effort.

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

[Jakub Kicinski]

On Sat, 1 Apr 2023 21:20:58 +0300 Vladimir Oltean wrote:
> > After this patch we'll be passing an in-kernel-space struct to drivers
> > rather than the ifr they have to copy themselves. I'm saying that we
> > should validate that exact copy, rather than copy, validate, copy, pass
> > to drivers, cause user space may change the values between the two
> > copies.
> > 
> > Unlikely to cause serious bugs but seems like a good code hygiene.
> > 
> > This is only for the drivers converted to the NDO, obviously, 
> > the legacy drivers will still have to copy themselves.  
> 
> Could you answer my second paragraph too, please?
> 
> | Perhaps I don't understand what is it that can change the contents
> | of the ifreq structure, which would make this a potential issue for
> | ndo_hwtstamp_set() that isn't an issue for ndo_eth_ioctl()...
> 
> I don't disagree with minimizing the number of copy_to_user() calls, but
> I don't understand the ToCToU argument that you're bringing....

As I said, just code hygiene, I haven't looked at the drivers.
Seems too obvious to invest time trying to come up with an exact
scenario.

Do you see a reason not to code this the right way?

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

[Vladimir Oltean]

On Sat, Apr 01, 2023 at 12:14:51PM -0700, Jakub Kicinski wrote:
> Do you see a reason not to code this the right way?

No. I didn't understand the justification you brought, and asked for
clarifications.

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

[Jakub Kicinski]

On Sat, 1 Apr 2023 22:12:15 +0300 Vladimir Oltean wrote:
> Actually, and here is the problem, DSA will want to see the timestamping
> request with the new code path too, not just with the legacy one.
> But, in this form, the dsa_ndo_eth_ioctl() -> dsa_master_ioctl() code
> path wants to do one of two things: it either denies the configuration,
> or passes it further, unchanged, to the master's netdev_ops->ndo_eth_ioctl().
> 
> By being written around the legacy ndo_eth_ioctl(), dsa_ndo_eth_ioctl()
> places a requirement which conflicts with any attempt to convert any
> kernel driver to the new API, because basically any net device can serve
> as a DSA master, and simply put, DSA wants to see timestamping requests
> to the DSA master, old or new API.
> 
> The only "useful" piece of logic from dsa_master_ioctl() is to deny the
> hwtstamp_set operation in some cases, so it's clear that it's useless
> for dsa_master_ioctl() to have to call the master's netdev_ops->ndo_eth_ioctl()
> when dev_eth_ioctl() already would have done it anyway.

So the current patch can only convert drivers which can't be a DSA
master :( (realistically any big iron NIC for example)
It should be relatively easy to plumb both the ifr and the in-kernel
config thru all the DSA APIs and have it call the right helper, too,
tho? SMOC?

> I can make dsa_ndo_eth_ioctl() disappear and replace it with a netdev
> notifier as per this patch:
> https://lore.kernel.org/netdev/20220317225035.3475538-1-vladimir.oltean@nxp.com/
> 
> My understanding of Jakub's objection is that the scope of the
> NETDEV_ETH_IOCTL is too wide, and as such, it would need to change to
> something like NETDEV_HWTSTAMP_SET. I can make that change if that is
> the only objection, and resubmit that as preparation work for the
> ndo_hwtstamp_set() effort.

My objection to the IOCTL is that there's a lot of boilerplate that 
the drivers have to copy and that it makes it harder to do meaningful
work in the core.

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

[Vladimir Oltean]

On Sat, Apr 01, 2023 at 12:24:50PM -0700, Jakub Kicinski wrote:
> On Sat, 1 Apr 2023 22:12:15 +0300 Vladimir Oltean wrote:
> > My understanding of Jakub's objection is that the scope of the
> > NETDEV_ETH_IOCTL is too wide, and as such, it would need to change to
> > something like NETDEV_HWTSTAMP_SET. I can make that change if that is
> > the only objection, and resubmit that as preparation work for the
> > ndo_hwtstamp_set() effort.
> 
> My objection to the IOCTL is that there's a lot of boilerplate that 
> the drivers have to copy and that it makes it harder to do meaningful
> work in the core.

By "NETDEV_ETH_IOCTL" I mean a netdev notifier with this name, and so,
I was expecting objections to that...

I can change that notifier proposal to be scoped only to the hwtstamping
operation, and that notifier would be 100% orthogonal to which API is
used to get/set hwtstamping, ndo_eth_ioctl() or ndo_hwtstamp_set().

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

[Vladimir Oltean]

On Sat, Apr 01, 2023 at 12:24:50PM -0700, Jakub Kicinski wrote:
> It should be relatively easy to plumb both the ifr and the in-kernel
> config thru all the DSA APIs and have it call the right helper, too,
> tho? SMOC?

Sorry, this does not compute.

"Plumbing the in-kernel config thru all the DSA APIs" would be the
netdev notifier that I proposed one year ago. I just need you to say
"yes, sounds ok, let's see how that looks with last year's feedback
addressed".

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

[Max Georgiev]

On Sat, Apr 1, 2023 at 2:18 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Sat, Apr 01, 2023 at 12:24:50PM -0700, Jakub Kicinski wrote:
> > It should be relatively easy to plumb both the ifr and the in-kernel
> > config thru all the DSA APIs and have it call the right helper, too,
> > tho? SMOC?
>
> Sorry, this does not compute.
>
> "Plumbing the in-kernel config thru all the DSA APIs" would be the
> netdev notifier that I proposed one year ago. I just need you to say
> "yes, sounds ok, let's see how that looks with last year's feedback
> addressed".

I sent out a second iteration of the ndo_hwtstamp_get/set patch.
I tried to address all the comments except fixing DSA API - I still
don't have a full understanding of what is the plan there.

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

[Vladimir Oltean]

On Sun, Apr 02, 2023 at 08:28:08AM -0600, Max Georgiev wrote:
> On Sat, Apr 1, 2023 at 2:18 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > On Sat, Apr 01, 2023 at 12:24:50PM -0700, Jakub Kicinski wrote:
> > > It should be relatively easy to plumb both the ifr and the in-kernel
> > > config thru all the DSA APIs and have it call the right helper, too,
> > > tho? SMOC?
> >
> > Sorry, this does not compute.
> >
> > "Plumbing the in-kernel config thru all the DSA APIs" would be the
> > netdev notifier that I proposed one year ago. I just need you to say
> > "yes, sounds ok, let's see how that looks with last year's feedback
> > addressed".
> 
> I sent out a second iteration of the ndo_hwtstamp_get/set patch.
> I tried to address all the comments except fixing DSA API - I still
> don't have a full understanding of what is the plan there.

Well, my plan is for this patch set (to which you were copied before
sending out your v2) to be merged, then you could see what remains to be
done on top of that, and that should be easy-peasy:
https://patchwork.kernel.org/project/netdevbpf/cover/20230402123755.2592507-1-vladimir.oltean@nxp.com/

I think that patch set explains quite clearly what's with DSA's API and
what I intend it to become.