netdev - Re: [PATCH, net-next, v6, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

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:   Thu, 27 Feb 2020 16:14:17 -0600
From:   Cris Forno <cforno12@...ux.ibm.com>
To:     Michal Kubecek <mkubecek@...e.cz>, netdev@...r.kernel.org
Cc:     Cris Forno <cforno12@...ux.vnet.ibm.com>, mst@...hat.com,
        jasowang@...hat.com, haiyangz@...rosoft.com,
        sthemmin@...rosoft.com, sashal@...nel.org, tlfalcon@...ux.ibm.com,
        davem@...emloft.net, willemdebruijn.kernel@...il.com,
        kuba@...nel.org
Subject: Re: [PATCH, net-next, v6, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

Michal Kubecek <mkubecek@...e.cz> writes:

> On Tue, Feb 25, 2020 at 03:41:10PM -0600, Cris Forno wrote:
>> Three virtual devices (ibmveth, virtio_net, and netvsc) all have
>> similar code to get link settings and validate ethtool command. To
>> eliminate duplication of code, it is factored out into core/ethtool.c.
>> 
>> Signed-off-by: Cris Forno <cforno12@...ux.vnet.ibm.com>
>> ---
> [...]
>> +int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>> +				       const struct ethtool_link_ksettings *cmd,
>> +				       u32 *dev_speed, u8 *dev_duplex,
>> +				       bool (*dev_virtdev_validate_cmd)
>> +				       (const struct ethtool_link_ksettings *))
>> +{
>> +	bool (*validate)(const struct ethtool_link_ksettings *);
>> +	u32 speed;
>> +	u8 duplex;
>> +
>> +	validate = dev_virtdev_validate_cmd ?: ethtool_virtdev_validate_cmd;
>> +	speed = cmd->base.speed;
>> +	duplex = cmd->base.duplex;
>> +	/* don't allow custom speed and duplex */
>> +	if (!ethtool_validate_speed(speed) ||
>> +	    !ethtool_validate_duplex(duplex) ||
>> +	    !(*validate)(cmd))
>> +		return -EINVAL;
>> +	*dev_speed = speed;
>> +	*dev_duplex = duplex;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(ethtool_virtdev_set_link_ksettings);
>
> I didn't realize it when I asked about netvsc_validate_ethtool_ss_cmd() 
> while reviewing v5 but after you got rid of it, all three callers of
> ethtool_virtdev_set_link_ksettings() call it with NULL as validator,
> i.e. use the default ethtool_virtdev_validate_cmd().
>
> This brings a question if we really need the possibility to provide
> a custom validator function. Do you think we should expect some driver
> needing a custom validator function soon? If not, we should probably use
> the default validation unconditionally for now and only add the option
> to provide custom validator function when (if) there is use for it.
>
> Michal
I agree, we do not need the custom validator function pointer parameter
in the set_link_ksettings function since it is currently not needed. The
parameter will be removed in v7. Thanks again Michal!

--Cris Forno