netdev - Re: [RFCv3 PATCH net-next 1/2] net-next: ethtool: extend ringparam set/get APIs for tx

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, 30 Mar 2022 09:55:29 +0800
From:   "wangjie (L)" <wangjie125@...wei.com>
To:     Michal Kubecek <mkubecek@...e.cz>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <netdev@...r.kernel.org>,
        <huangguangbin2@...wei.com>, <lipeng321@...wei.com>,
        <shenjian15@...wei.com>, <moyufeng@...wei.com>,
        <linyunsheng@...wei.com>, <salil.mehta@...wei.com>,
        <chenhao288@...ilicon.com>
Subject: Re: [RFCv3 PATCH net-next 1/2] net-next: ethtool: extend ringparam
 set/get APIs for tx_push



On 2022/3/30 3:09, Michal Kubecek wrote:
> On Tue, Mar 29, 2022 at 05:19:12PM +0800, Jie Wang wrote:
>> Currently tx push is a standard driver feature which controls use of a fast
>> path descriptor push. So this patch extends the ringparam APIs and data
>> structures to support set/get tx push by ethtool -G/g.
>>
>> Signed-off-by: Jie Wang <wangjie125@...wei.com>
>> ---
> [...]
>> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
>> index 9f33c9689b56..2bc2d91f2e66 100644
>> --- a/net/ethtool/rings.c
>> +++ b/net/ethtool/rings.c
> [...]
>> @@ -205,6 +210,15 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
>>  		goto out_ops;
>>  	}
>>
>> +	if (kernel_ringparam.tx_push &&
>> +	    !(ops->supported_ring_params & ETHTOOL_RING_USE_TX_PUSH)) {
>> +		ret = -EOPNOTSUPP;
>> +		NL_SET_ERR_MSG_ATTR(info->extack,
>> +				    tb[ETHTOOL_A_RINGS_TX_PUSH],
>> +				    "setting tx push not supported");
>> +		goto out_ops;
>> +	}
>> +
>>  	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
>>  					      &kernel_ringparam, info->extack);
>>  	if (ret < 0)
>
> This only disallows setting the parameter to true but allows requests
> trying to set it to false. I would rather prefer
>
> 	if (tb[ETHTOOL_A_RINGS_TX_PUSH] &&
> 	    !(ops->supported_ring_params & ETHTOOL_RING_USE_TX_PUSH)) {
> 		...
> 	}
>
> and putting the check before rtnl_lock() so that we do not do useless
> work if the request is invalid.
>
> But the same can be said about the two checks we already have so those
> should be probably changed and moved as well.
>
> Michal

OK, I will move these three checks in a new patch.

>