linux-kernel - Re: [PATCH v2 1/2] ethtool: provide UAPI for PHY master/slave configuration.

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] [day] [month] [year] [list]

Date:   Mon, 20 Apr 2020 21:45:04 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Michal Kubecek <mkubecek@...e.cz>,
        David Jander <david@...tonic.nl>, kernel@...gutronix.de,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Russell King <linux@...linux.org.uk>, mkl@...gutronix.de,
        Marek Vasut <marex@...x.de>,
        Christian Herber <christian.herber@....com>
Subject: Re: [PATCH v2 1/2] ethtool: provide UAPI for PHY master/slave
 configuration.

> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -294,7 +294,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  			 phydev->advertising, autoneg == AUTONEG_ENABLE);
>  
>  	phydev->duplex = duplex;
> -
> +	phydev->master_slave_set = cmd->base.master_slave;

Shouldn't you validate what has been passed from userspace it before
setting it?

>  	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>  
>  	/* Restart the PHY */
> @@ -313,6 +313,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  
>  	cmd->base.speed = phydev->speed;
>  	cmd->base.duplex = phydev->duplex;
> +	cmd->base.master_slave = phydev->master_slave_get;
>  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>  		cmd->base.port = PORT_BNC;
>  	else

You had me confused for a while. You are packing multiple things into
master_slave_get. Some bits are configuration, some are current
state. I don't like this. I think this is so you can shoe-horn this
into the existing IOCTL API? There are limited spare bytes?

Maybe we should not support this via the IOCTL, only the netlink
ethtool?

> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -185,6 +185,7 @@ enum {
>  	ETHTOOL_A_LINKMODES_PEER,		/* bitset */
>  	ETHTOOL_A_LINKMODES_SPEED,		/* u32 */
>  	ETHTOOL_A_LINKMODES_DUPLEX,		/* u8 */
> +	ETHTOOL_A_LINKMODES_MASTER_SLAVE,	/* u8 */

We would want two enums here, one for configuration, one for state.

> @@ -119,7 +121,9 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
>  	}
>  
>  	if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
> -	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
> +	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
> +	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE,
> +		       lsettings->master_slave))

Here we return both configuration and current state.

>  		return -EMSGSIZE;
>  
>  	return 0;
> @@ -248,6 +252,7 @@ linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
>  	[ETHTOOL_A_LINKMODES_PEER]		= { .type = NLA_REJECT },
>  	[ETHTOOL_A_LINKMODES_SPEED]		= { .type = NLA_U32 },
>  	[ETHTOOL_A_LINKMODES_DUPLEX]		= { .type = NLA_U8 },
> +	[ETHTOOL_A_LINKMODES_MASTER_SLAVE]	= { .type = NLA_U8 },
>  };
>  
>  /* Set advertised link modes to all supported modes matching requested speed
> @@ -310,6 +315,8 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
>  			 mod);
>  	ethnl_update_u8(&lsettings->duplex, tb[ETHTOOL_A_LINKMODES_DUPLEX],
>  			mod);
> +	ethnl_update_u8(&lsettings->master_slave,
> +			tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE], mod);

Here we expect just configuration.

There actually is space for two fields in the ethtool_link_settings,
so i don't see why you actually did not use two.

   Andrew