netdev - Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces

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: <20200930143527.GA1824481@shredder>
Date:   Wed, 30 Sep 2020 17:35:27 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Michal Kubecek <mkubecek@...e.cz>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org, f.fainelli@...il.com,
        ayal@...dia.com, danieller@...dia.com, amcohen@...dia.com,
        mlxsw@...dia.com, Ido Schimmel <idosch@...dia.com>
Subject: Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and
 ioctl interfaces

On Wed, Sep 30, 2020 at 04:19:09PM +0200, Andrew Lunn wrote:
> > > I don't think so. Doing:
> > > 
> > > # ethtool -s eth0 autoneg
> > > 
> > > Is a pretty established behavior to enable all the supported advertise
> > > bits.
> 
> I would disagree. phylib will return -EINVAL for this.

This has nothing to do with the kernel / phylib. With the ioctl
interface when you do:

# ethtool -s eth0 autoneg on

The ethtool user space utility will enable all the supported link modes:

https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/ethtool.c#n3170

For the netlink interface this is done by the kernel:

https://github.com/torvalds/linux/blob/master/net/ethtool/linkmodes.c#L2

But only if speed or duplex were specified:

https://github.com/torvalds/linux/blob/master/net/ethtool/linkmodes.c#L383

Which is a problem.

> 
> int phy_ethtool_ksettings_set(struct phy_device *phydev,
>                               const struct ethtool_link_ksettings *cmd)
> {
>         __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>         u8 autoneg = cmd->base.autoneg;
>         u8 duplex = cmd->base.duplex;
>         u32 speed = cmd->base.speed;
> 
> ...
>         linkmode_copy(advertising, cmd->link_modes.advertising);
> 
> ...
> 
>         if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising))
>                 return -EINVAL;
> 
> You have to pass a list of modes you want it to advertise. If you are
> using phylink and not a copper PHY, and autoneg, that means you are
> using in-band signalling. The same is imposed:
> 
>         /* If autonegotiation is enabled, we must have an advertisement */
>         if (config.an_enabled && phylink_is_empty_linkmode(config.advertising))
>                 return -EINVAL;
> 
> We have consistent behaviour whenever Linux is controlling the PHY
> because the core is imposing that behaviour. It would be nice if
> drivers ignoring the PHY core where consistent with this.

You will get an error from mlxsw as well (see example in the change
log).

> 
> 	Andrew