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  PHC 
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 12 May 2020 19:37:01 -0700
From:   Doug Berger <>
To:     Michal Kubecek <>,
Cc:     Andrew Lunn <>,
        "David S. Miller" <>,
        Florian Fainelli <>,
        Heiner Kallweit <>,
        Russell King <>,,
Subject: Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg

On 5/12/2020 12:08 PM, Michal Kubecek wrote:
> On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
>> On 5/11/2020 5:47 PM, Andrew Lunn wrote:
>>> On Mon, May 11, 2020 at 05:24:07PM -0700, Doug Berger wrote:
>>>> A comment in uapi/linux/ethtool.h states "Drivers should reject a
>>>> non-zero setting of @autoneg when autoneogotiation is disabled (or
>>>> not supported) for the link".
>>>> That check should be added to phy_validate_pause() to consolidate
>>>> the code where possible.
>>>> Fixes: 22b7d29926b5 ("net: ethernet: Add helper to determine if pause configuration is supported")
>>> Hi Doug
>>> If this is a real fix, please submit this to net, not net-next.
>>>    Andrew
>> This was intended as a fix, but I thought it would be better to keep it
>> as part of this set for context and since net-next is currently open.
>> The context is trying to improve the phylib support for offloading
>> ethtool pause configuration and this is something that could be checked
>> in a single location rather than by individual drivers.
>> I included it here to get feedback about its appropriateness as a common
>> behavior. I should have been more explicit about that.
>> Personally, I'm actually not that fond of this change since it can
>> easily be a source of confusion with the ethtool interface because the
>> link autonegotiation and the pause autonegotiation are controlled by
>> different commands.
>> Since the ethtool -A command performs a read/modify/write of pause
>> parameters, you can get strange results like these:
>> # ethtool -s eth0 speed 100 duplex full autoneg off
>> # ethtool -A eth0 tx off
>> Cannot set device pause parameters: Invalid argument
>> #
>> Because, the get read pause autoneg as enabled and only the tx_pause
>> member of the structure was updated.
> This would be indeed unfortunate. We could use extack to make the error
> message easier to understand but the real problem IMHO is that
> ethtool_ops::get_pauseparam() returns value which is rejected by
> ethtool_ops::set_pauseparam(). This is something we should avoid.
> If we really wanted to reject ethtool_pauseparam::autoneg on when
> general autonegotiation is off, we would have to disable pause
> autonegotiation whenever general autonegotiation is disabled. I don't
> like that idea, however, as that would mean that
>   ethtool -s dev autoneg off ...
>   ethtool -s dev autoneg on ...
> would reset the setting of pause autonegotiation.
> Therefore I believe the comment should be rather replaced by a warning
> that even if ethtool_pauseparam::autoneg is enabled, pause
> autonegotiation is only active if general autonegotiation is also
> enabled.
> Michal
Thanks for your reply.

I agree with your concerns and will remove this commit from the set when
I resubmit. I also favor replacing the comment in ethtool.h.


Powered by blists - more mailing lists