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: <87bc57e0-1c37-1867-d958-fcfdda9ac743@huawei.com>
Date:   Thu, 27 May 2021 09:39:57 +0800
From:   Huazhong Tan <tanhuazhong@...wei.com>
To:     Jakub Kicinski <kuba@...nel.org>
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>, <mkubecek@...e.cz>
Subject: Re: [RFC net-next 1/4] ethtool: extend coalesce API


On 2021/5/27 7:56, Jakub Kicinski wrote:
> 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.


yes, will fix it.


>> +				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?


This is a similar approach as struct ethtool_link_ksettings
suggested by Michal in last year's discussion.
https://lore.kernel.org/lkml/20201119220203.fv2uluoeekyoyxrv@lion.mk-sys.cz/

add a new separate structure can make less change. like below
what we have to do is just add a new parameter.
static int wil_ethtoolops_set_coalesce(struct net_device *ndev,
-                       struct ethtool_coalesce *cp)
+                       struct ethtool_coalesce *cp,
+                       struct ethtool_ext_coalesce *ext_cp,
+                       struct netlink_ext_ack *extack)
{
      struct wil6210_priv *wil = ndev_to_wil(ndev);
      struct wireless_dev *wdev = ndev->ieee80211_ptr;

If this is ok, i will send a V2 for it.


>> +	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?


IOCTL will overwrite the setting with random value,
will a get_coalesce before copy_from_user() to fix it.


Thanks.

Huazhong.

> .