netdev - Re: [RFC PATCH net-next 3/6] net: ethtool: let the core choose RSS context IDs

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: <ecd752db-ff2a-6948-2ff8-531343f80696@gmail.com>
Date:   Tue, 4 Apr 2023 13:14:39 +0100
From:   Edward Cree <ecree.xilinx@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>, edward.cree@....com
Cc:     linux-net-drivers@....com, davem@...emloft.net, pabeni@...hat.com,
        edumazet@...gle.com, netdev@...r.kernel.org,
        habetsm.xilinx@...il.com, sudheer.mogilappagari@...el.com
Subject: Re: [RFC PATCH net-next 3/6] net: ethtool: let the core choose RSS
 context IDs

On 03/04/2023 22:54, Jakub Kicinski wrote:
> On Mon, 3 Apr 2023 17:33:00 +0100 edward.cree@....com wrote:
>>  	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
>>  				    const u8 *key, const u8 hfunc,
>> -				    u32 *rss_context, bool delete);
>> +				    u32 rss_context, bool delete);
> 
> Would it be easier to pass struct ethtool_rxfh_context instead of
> doing it field by field?  Otherwise Intel will need to add more
> arguments and touch all drivers. Or are you thinking that they should
> use a separate callback for the "RR RSS" or whatever their thing is?

Initially I tried to just pass in ctx with the new values already
 filled in.  But that breaks if the op fails; we have to leave the
 old values in ctx.  We maybe could create a second, ephemeral
 struct ethtool_rxfh_context to pass the new values in, but then
 we have to worry about which one's priv the driver uses.
(We can't e.g. just pass in the ephemeral one, and copy its priv
 across when we update the real ctx after the op returns, because
 what if the driver stores, say, a list_head in its priv?)

And if we did pass a struct wrapping indir, key and hfunc, then
 any patch adding more fields to it would need existing drivers
 to check the new fields were unused / set to NO_CHANGE.

So I think we just have to accept that new fields will mean
 changing all drivers.  (There's only half a dozen, anyway.)
And doing that through the op arguments means the compiler will
 catch any driver that hasn't been updated, rather than the
 driver potentially silently ignoring the new field.

> And maybe separate op for create / change / delete?

Good idea, that would also elide renaming the legacy op.

> And an extack on top... :)

Sure.