| 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
| ||
|
Message-ID: <CAP5jrPH__dJpGepM6Vs45PH+Pppx6KOVnUDS5f44DGeyseghfQ@mail.gmail.com>
Date: Wed, 5 Apr 2023 09:54:27 -0600
From: Max Georgiev <glipus@...il.com>
To: Vladimir Oltean <vladimir.oltean@....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 5, 2023 at 6:31 AM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> 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).
>
Sorry I missed you referred the exact function name in your previous comments.
Will be happy to rename.
> > +{
> > + 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"?
I've added kernel_cfg zero-initialization in [RFC PATCH v3 2/5].
But your comment makes sense.
Let me move the initialization to this patch.
>
> > + 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())
Thank you for pointing it out, I'll add the blank lines around these ifs.
>
> > + return 0;
> > }
> >
> > static int dev_siocbond(struct net_device *dev,
> > --
> > 2.39.2
> >
Vladimir, thank you for the feedback!
Powered by blists - more mailing lists