netdev - Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping layer be selectable.

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: <20230502130525.02ade4a8@kmaincent-XPS-13-7390>
Date:   Tue, 2 May 2023 13:05:25 +0200
From:   Köry Maincent <kory.maincent@...tlin.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, glipus@...il.com,
        maxime.chevallier@...tlin.com, vadim.fedorenko@...ux.dev,
        richardcochran@...il.com, gerhard@...leder-embedded.com,
        thomas.petazzoni@...tlin.com, krzysztof.kozlowski+dt@...aro.org,
        robh+dt@...nel.org, linux@...linux.org.uk
Subject: Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping
 layer be selectable.

On Sat, 29 Apr 2023 20:58:07 +0300
Vladimir Oltean <vladimir.oltean@....com> wrote:

Thanks for the review and sorry for the coding style issues, I forgot to run the
checkpatch script.

> >  
> >  #if IS_ENABLED(CONFIG_MACSEC)
> > @@ -2879,6 +2885,7 @@ enum netdev_cmd {
> >  	NETDEV_OFFLOAD_XSTATS_REPORT_DELTA,
> >  	NETDEV_XDP_FEAT_CHANGE,
> >  	NETDEV_PRE_CHANGE_HWTSTAMP,
> > +	NETDEV_CHANGE_HWTSTAMP,  
> 
> Don't create new netdev notifiers with no users. Also,
> NETDEV_PRE_CHANGE_HWTSTAMP has disappered.

Ok, right you move it on to dsa stub. What do you think of our case, should we
continue with netdev notifier? 

> > diff --git a/include/uapi/linux/ethtool_netlink.h
> > b/include/uapi/linux/ethtool_netlink.h index 447908393b91..4f03e7cde271
> > 100644 --- a/include/uapi/linux/ethtool_netlink.h
> > +++ b/include/uapi/linux/ethtool_netlink.h
> > @@ -41,6 +41,7 @@ enum {
> >  	ETHTOOL_MSG_TSINFO_GET,
> >  	ETHTOOL_MSG_TSLIST_GET,
> >  	ETHTOOL_MSG_TS_GET,
> > +	ETHTOOL_MSG_TS_SET,  
> 
> The way in which the Linux kernel ensures a stable API towards user
> space is that programs written against kernel headers from 5 years ago
> still work with kernels from today. In your case, you would be breaking
> that, because before this patch, ETHTOOL_MSG_CABLE_TEST_ACT was 26, and
> after your patch it is 27. So old applications emitting a cable test
> netlink message would be interpreted by new kernels as emitting a "set
> timestamping layer" netlink message. Of course not only the cable test
> is affected, every other netlink message until the end is now shifted by
> 1. This is why we put new enum values only at the very end, where it
> actually says they should go:

Sorry for that, this indeed didn't cross my head. I will fix it.

> >  
> > +/* TS_SET */
> > +const struct nla_policy ethnl_ts_set_policy[] = {
> > +	[ETHTOOL_A_TS_HEADER]	=
> > NLA_POLICY_NESTED(ethnl_header_policy),
> > +	[ETHTOOL_A_TS_LAYER]	= { .type = NLA_U32 },
> > +};  
> 
> I wanted to explore this topic myself, but I can't seem to find the
> time, so here's something a bit hand-wavey.
> 
> We should make a distinction between what the kernel exposes as UAPI
> (our future selves need to work with that in a backwards-compatible way)
> and the internal, unstable kernel implementation.
> 
> It would be good if, instead of the ETHTOOL_A_TS_LAYER netlink
> attribute, the kernel could expose a more generic ETHTOOL_A_TS_PHC, from
> which the ethtool core could deduce if the PHC number belongs to the MAC
> or to the PHY. We could still keep something like
> netdev->selected_timestamping_layer in code private to the kernel, but from
> the UAPI perspective, I agree with Andrew that we should design something
> that is cleanly extensible to N PHCs, not just to a vague "layer".

Just some thought:
Maybe we could create a PHC_ID 0xXY where X is the layer and Y the PHCs number
in this layer, but what will append if there is two MACs consecutively?
Also in case of several MACs or PHYs in serial
netdev->selected_timestamping_layer is inappropriate. 

Maybe using it like 0xABC where A is the MAC number, B the PHY number and C
the PHC number in the device.
For example if we select the second MAC and its third PHC, PHC_ID will be
0x203. Or if we select the second PHC of the PHY it will be 0x012.