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

[Jakub Kicinski <kuba@kernel.org>]

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.