| 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