netdev - Re: [PATCH net-next 7/7] net: create a netdev notifier for DSA to reject PTP on DSA master

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]

Date:   Sun, 2 Apr 2023 06:01:25 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
        Maxim Georgiev <glipus@...il.com>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Köry Maincent <kory.maincent@...tlin.com>,
        Maxime Chevallier <maxime.chevallier@...tlin.com>
Subject: Re: [PATCH net-next 7/7] net: create a netdev notifier for DSA to
 reject PTP on DSA master



On 4/2/2023 5:37 AM, Vladimir Oltean wrote:
> The fact that PTP 2-step TX timestamping is broken on DSA switches if
> the master also timestamps the same packets is documented by commit
> f685e609a301 ("net: dsa: Deny PTP on master if switch supports it").
> We attempt to help the users avoid shooting themselves in the foot by
> making DSA reject the timestamping ioctls on an interface that is a DSA
> master, and the switch tree beneath it contains switches which are aware
> of PTP.
> 
> The only problem is that there isn't an established way of intercepting
> ndo_eth_ioctl calls, so DSA creates avoidable burden upon the network
> stack by creating a struct dsa_netdevice_ops with overlaid function
> pointers that are manually checked from the relevant call sites. There
> used to be 2 such dsa_netdevice_ops, but now, ndo_eth_ioctl is the only
> one left.
> 
> There is an ongoing effort to migrate driver-visible hardware timestamping
> control from the ndo_eth_ioctl() based API to a new ndo_hwtstamp_set()
> model, but DSA actively prevents that migration, since dsa_master_ioctl()
> is currently coded to manually call the master's legacy ndo_eth_ioctl(),
> and so, whenever a network device driver would be converted to the new
> API, DSA's restrictions would be circumvented, because any device could
> be used as a DSA master.
> 
> The established way for unrelated modules to react on a net device event
> is via netdevice notifiers. So we create a new notifier which gets
> called whenever there is an attempt to change hardware timestamping
> settings on a device.
> 
> Finally, there is another reason why a netdev notifier will be a good
> idea, besides strictly DSA, and this has to do with PHY timestamping.
> 
> With ndo_eth_ioctl(), all MAC drivers must manually call
> phy_has_hwtstamp() before deciding whether to act upon SIOCSHWTSTAMP,
> otherwise they must pass this ioctl to the PHY driver via
> phy_mii_ioctl().
> 
> With the new ndo_hwtstamp_set() API, it will be desirable to simply not
> make any calls into the MAC device driver when timestamping should be
> performed at the PHY level.
> 
> But there exist drivers, such as the lan966x switch, which need to
> install packet traps for PTP regardless of whether they are the layer
> that provides the hardware timestamps, or the PHY is. That would be
> impossible to support with the new API.
> 
> The proposal there, too, is to introduce a netdev notifier which acts as
> a better cue for switching drivers to add or remove PTP packet traps,
> than ndo_hwtstamp_set(). The one introduced here "almost" works there as
> well, except for the fact that packet traps should only be installed if
> the PHY driver succeeded to enable hardware timestamping, whereas here,
> we need to deny hardware timestamping on the DSA master before it
> actually gets enabled. This is why this notifier is called "PRE_", and
> the notifier that would get used for PHY timestamping and packet traps
> would be called NETDEV_CHANGE_HWTSTAMP. This isn't a new concept, for
> example NETDEV_CHANGEUPPER and NETDEV_PRECHANGEUPPER do the same thing.
> 
> In expectation of future netlink UAPI, we also pass a non-NULL extack
> pointer to the netdev notifier, and we make DSA populate it with an
> informative reason for the rejection. To avoid making it go to waste, we
> make the ioctl-based dev_set_hwtstamp() create a fake extack and print
> the message to the kernel log.
> 
> Link: https://lore.kernel.org/netdev/20230401191215.tvveoi3lkawgg6g4@skbuf/
> Link: https://lore.kernel.org/netdev/20230310164451.ls7bbs6pdzs4m6pw@skbuf/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian