Re: [PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

[Heiner Kallweit <hkallweit1@gmail.com>]

On 15.02.2024 21:27, Heiner Kallweit wrote:
> On 15.02.2024 16:53, Russell King (Oracle) wrote:
>> On Thu, Feb 15, 2024 at 02:05:54PM +0100, Heiner Kallweit wrote:
>>> Let the core check whether userspace returned unsupported modes in the
>>> EEE advertisement bitmap. This allows to remove these checks from
>>> drivers.
>>
>> Why is this a good thing to implement?
>>
> Because it allows to remove all the duplicated checks from drivers.
> 
>> Concerns:
>> 1) This is a change of behaviour for those drivers that do not
>> implement this behaviour.
>>
> Not of regular behavior. And at least for all drivers using phylib
> it's no change.
> 
>> 2) This behaviour is different from ksettings_set() which silently
>> trims the advertisement down to the modes that are supported
>>
> It's the same check that we have in genphy_c45_ethtool_set_eee().
> So it's in line with what we do in phylib.
> But I would also be fine with silently trimming the advertisement.
> 
>> 3) This check is broken. Userspace is at liberty to pass in ~0 for
>> the supported mask and the advertising mask which subverts this
>> check.
>>
> ethtool retrieves the supported mask with get_eee() from kernel.
> And this (unmodified) value is passed with set_eee().
> So at least with ethtool this scenario can't occur.
> 
In addition: In the netlink case the supported mask isn't even
transferred to userspace for the set_eee operation.

>> So... I think overall, it's a NAK to this from me - I don't think
>> it's something that anyone should implement. Restricting the
>> advertisement to the modes that are supported (where the supported
>> mask is pulled from the network driver and not userspace) would
>> be acceptable, but is that actually necessary?
>>
>