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:   Tue, 29 Mar 2022 21:09:24 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jie Wang <wangjie125@...wei.com>
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 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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)