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: <CAP5jrPGzrzMYmBBT+B6U5Oh6v_Tcie1rj0KqsWOEZOBR7JBoXA@mail.gmail.com> Date: Thu, 6 Apr 2023 00:21:37 -0600 From: Max Georgiev <glipus@...il.com> To: Jakub Kicinski <kuba@...nel.org> Cc: Vladimir Oltean <vladimir.oltean@....com>, 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 Wed, Apr 5, 2023 at 6:00 PM Jakub Kicinski <kuba@...nel.org> wrote: > > On Wed, 5 Apr 2023 21:01:21 +0300 Vladimir Oltean wrote: > > - bonding is also DSA master when it has a DSA master as lower, so the > > DSA master restriction has already run once - on the bonding device > > itself > > Huh, didn't know that. > > > > The latter could be used for the first descend as well I'd presume. > > > And it can be exported for the use of more complex drivers like > > > bonding which want to walk the lowers themselves. > > > > > > > - it requires cfg.flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX to be set in > > > > SET requests > > > > > > > > - it sets cfg.flags | HWTSTAMP_FLAG_BONDED_PHC_INDEX in GET responses > > > > > > IIRC that was to indicate to user space that the real PHC may change > > > for this netdev so it needs to pay attention to netlink notifications. > > > Shouldn't apply to *vlans? > > > > No, this shouldn't apply to *vlans, but I didn't suggest that it should. > > Good, so if we just target *vlans we don't have to worry. > > > I don't think my proposal was clear enough, so here's some code > > (untested, written in email client). > > > > static int macvlan_hwtstamp_get(struct net_device *dev, > > struct kernel_hwtstamp_config *cfg, > > struct netlink_ext_ack *extack) > > { > > struct net_device *real_dev = macvlan_dev_real_dev(dev); > > > > return generic_hwtstamp_get_lower(real_dev, cfg, extack); > > } > > > > static int macvlan_hwtstamp_set(struct net_device *dev, > > struct kernel_hwtstamp_config *cfg, > > struct netlink_ext_ack *extack) > > { > > struct net_device *real_dev = macvlan_dev_real_dev(dev); > > > > return generic_hwtstamp_set_lower(real_dev, cfg, extack); > > } > > > > static int vlan_hwtstamp_get(struct net_device *dev, > > struct kernel_hwtstamp_config *cfg, > > struct netlink_ext_ack *extack) > > { > > struct net_device *real_dev = vlan_dev_priv(dev)->real_dev; > > > > return generic_hwtstamp_get_lower(real_dev, cfg, extack); > > } > > > > static int vlan_hwtstamp_set(struct net_device *dev, > > struct kernel_hwtstamp_config *cfg, > > struct netlink_ext_ack *extack) > > { > > struct net_device *real_dev = vlan_dev_priv(dev)->real_dev; > > > > return generic_hwtstamp_set_lower(real_dev, cfg, extack); > > } > > I got that, but why wouldn't this not be better, as it avoids > the 3 driver stubs? (also written in the MUA) > > int net_lower_hwtstamp_set(struct net_device *dev, > struct kernel_hwtstamp_config *cfg, > struct netlink_ext_ack *extack) > { > struct list_head *iter = dev->adj_list.lower.next; > struct net_device *lower; > > lower = netdev_lower_get_next(dev, &iter); > return generic_hwtstamp_set_lower(lower, cfg, extack); > } > > > static int bond_hwtstamp_get(struct net_device *bond_dev, > > struct kernel_hwtstamp_config *cfg, > > struct netlink_ext_ack *extack) > > { > > struct bonding *bond = netdev_priv(bond_dev); > > struct net_device *real_dev = bond_option_active_slave_get_rcu(bond); > > int err; > > > > if (!real_dev) > > return -EOPNOTSUPP; > > > > err = generic_hwtstamp_get_lower(real_dev, cfg, extack); > > if (err) > > return err; > > > > /* Set the BOND_PHC_INDEX flag to notify user space */ > > cfg->flags |= HWTSTAMP_FLAG_BONDED_PHC_INDEX; > > > > return 0; > > } > > > > static int bond_hwtstamp_set(struct net_device *bond_dev, > > struct kernel_hwtstamp_config *cfg, > > struct netlink_ext_ack *extack) > > { > > struct bonding *bond = netdev_priv(bond_dev); > > struct net_device *real_dev = bond_option_active_slave_get_rcu(bond); > > int err; > > > > if (!real_dev) > > return -EOPNOTSUPP; > > > > if (!(cfg->flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX)) > > return -EOPNOTSUPP; > > > > return generic_hwtstamp_set_lower(real_dev, cfg, extack); > > } > > > > Doesn't seem in any way necessary to complicate things with the netdev > > adjacence lists? > > What is the complication? We can add a "get first" helper maybe to hide > the oddities of the linking. > > > > Yes, user space must be involved anyway, because the entire clock will > > > change. IMHO implementing the pass thru for timestamping requests on > > > bonding is checkbox engineering, kernel can't make it work > > > transparently. But nobody else spoke up when it was proposed so... > > > > ok, but that's a bit beside the point here. > > You cut off the quote it was responding to so IDK if it is. 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; } 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?
Powered by blists - more mailing lists