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: <20230405170010.1c989a8f@kernel.org> Date: Wed, 5 Apr 2023 17:00:10 -0700 From: Jakub Kicinski <kuba@...nel.org> To: Vladimir Oltean <vladimir.oltean@....com> Cc: Maxim Georgiev <glipus@...il.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, 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.
Powered by blists - more mailing lists