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: <CAP5jrPEmZY4eGVNw+WWcmn0FdN4wXsq0x=h-9aZgX3gJYyi6XQ@mail.gmail.com> Date: Thu, 6 Apr 2023 10:18:36 -0600 From: Max Georgiev <glipus@...il.com> To: Vladimir Oltean <vladimir.oltean@....com> Cc: Jakub Kicinski <kuba@...nel.org>, kory.maincent@...tlin.com, netdev@...r.kernel.org, maxime.chevallier@...tlin.com, vadim.fedorenko@...ux.dev, richardcochran@...il.com, gerhard@...leder-embedded.com Subject: Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path On Thu, Apr 6, 2023 at 9:02 AM Vladimir Oltean <vladimir.oltean@....com> wrote: > > On Thu, Apr 06, 2023 at 12:21:37AM -0600, Max Georgiev wrote: > > I tried my best to follow the discussion, and convert it to compilable code. > > Here is what I have in mind for generic_hwtstamp_get_lower(): > > > > int generic_hwtstamp_get_lower(struct net_dev *dev, > > struct kernel_hwtstamp_config *kernel_cfg, > > struct netlink_ext_ack *extack) > > { > > const struct net_device_ops *ops = dev->netdev_ops; > > struct hwtstamp_config cfg; > > int err; > > > > if (!netif_device_present(dev)) > > return -ENODEV; > > > > if (ops->ndo_hwtstamp_get) > > return ops->ndo_hwtstamp_get(dev, cfg, extack); > > > > if (!cfg->ifr) > > return -EOPNOTSUPP; > > > > err = dev_eth_ioctl(dev, cfg->ifr, SIOCGHWTSTAMP); > > if (err) > > return err; > > > > if (copy_from_user(&cfg, cfg->ifr->ifr_data, sizeof(cfg))) > > return -EFAULT; > > > > hwtstamp_config_to_kernel(kernel_cfg, &cfg); > > > > return 0; > > } > > Side note, it doesn't look like this code is particularly compilable > either - "cfg" is used in some places instead of "kernel_cfg". That's true, this code wouldn't compile. I copied it here to illustrate the potentially concerning logic. Thank you for pointing this out though! > > > > > It looks like there is a possibility that the returned hwtstamp_config structure > > will be copied twice to ifr and copied once from ifr on the return path > > in case if the underlying driver does not implement ndo_hwtstamp_get(): > > - the underlying driver calls copy_to_user() inside its ndo_eth_ioctl() > > implementation to return the data to generic_hwtstamp_get_lower(); > > - then generic_hwtstamp_get_lower() calls copy_from_user() to copy it > > back out of the ifr to kernel_hwtstamp_config structure; > > - then dev_get_hwtstamp() calls copy_to_user() again to update > > the same ifr with the same data the ifr already contains. > > > > Should we consider this acceptable? > > Thanks for laying this out. I guess with a table it's going to be > clearer, so to summarize, I believe this is the status: > > Assuming we convert *vlan to ndo_hwtstamp_set(): > > =================== > > If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev > driver uses ndo_eth_ioctl(), we have: > - one copy_from_user() in dev_set_hwtstamp() > - one copy_from_user() in the real_dev's ndo_eth_ioctl() > - one copy_to_user() in the real_dev's ndo_eth_ioctl() > - one copy_from_user() in generic_hwtstamp_get_lower() > - one copy_to_user() in dev_set_hwtstamp() > > If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev > is converted too, we have: > - one copy_from_user() in dev_set_hwtstamp() > - one copy_to_user() in dev_set_hwtstamp() > > =================== > > Assuming we don't convert *vlan to ndo_hwtstamp_set(): > > =================== > > If the vlan driver isn't converted to ndo_hwtstamp_set() and the > real_dev driver isn't converted either, we have: > - one copy_from_user() in dev_set_hwtstamp() > - one copy_from_user() in the real_dev's ndo_eth_ioctl() > - one copy_to_user() in the real_dev's ndo_eth_ioctl() > > If the vlan driver isn't converted to ndo_hwtstamp_set(), but the > real_dev driver is, we have: > - one copy_from_user() in dev_set_hwtstamp() > - one copy_from_user() in the vlan's ndo_eth_ioctl() > - one copy_to_user() in the vlan's ndo_eth_ioctl() > > =================== > > So between converting and not converting the *vlans to ndo_hwtstamp_set(), > the worst case is going to be worse (with a mix of new API in *vlan and > old API in real_dev) and the best case is going to be better (with new > API in both *vlan and real_dev). OTOH, with old API in *vlan, the number > of copies to/from the user buffer is going to be constant at 3, which is > not the best, not the worst. > > I guess the data indicates that we should convert the *vlans to > ndo_hwtstamp_set() at the very end of the process, and for now, just > make them compatible with a real_dev that uses the new API? > > Note that I haven't done the math for the "get" operation yet, but I > believe it to be similar. Thank you for putting this overhead tracking table together! Here is my guess on how this accounting would look like for the "get" codepath: Assuming we convert *vlan to ndo_hwtstamp_set(): =================== If the vlan driver is converted to ndo_hwtstamp_get() and the real_dev driver uses ndo_eth_ioctl(), we have: - one copy_to_user() in the real_dev's ndo_eth_ioctl() - one copy_from_user() in generic_hwtstamp_get_lower() - one copy_to_user() in dev_get_hwtstamp() If the vlan driver is converted to ndo_hwtstamp_get() and the real_dev is converted too, we have: - one copy_to_user() in dev_get_hwtstamp() =================== Assuming we don't convert *vlan to ndo_hwtstamp_get(): =================== If the vlan driver isn't converted to ndo_hwtstamp_get() and the real_dev driver isn't converted either, we have: - one copy_to_user() in the real_dev's ndo_eth_ioctl() If the vlan driver isn't converted to ndo_hwtstamp_get(), but the real_dev driver is, we have: - one copy_to_user() in the vlan's ndo_eth_ioctl() =================== Adding real_dev->ndo_hwtstamp_get/set() support to vlan_eth_ioctl() and holding back with ndo_hwtstamp_get/set() implementation in vlan code looks like a winner again. If I may, there are other ways to work around this inefficiency. Since kernel_hwtstamp_config was meant to be easily extendable, we can abuse it and add a flag field there. One of the flag values can indicate that the operation result structure was already copied to kernel_config->ifr by the function that received this kernel_config instance as a parameter, and that the content of the hwtstamp_config-related fields in kernel_config structure must be ignored when the function returns. It would complicate the implementation logic, but we'd avoid some unnecessary copy operations while converting *vlan components to the newer interface. Would it be a completely unreasonable approach?
Powered by blists - more mailing lists