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: <20230511093018.xjeo2rep2pp5st3b@skbuf> Date: Thu, 11 May 2023 12:30:18 +0300 From: Vladimir Oltean <vladimir.oltean@....com> To: Maxim Georgiev <glipus@...il.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, liuhangbin@...il.com Subject: Re: [RFC PATCH net-next v6 2/5] net: Add ifreq pointer field to kernel_hwtstamp_config structure 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@...nel.org> > Signed-off-by: Maxim Georgiev <glipus@...il.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".
Powered by blists - more mailing lists