| 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