linux-kernel - Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

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: <20200512185503.GD1551@shell.armlinux.org.uk>
Date:   Tue, 12 May 2020 19:55:03 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Doug Berger <opendmb@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        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:
> 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 looks like the same argument I've been having with Heiner over
the EEE interface, except there's a difference here.

# ethtool -A eth0 autoneg on
# ethtool -s eth0 autoneg off speed 100 duplex full

After those two commands, what is the state of pause mode?  The answer
is, it's disabled.

# ethtool -A eth0 autoneg off rx on tx on

is perfectly acceptable, as we are forcing pause modes at the local
end of the link.

# ethtool -A eth0 autoneg on

Now, the question is whether that should be allowed or not - but this
is merely restoring the "pause" settings that were in effect prior
to the previous command.  It does not enable pause negotiation,
because autoneg as a whole is disabled, but it _allows_ pause
negotiation to occur when autoneg is enabled at some point in the
future.

Also, allowing "ethtool -A eth0 autoneg on" when "ethtool -s eth0
autoneg off" means you can configure the negotiation parameters
_before_ triggering a negotiation cycle on the link.  In other words,
it would avoid:

# ethtool -s eth0 autoneg on
# # Link renegotiates
# ethtool -A eth0 autoneg on
# # Link renegotiates a second time

and it also means that if stuff has already been scripted to avoid
this, nothing breaks.

If we start rejecting ethtool -A because autoneg is disabled, then
things get difficult to configure - we would need ethtool documentation
to state that autoneg must be enabled before configuration of pause
and EEE can be done.  IMHO, that hurts usability, and adds confusion.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up