netdev - Re: [PATCH net v2 3/3] ethtool: Don't omit the netlink reply if no features were changed

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: <20200818224632.bczm5f6ih5bbhreu@lion.mk-sys.cz>
Date:   Wed, 19 Aug 2020 00:46:32 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Maxim Mikityanskiy <maximmi@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net v2 3/3] ethtool: Don't omit the netlink reply if no
 features were changed

On Mon, Aug 17, 2020 at 04:34:07PM +0300, Maxim Mikityanskiy wrote:
> The legacy ethtool userspace tool shows an error when no features could
> be changed. It's useful to have a netlink reply to be able to show this
> error when __netdev_update_features wasn't called, for example:
> 
> 1. ethtool -k eth0
>    large-receive-offload: off
> 2. ethtool -K eth0 rx-fcs on
> 3. ethtool -K eth0 lro on
>    Could not change any device features
>    rx-lro: off [requested on]
> 4. ethtool -K eth0 lro on
>    # The output should be the same, but without this patch the kernel
>    # doesn't send the reply, and ethtool is unable to detect the error.
> 
> This commit makes ethtool-netlink always return a reply when requested,
> and it still avoids unnecessary calls to __netdev_update_features if the
> wanted features haven't changed.
> 
> Fixes: 0980bfcd6954 ("ethtool: set netdev features with FEATURES_SET request")
> Signed-off-by: Maxim Mikityanskiy <maximmi@...lanox.com>

Reviewed-by: Michal Kubecek <mkubecek@...e.cz>

> ---
>  net/ethtool/features.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ethtool/features.c b/net/ethtool/features.c
> index 6b288bfd7678..495635f152ba 100644
> --- a/net/ethtool/features.c
> +++ b/net/ethtool/features.c
> @@ -268,14 +268,11 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
>  	bitmap_and(req_wanted, req_wanted, req_mask, NETDEV_FEATURE_COUNT);
>  	bitmap_andnot(new_wanted, old_wanted, req_mask, NETDEV_FEATURE_COUNT);
>  	bitmap_or(req_wanted, new_wanted, req_wanted, NETDEV_FEATURE_COUNT);
> -	if (bitmap_equal(req_wanted, old_wanted, NETDEV_FEATURE_COUNT)) {
> -		ret = 0;
> -		goto out_rtnl;
> +	if (!bitmap_equal(req_wanted, old_wanted, NETDEV_FEATURE_COUNT)) {
> +		dev->wanted_features &= ~dev->hw_features;
> +		dev->wanted_features |= ethnl_bitmap_to_features(req_wanted) & dev->hw_features;
> +		__netdev_update_features(dev);
>  	}
> -
> -	dev->wanted_features &= ~dev->hw_features;
> -	dev->wanted_features |= ethnl_bitmap_to_features(req_wanted) & dev->hw_features;
> -	__netdev_update_features(dev);
>  	ethnl_features_to_bitmap(new_active, dev->features);
>  	mod = !bitmap_equal(old_active, new_active, NETDEV_FEATURE_COUNT);

We could also move these last two lines to the branch where
__netdev_update_features() is actually called and replace them with

	bitmap_copy(new_active, old_active, NETDEV_FEATURE_COUNT);
	mod = false;

otherwise. But it's probably not worth complicating the code.

Michal

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