netdev - Re: [PATCH net-next 2/2] ethtool: netlink: convert commands to common SET

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, 24 Jan 2023 12:41:34 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, edumazet@...gle.com,
        piergiorgio.beruto@...il.com, gal@...dia.com, tariqt@...dia.com,
        dnlplm@...il.com, sean.anderson@...o.com, linux@...pel-privat.de,
        lkp@...el.com, bagasdotme@...il.com, wangjie125@...wei.com,
        huangguangbin2@...wei.com
Subject: Re: [PATCH net-next 2/2] ethtool: netlink: convert commands to
 common SET

Hi,

On Fri, 2023-01-20 at 21:44 -0800, Jakub Kicinski wrote:
> @@ -241,49 +229,41 @@ const struct nla_policy ethnl_coalesce_set_policy[] = {
>  	[ETHTOOL_A_COALESCE_TX_AGGR_TIME_USECS] = { .type = NLA_U32 },
>  };
>  
> -int ethnl_set_coalesce(struct sk_buff *skb, struct genl_info *info)
> +static int
> +ethnl_set_coalesce_validate(struct ethnl_req_info *req_info,
> +			    struct genl_info *info)
>  {
> -	struct kernel_ethtool_coalesce kernel_coalesce = {};
> -	struct ethtool_coalesce coalesce = {};
> -	struct ethnl_req_info req_info = {};
> +	const struct ethtool_ops *ops = req_info->dev->ethtool_ops;
>  	struct nlattr **tb = info->attrs;
> -	const struct ethtool_ops *ops;
> -	struct net_device *dev;
>  	u32 supported_params;
> -	bool mod = false;
> -	int ret;
>  	u16 a;
>  
> -	ret = ethnl_parse_header_dev_get(&req_info,
> -					 tb[ETHTOOL_A_COALESCE_HEADER],
> -					 genl_info_net(info), info->extack,
> -					 true);
> -	if (ret < 0)
> -		return ret;
> -	dev = req_info.dev;
> -	ops = dev->ethtool_ops;
> -	ret = -EOPNOTSUPP;
> -	if (!ops->get_coalesce || !ops->set_coalesce)
> -		goto out_dev;
> -
>  	/* make sure that only supported parameters are present */
>  	supported_params = ops->supported_coalesce_params;
>  	for (a = ETHTOOL_A_COALESCE_RX_USECS; a < __ETHTOOL_A_COALESCE_CNT; a++)
>  		if (tb[a] && !(supported_params & attr_to_mask(a))) {
> -			ret = -EINVAL;
>  			NL_SET_ERR_MSG_ATTR(info->extack, tb[a],
>  					    "cannot modify an unsupported parameter");
> -			goto out_dev;
> +			return -EINVAL;
>  		}
>  
> -	rtnl_lock();
> -	ret = ethnl_ops_begin(dev);
> -	if (ret < 0)
> -		goto out_rtnl;
> -	ret = ops->get_coalesce(dev, &coalesce, &kernel_coalesce,
> -				info->extack);
> +	return ops->get_coalesce && ops->set_coalesce ? 1 : -EOPNOTSUPP;

The above changes the error code in case a coalesce op is missing (and
likely supported_coalesce_params is zero) from EOPNOTSUPP to EINVAL.

I guess it's not a big deal but perhaps we can preserve the same error
code as prior to this patch? 

Not a big deal, I see other commands check the oops last.

Thanks,

Paolo