netdev - Re: [PATCH net-next v3 1/7] ethtool: Extend link modes settings uAPI with lanes

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: <20210125110224.08886797@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 25 Jan 2021 11:02:24 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Danielle Ratson <danieller@...dia.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Jiri Pirko <jiri@...dia.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "mkubecek@...e.cz" <mkubecek@...e.cz>, mlxsw <mlxsw@...dia.com>,
        Ido Schimmel <idosch@...dia.com>
Subject: Re: [PATCH net-next v3 1/7] ethtool: Extend link modes settings
 uAPI with lanes

On Mon, 25 Jan 2021 15:53:24 +0000 Danielle Ratson wrote:
> > > @@ -353,10 +358,39 @@ static int ethnl_update_linkmodes(struct
> > > genl_info *info, struct nlattr **tb,
> > >
> > >  	*mod = false;
> > >  	req_speed = tb[ETHTOOL_A_LINKMODES_SPEED];
> > > +	req_lanes = tb[ETHTOOL_A_LINKMODES_LANES];
> > >  	req_duplex = tb[ETHTOOL_A_LINKMODES_DUPLEX];
> > >
> > >  	ethnl_update_u8(&lsettings->autoneg, tb[ETHTOOL_A_LINKMODES_AUTONEG],
> > >  			mod);
> > > +
> > > +	if (req_lanes) {
> > > +		u32 lanes_cfg = nla_get_u32(tb[ETHTOOL_A_LINKMODES_LANES]);  
> > 
> > req_lanes == tb[ETHTOOL_A_LINKMODES_LANES], right?   
> 
> Yes, but req_lanes is a bool and doesn't fit to nla_get_u32. Do you want me to change the req_lanes type and name?

Ah, yes please.

> > Please use req_lanes variable where possible.
> >   
> > > +
> > > +		if (!is_power_of_2(lanes_cfg)) {
> > > +			NL_SET_ERR_MSG_ATTR(info->extack,
> > > +					    tb[ETHTOOL_A_LINKMODES_LANES],
> > > +					    "lanes value is invalid");
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		/* If autoneg is off and lanes parameter is not supported by the
> > > +		 * driver, return an error.
> > > +		 */
> > > +		if (!lsettings->autoneg &&
> > > +		    !dev->ethtool_ops->cap_link_lanes_supported) {
> > > +			NL_SET_ERR_MSG_ATTR(info->extack,
> > > +					    tb[ETHTOOL_A_LINKMODES_LANES],
> > > +					    "lanes configuration not supported by device");
> > > +			return -EOPNOTSUPP;
> > > +		}  
> > 
> > This validation does not depend on the current settings at all,
> > it's just input validation, it can be done before rtnl_lock is
> > taken (in a new function).
> > 
> > You can move ethnl_validate_master_slave_cfg() to that function as
> > well (as a cleanup before this patch).  
> 
> Do you mean to move the ethnl_validate_master_slave_cfg() if from
> that function? 

Yes, to a separate helper.

> Doesn't it depend on the current settings, as opposed
> to the supported lanes param that you wanted me to move as well? Not
> sure I understand the second part of the request...

Sorry maybe I quoted a little too much context form the patch.

A helper like this:

static int ethnl_check_linkmodes(...)
{
	const struct nlattr *master_slave_cfg;
	
	master_slave_cfg = tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG];
	if (master_slave_cfg && 
	    !ethnl_validate_master_slave_cfg(nla_get_u8(master_slave_cfg))) {
		NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
				    "master/slave value is invalid");
		return -EOPNOTSUPP;
	}

	lanes_cfg = ...
	if (!is_power_of_2(...lanes_cfg)) {
		...
		return -EINVAL;
	}

	return 0;
}
 
Which you can call before the device reference is taken:

 int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info)
 {
 	struct ethtool_link_ksettings ksettings = {};
 	struct ethnl_req_info req_info = {};
 	struct nlattr **tb = info->attrs;
 	struct net_device *dev;
 	bool mod = false;
 	int ret;
 
+	ret = ethnl_check_linkmodes(tb);
+	if (ret)
+		return ret;
 
 	ret = ethnl_parse_header_dev_get(&req_info,
 					 tb[ETHTOOL_A_LINKMODES_HEADER],
 					 genl_info_net(info), info->extack,
 					 true);
 	if (ret < 0)
 		return ret;


But please make sure that you move the master_slave_cfg check in a
separate patch.