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: <20200929164455.pzymi4chmvl3yua5@lion.mk-sys.cz>
Date:   Tue, 29 Sep 2020 18:44:55 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Ido Schimmel <idosch@...sch.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 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.

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?

Michal