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  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]
Date:   Tue, 12 May 2020 20:37:38 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Doug Berger <opendmb@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg
 setting

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.

My real question is, do you think this should be back ported in
stable?  If so, it should be against net. If this is only intended for
new kernels, don't add a Fixes: tag.

> 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.

We can at least improve the error message when using netlink
ethtool. Using extack, we can pass back a string, saying why this
configuration is invalid, that link autoneg is off.

	Andrew

Powered by blists - more mailing lists