netdev - Re: [RFC net-next 1/4] ethtool: extend coalesce API

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: <20210526165633.3f7982c9@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 26 May 2021 16:56:33 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Huazhong Tan <tanhuazhong@...wei.com>
Cc:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <salil.mehta@...wei.com>, <yisen.zhuang@...wei.com>,
        <huangdaode@...wei.com>, <linuxarm@...wei.com>,
        <dledford@...hat.com>, <jgg@...pe.ca>, <netanel@...zon.com>,
        <akiyano@...zon.com>, <thomas.lendacky@....com>,
        <irusskikh@...vell.com>, <michael.chan@...adcom.com>,
        <edwin.peer@...adcom.com>, <rohitm@...lsio.com>,
        <jesse.brandeburg@...el.com>, <jacob.e.keller@...el.com>,
        <ioana.ciornei@....com>, <vladimir.oltean@....com>,
        <sgoutham@...vell.com>, <sbhatta@...vell.com>, <saeedm@...dia.com>,
        <ecree.xilinx@...il.com>, <grygorii.strashko@...com>,
        <merez@...eaurora.org>, <kvalo@...eaurora.org>,
        <linux-wireless@...r.kernel.org>
Subject: Re: [RFC net-next 1/4] ethtool: extend coalesce API

On Wed, 26 May 2021 17:27:39 +0800 Huazhong Tan wrote:
> @@ -606,8 +611,12 @@ struct ethtool_ops {
>  			      struct ethtool_eeprom *, u8 *);
>  	int	(*set_eeprom)(struct net_device *,
>  			      struct ethtool_eeprom *, u8 *);
> -	int	(*get_coalesce)(struct net_device *, struct ethtool_coalesce *);
> -	int	(*set_coalesce)(struct net_device *, struct ethtool_coalesce *);
> +	int	(*get_coalesce)(struct net_device *,
> +				struct netlink_ext_ack *,

ext_ack is commonly the last argument AFAIR.

> +				struct kernel_ethtool_coalesce *);

Seeing all the driver changes I can't say I'm a huge fan of 
the encapsulation. We end up with a local variable for the "base"
structure, e.g.:

 static int wil_ethtoolops_set_coalesce(struct net_device *ndev,
-				       struct ethtool_coalesce *cp)
+				       struct netlink_ext_ack *extack,
+				       struct kernel_ethtool_coalesce *cp)
 {
+	struct ethtool_coalesce *coal_base = &cp->base;
 	struct wil6210_priv *wil = ndev_to_wil(ndev);
 	struct wireless_dev *wdev = ndev->ieee80211_ptr;

so why not leave the base alone and pass the new members in a separate
structure?

> +	int	(*set_coalesce)(struct net_device *,
> +				struct netlink_ext_ack *,
> +				struct kernel_ethtool_coalesce *);
>  	void	(*get_ringparam)(struct net_device *,
>  				 struct ethtool_ringparam *);
>  	int	(*set_ringparam)(struct net_device *,

>  static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
>  						   void __user *useraddr)
>  {
> -	struct ethtool_coalesce coalesce;
> +	struct kernel_ethtool_coalesce coalesce;
>  	int ret;
>  
>  	if (!dev->ethtool_ops->set_coalesce)
>  		return -EOPNOTSUPP;
>  
> -	if (copy_from_user(&coalesce, useraddr, sizeof(coalesce)))
> +	if (copy_from_user(&coalesce.base, useraddr, sizeof(coalesce.base)))
>  		return -EFAULT;
>  
>  	if (!ethtool_set_coalesce_supported(dev, &coalesce))
>  		return -EOPNOTSUPP;
>  
> -	ret = dev->ethtool_ops->set_coalesce(dev, &coalesce);
> +	ret = dev->ethtool_ops->set_coalesce(dev, NULL, &coalesce);
>  	if (!ret)
>  		ethtool_notify(dev, ETHTOOL_MSG_COALESCE_NTF, NULL);
>  	return ret;

Should IOCTL overwrite the settings it doesn't know about with 0 
or preserve the existing values?