netdev - Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230405180121.cefhbjlejuisywhk@skbuf>
Date:   Wed, 5 Apr 2023 21:01:21 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
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, Apr 05, 2023 at 10:42:53AM -0700, Jakub Kicinski wrote:
> On Wed, 5 Apr 2023 20:28:40 +0300 Vladimir Oltean wrote:
> > So what do you suggest doing with bonding, then? Not use the generic
> > helper at all?
> 
> It'd seem most natural to me to split the generic "descend" helper into
> two functions, one which retrieves the lower and one which does the
> appropriate calling dance (ndo vs ioctl, and DSA, which I guess is now
> gone?).

There's nothing DSA-related to be done. DSA masters can't be lowers of
any other virtual interface kinds except bridge or bonding/team, and:
- bridge doesn't support hwtstamping
- 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

> 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.
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);
}

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?

> 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.