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

Date:   Wed, 26 Feb 2020 17:12:48 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     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

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