| 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
| ||
|
Date: Wed, 30 Sep 2020 10:25:29 +0300
From: Ido Schimmel <idosch@...sch.org>
To: Michal Kubecek <mkubecek@...e.cz>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
f.fainelli@...il.com, andrew@...n.ch, 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 Tue, Sep 29, 2020 at 06:44:55PM +0200, Michal Kubecek wrote:
> On Tue, Sep 29, 2020 at 07:02:47PM +0300, Ido Schimmel wrote:
> > From: Ido Schimmel <idosch@...dia.com>
> >
> > With the ioctl interface, when autoneg is enabled, but without
> > specifying speed, duplex or link modes, the advertised link modes are
> > set to the supported link modes by the ethtool user space utility.
> >
> [...]
> >
> > With the netlink interface, the same thing is done by the kernel, but
> > only if speed or duplex are specified. In which case, the advertised
> > link modes are set by traversing the supported link modes and picking
> > the ones matching the specified speed or duplex.
> >
> > However, if speed nor duplex are specified, the driver is passed an
> > empty advertised link modes bitmap. This causes the mlxsw driver to
> > return an error. Other drivers might also be affected. Example:
>
> This is not completely correct. What actually happens is that the
> advertised modes bitmap is left untouched. The reason why advertised
> modes are cleared for mlxsw is that this driver reports empty advertised
> modes whenever autonegotiation is disabled.
Correct. I'll reword.
>
> This is similar to recent discussions about distinguishing between
> configuration and state. One of them was related to EEE settings, one to
> pause settings, one to WoL settings vs. general wakeup enable/disable:
>
> http://lkml.kernel.org/r/20200511132258.GT1551@shell.armlinux.org.uk
> http://lkml.kernel.org/r/20200512185503.GD1551@shell.armlinux.org.uk
> http://lkml.kernel.org/r/20200521192342.GE8771@lion.mk-sys.cz
>
> All of these are about common problem: we have a settings A and B such
> that B is only effective if A is enabled. Should we report B as disabled
> whenever A is disabled? I believe - and the consensus in those
> discussions seemed to be - that we should report and set A and B
> independently to distinguish between configuration (what user wants) and
> state (how the device behaves). There is also practical aspect: (1) if
> we don't do this, switching A off and on would reset the value of B even
> if no change of B was requested and (2) commands setting A and B must be
> issued in a specific order for changes of B to take effect.
>
> Unfortunately there are drivers like mlxsw (I'm not sure how many) which
> report zero advertised bitmap whenever autonegotiation is off.
>
> > diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
> > index 7044a2853886..a9458c76209e 100644
> > --- a/net/ethtool/linkmodes.c
> > +++ b/net/ethtool/linkmodes.c
> > @@ -288,9 +288,9 @@ linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
> > };
> >
> > /* Set advertised link modes to all supported modes matching requested speed
> > - * and duplex values. Called when autonegotiation is on, speed or duplex is
> > - * requested but no link mode change. This is done in userspace with ioctl()
> > - * interface, move it into kernel for netlink.
> > + * and duplex values, if specified. Called when autonegotiation is on, but no
> > + * link mode change. This is done in userspace with ioctl() interface, move it
> > + * into kernel for netlink.
> > * Returns true if advertised modes bitmap was modified.
> > */
> > static bool ethnl_auto_linkmodes(struct ethtool_link_ksettings *ksettings,
> > @@ -381,7 +381,6 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
> > ethnl_update_u8(&lsettings->master_slave_cfg, master_slave_cfg, mod);
> >
> > if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
> > - (req_speed || req_duplex) &&
> > ethnl_auto_linkmodes(ksettings, req_speed, req_duplex))
> > *mod = true;
>
> I'm afraid we will have to cope with existing drivers hiding advertised
> mode setting when autonegotiation is off. Could we at least limit the
> call to such drivers, i.e. replacing that line with something like
>
> (req_speed || req_duplex || (!old_autoneg && advertised_empty))
>
> where old_autoneg would be the original value of lsettings->autoneg and
> advertised_empty would be set if currently reported advertised modes are
> zero?
I don't think so. Doing:
# ethtool -s eth0 autoneg
Is a pretty established behavior to enable all the supported advertise
bits. Here is an example with an unpatched kernel, two ethtool versions
(ioctl & netlink) and e1000 in QEMU.
Ioctl:
# ethtool --version
ethtool version 5.4
# ethtool -s eth0 advertise 0xC autoneg on
# ethtool eth0
Settings for eth0:
Supported ports: [ TP ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 100baseT/Half 100baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Speed: 1000Mb/s
Duplex: Full
Port: Twisted Pair
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
MDI-X: off (auto)
Supports Wake-on: umbg
Wake-on: d
Current message level: 0x00000007 (7)
drv probe link
Link detected: yes
# ethtool -s eth0 autoneg on
# ethtool eth0
Settings for eth0:
Supported ports: [ TP ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Speed: 1000Mb/s
Duplex: Full
Port: Twisted Pair
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
MDI-X: off (auto)
Supports Wake-on: umbg
Wake-on: d
Current message level: 0x00000007 (7)
drv probe link
Link detected: yes
Netlink:
# ethtool --version
ethtool version 5.8
# ethtool -s eth0 advertise 0xC autoneg on
# ethtool eth0
Settings for eth0:
Supported ports: [ TP ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 100baseT/Half 100baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Speed: 1000Mb/s
Duplex: Full
Auto-negotiation: on
Port: Twisted Pair
PHYAD: 0
Transceiver: internal
MDI-X: off (auto)
Supports Wake-on: umbg
Wake-on: d
Current message level: 0x00000007 (7)
drv probe link
Link detected: yes
# ethtool -s eth0 autoneg on
# ethtool eth0
Settings for eth0:
Supported ports: [ TP ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 100baseT/Half 100baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Speed: 1000Mb/s
Duplex: Full
Auto-negotiation: on
Port: Twisted Pair
PHYAD: 0
Transceiver: internal
MDI-X: off (auto)
Supports Wake-on: umbg
Wake-on: d
Current message level: 0x00000007 (7)
drv probe link
Link detected: yes
Powered by blists - more mailing lists