netdev - Re: ethtool -s always return 0 even for errors

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: <20200423133649.GF6778@lion.mk-sys.cz>
Date:   Thu, 23 Apr 2020 15:36:49 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Yi Zhao <yi.zhao@...driver.com>, linville@...driver.com
Subject: Re: ethtool -s always return 0 even for errors

On Thu, Apr 23, 2020 at 05:45:47PM +0800, Yi Zhao wrote:
> The ethtool -s returns 0 when it fails with an error (stderr):
> 
> $ ethtool -s eth0 duplex full
> Cannot advertise duplex full
> $ echo $?
> 0
> $ ethtool -s eth0 speed 10
> Cannot advertise speed 10
> $ echo $?
> 0

These two are not really errors, just warnings. According to comments in
the code, the idea was that requesting speed and/or duplex with
autonegotiation enabled (either already enabled or requested to be
enabled) and no explicit request for advertised modes (no "advertise"
keyword), ethtool should enable exactly the modes (out of those
supported by the device) which match requested speed and/or duplex
value(s). The messages you see above are warnings that this logic is not
implemented and all parameters are just passed to kernel and probably
ignored (depends on the driver).

Actually, with kernel 5.6 (with CONFIG_ETHTOOL_NETLINK=y) and ethtool
built from git (or 5.6 once released), these commands work as expected:

lion:/home/mike/work/git/ethtool # ./ethtool eth0
Settings for eth0:
...
        Advertised link modes:  10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Full
...
        Auto-negotiation: on
...
lion:/home/mike/work/git/ethtool # ./ethtool -s eth0 speed 100
lion:/home/mike/work/git/ethtool # ./ethtool eth0
Settings for eth0:
...
        Advertised link modes:  100baseT/Half 100baseT/Full
...
lion:/home/mike/work/git/ethtool # ./ethtool -s eth0 duplex full
lion:/home/mike/work/git/ethtool # ./ethtool eth0
Settings for eth0:
...
        Advertised link modes:  10baseT/Full
                                100baseT/Full
                                1000baseT/Full
...
lion:/home/mike/work/git/ethtool # ./ethtool -s eth0 speed 100 duplex full
lion:/home/mike/work/git/ethtool # ./ethtool eth0
Settings for eth0:
...
        Advertised link modes:  100baseT/Full
...

> $ ethtool -s eth1 duplex full
> Cannot get current device settings: No such device
>   not setting duplex
> $ echo $?
> 0

The problem here is that for historical reasons, "ethtool -s" may issue
up to three separate ioctl requests (or up to four netlink requests with
new kernel and ethtool), depending on which parameters you request on
command line. Each of them can either succeed or fail and you can even
see multiple error messages:

lion:/home/mike/work/git/ethtool # ethtool -s foo phyad 3 wol um msglvl 7
Cannot get current device settings: No such device
  not setting phy_address
Cannot get current wake-on-lan settings: No such device
  not setting wol
Cannot get msglvl: No such device

Currently, do_sset() always returns 0. While it certainly feels wrong to
return 0 if all requests fail (including your case where there was only
one request and it failed), it is much less obvious what should we
return if some of the requests succeed and some fail.

Michal