netdev - Re: [PATCH net-next v6 6/7] net: selftests: Export net_test_phy_loopback

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: <464ec7ed-2943-4696-a198-2495d4035f91@lunn.ch>
Date: Wed, 12 Feb 2025 23:34:14 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Gerhard Engleder <gerhard@...leder-embedded.com>
Cc: hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH net-next v6 6/7] net: selftests: Export
 net_test_phy_loopback_*

On Wed, Feb 12, 2025 at 10:36:00PM +0100, Gerhard Engleder wrote:
> On 12.02.25 21:46, Andrew Lunn wrote:
> > > Reusing the complete test set as extension is not feasible as the first
> > > test requires an existing link and for loopback test no link is
> > > necessary. But yes, I can do better and rework it to reusable tests.
> > > I'm not sure if I will use ethtool_test_flags as IMO ideally all tests
> > > run always to ensure easy use.
> > 
> > We try to ensure backwards/forwards compatibly between ethtool and the
> > kernel.
> > 
> > The point about ethtool_test_flags is that for older versions of
> > ethtool, you have no idea what the reserved field contains. Has it
> > always been set to zero? If there is a flag indicating reserved
> > contains a value, you then know it is safe to use it.
> 
> I'm not sure if I understand the last sentence. Do you mean it is safe
> to use a flag that was previously marked as reserved if the clients did
> set it to zero, but for ethtool_test_flags this is not the case?

We have:

struct ethtool_test {
	__u32	cmd;
	__u32	flags;
	__u32	reserved;
	__u32	len;
	__u64	data[];
};

where flags takes a bitmap from:

enum ethtool_test_flags {
	ETH_TEST_FL_OFFLINE	= (1 << 0),
	ETH_TEST_FL_FAILED	= (1 << 1),
	ETH_TEST_FL_EXTERNAL_LB	= (1 << 2),
	ETH_TEST_FL_EXTERNAL_LB_DONE	= (1 << 3),
};

I've not looked at ethtool, but it is possible the struct ethtool_test
is on the stack, or the heap, and not zeroed. So reserved contains
random junk. flags is however given a value, bad things would happen
otherwise. So we know the bits which are currently unused should be
zero. A new flag can be added, which indicates user space wants to set
the speed, and that the speed is in the reserved field. Without the
flag being set, reserved contains random junk, with it set, we know we
have a version of ethtool which sets it to a specific value.

It might however be that we cannot rename reserved, it is now part of
the kAPI, and changing it to another name would break the compilation
of something. That is one of the advantages of netlink API, you can
add new attributes without having to worry about breaking older
builds. Unfortunately, the self test is still using the IOCTL, it has
not been converted to netlink.

    Andrew