[PATCH net v2 0/3] ethtool-netlink bug fixes

[PATCH net v2 0/3] ethtool-netlink bug fixes

[Maxim Mikityanskiy]

This series contains a few bug fixes for ethtool-netlink. These bugs are
specific for the netlink interface, and the legacy ioctl interface is
not affected. These patches aim to have the same behavior in
ethtool-netlink as in the legacy ethtool.

Please also see the sibling series for the userspace tool.

v2 changes: Added Fixes tags.

Maxim Mikityanskiy (3):
  ethtool: Fix preserving of wanted feature bits in netlink interface
  ethtool: Account for hw_features in netlink interface
  ethtool: Don't omit the netlink reply if no features were changed

 net/ethtool/features.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH net v2 1/3] ethtool: Fix preserving of wanted feature bits in netlink interface

[Maxim Mikityanskiy]

Currently, ethtool-netlink calculates new wanted bits as:
(req_wanted & req_mask) | (old_active & ~req_mask)

It completely discards the old wanted bits, so they are forgotten with
the next ethtool command. Sample steps to reproduce:

1. ethtool -k eth0
   tx-tcp-segmentation: on # TSO is on from the beginning
2. ethtool -K eth0 tx off
   tx-tcp-segmentation: off [not requested]
3. ethtool -k eth0
   tx-tcp-segmentation: off [requested on]
4. ethtool -K eth0 rx off # Some change unrelated to TSO
5. ethtool -k eth0
   tx-tcp-segmentation: off # "Wanted on" is forgotten

This commit fixes it by changing the formula to:
(req_wanted & req_mask) | (old_wanted & ~req_mask),
where old_active was replaced by old_wanted to account for the wanted
bits.

The shortcut condition for the case where nothing was changed now
compares wanted bitmasks, instead of wanted to active.

Fixes: 0980bfcd6954 ("ethtool: set netdev features with FEATURES_SET request")
Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 net/ethtool/features.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/ethtool/features.c b/net/ethtool/features.c
index 4e632dc987d8..ec196f0fddc9 100644
--- a/net/ethtool/features.c
+++ b/net/ethtool/features.c
@@ -224,7 +224,9 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
 	DECLARE_BITMAP(wanted_diff_mask, NETDEV_FEATURE_COUNT);
 	DECLARE_BITMAP(active_diff_mask, NETDEV_FEATURE_COUNT);
 	DECLARE_BITMAP(old_active, NETDEV_FEATURE_COUNT);
+	DECLARE_BITMAP(old_wanted, NETDEV_FEATURE_COUNT);
 	DECLARE_BITMAP(new_active, NETDEV_FEATURE_COUNT);
+	DECLARE_BITMAP(new_wanted, NETDEV_FEATURE_COUNT);
 	DECLARE_BITMAP(req_wanted, NETDEV_FEATURE_COUNT);
 	DECLARE_BITMAP(req_mask, NETDEV_FEATURE_COUNT);
 	struct nlattr *tb[ETHTOOL_A_FEATURES_MAX + 1];
@@ -250,6 +252,7 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
 
 	rtnl_lock();
 	ethnl_features_to_bitmap(old_active, dev->features);
+	ethnl_features_to_bitmap(old_wanted, dev->wanted_features);
 	ret = ethnl_parse_bitset(req_wanted, req_mask, NETDEV_FEATURE_COUNT,
 				 tb[ETHTOOL_A_FEATURES_WANTED],
 				 netdev_features_strings, info->extack);
@@ -261,11 +264,11 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
 		goto out_rtnl;
 	}
 
-	/* set req_wanted bits not in req_mask from old_active */
+	/* set req_wanted bits not in req_mask from old_wanted */
 	bitmap_and(req_wanted, req_wanted, req_mask, NETDEV_FEATURE_COUNT);
-	bitmap_andnot(new_active, old_active, req_mask, NETDEV_FEATURE_COUNT);
-	bitmap_or(req_wanted, new_active, req_wanted, NETDEV_FEATURE_COUNT);
-	if (bitmap_equal(req_wanted, old_active, 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;
 	}
-- 
2.25.1

[PATCH net v2 2/3] ethtool: Account for hw_features in netlink interface

[Maxim Mikityanskiy]

ethtool-netlink ignores dev->hw_features and may confuse the drivers by
asking them to enable features not in the hw_features bitmask. For
example:

1. ethtool -k eth0
   tls-hw-tx-offload: off [fixed]
2. ethtool -K eth0 tls-hw-tx-offload on
   tls-hw-tx-offload: on
3. ethtool -k eth0
   tls-hw-tx-offload: on [fixed]

Fitler out dev->hw_features from req_wanted to fix it and to resemble
the legacy ethtool behavior.

Fixes: 0980bfcd6954 ("ethtool: set netdev features with FEATURES_SET request")
Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 net/ethtool/features.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/features.c b/net/ethtool/features.c
index ec196f0fddc9..6b288bfd7678 100644
--- a/net/ethtool/features.c
+++ b/net/ethtool/features.c
@@ -273,7 +273,8 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
 		goto out_rtnl;
 	}
 
-	dev->wanted_features = ethnl_bitmap_to_features(req_wanted);
+	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);
-- 
2.25.1

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

[Maxim Mikityanskiy]

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@mellanox.com>
---
 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);
 
-- 
2.25.1

Re: [PATCH net v2 1/3] ethtool: Fix preserving of wanted feature bits in netlink interface

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 3313 bytes --]

On Mon, Aug 17, 2020 at 04:34:05PM +0300, Maxim Mikityanskiy wrote:
> Currently, ethtool-netlink calculates new wanted bits as:
> (req_wanted & req_mask) | (old_active & ~req_mask)
> 
> It completely discards the old wanted bits, so they are forgotten with
> the next ethtool command. Sample steps to reproduce:
> 
> 1. ethtool -k eth0
>    tx-tcp-segmentation: on # TSO is on from the beginning
> 2. ethtool -K eth0 tx off
>    tx-tcp-segmentation: off [not requested]
> 3. ethtool -k eth0
>    tx-tcp-segmentation: off [requested on]
> 4. ethtool -K eth0 rx off # Some change unrelated to TSO
> 5. ethtool -k eth0
>    tx-tcp-segmentation: off # "Wanted on" is forgotten
> 
> This commit fixes it by changing the formula to:
> (req_wanted & req_mask) | (old_wanted & ~req_mask),
> where old_active was replaced by old_wanted to account for the wanted
> bits.
> 
> The shortcut condition for the case where nothing was changed now
> compares wanted bitmasks, instead of wanted to active.
> 
> Fixes: 0980bfcd6954 ("ethtool: set netdev features with FEATURES_SET request")
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> ---

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

>  net/ethtool/features.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ethtool/features.c b/net/ethtool/features.c
> index 4e632dc987d8..ec196f0fddc9 100644
> --- a/net/ethtool/features.c
> +++ b/net/ethtool/features.c
> @@ -224,7 +224,9 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
>  	DECLARE_BITMAP(wanted_diff_mask, NETDEV_FEATURE_COUNT);
>  	DECLARE_BITMAP(active_diff_mask, NETDEV_FEATURE_COUNT);
>  	DECLARE_BITMAP(old_active, NETDEV_FEATURE_COUNT);
> +	DECLARE_BITMAP(old_wanted, NETDEV_FEATURE_COUNT);
>  	DECLARE_BITMAP(new_active, NETDEV_FEATURE_COUNT);
> +	DECLARE_BITMAP(new_wanted, NETDEV_FEATURE_COUNT);
>  	DECLARE_BITMAP(req_wanted, NETDEV_FEATURE_COUNT);
>  	DECLARE_BITMAP(req_mask, NETDEV_FEATURE_COUNT);
>  	struct nlattr *tb[ETHTOOL_A_FEATURES_MAX + 1];
> @@ -250,6 +252,7 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
>  
>  	rtnl_lock();
>  	ethnl_features_to_bitmap(old_active, dev->features);
> +	ethnl_features_to_bitmap(old_wanted, dev->wanted_features);
>  	ret = ethnl_parse_bitset(req_wanted, req_mask, NETDEV_FEATURE_COUNT,
>  				 tb[ETHTOOL_A_FEATURES_WANTED],
>  				 netdev_features_strings, info->extack);
> @@ -261,11 +264,11 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
>  		goto out_rtnl;
>  	}
>  
> -	/* set req_wanted bits not in req_mask from old_active */
> +	/* set req_wanted bits not in req_mask from old_wanted */
>  	bitmap_and(req_wanted, req_wanted, req_mask, NETDEV_FEATURE_COUNT);
> -	bitmap_andnot(new_active, old_active, req_mask, NETDEV_FEATURE_COUNT);
> -	bitmap_or(req_wanted, new_active, req_wanted, NETDEV_FEATURE_COUNT);
> -	if (bitmap_equal(req_wanted, old_active, 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;
>  	}
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net v2 2/3] ethtool: Account for hw_features in netlink interface

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

On Mon, Aug 17, 2020 at 04:34:06PM +0300, Maxim Mikityanskiy wrote:
> ethtool-netlink ignores dev->hw_features and may confuse the drivers by
> asking them to enable features not in the hw_features bitmask. For
> example:
> 
> 1. ethtool -k eth0
>    tls-hw-tx-offload: off [fixed]
> 2. ethtool -K eth0 tls-hw-tx-offload on
>    tls-hw-tx-offload: on
> 3. ethtool -k eth0
>    tls-hw-tx-offload: on [fixed]
> 
> Fitler out dev->hw_features from req_wanted to fix it and to resemble
> the legacy ethtool behavior.
> 
> Fixes: 0980bfcd6954 ("ethtool: set netdev features with FEATURES_SET request")
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>

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

> ---
>  net/ethtool/features.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/features.c b/net/ethtool/features.c
> index ec196f0fddc9..6b288bfd7678 100644
> --- a/net/ethtool/features.c
> +++ b/net/ethtool/features.c
> @@ -273,7 +273,8 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
>  		goto out_rtnl;
>  	}
>  
> -	dev->wanted_features = ethnl_bitmap_to_features(req_wanted);
> +	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);
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]

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@mellanox.com>

Reviewed-by: Michal Kubecek <mkubecek@suse.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net v2 0/3] ethtool-netlink bug fixes

[David Miller]

From: Maxim Mikityanskiy <maximmi@mellanox.com>
Date: Mon, 17 Aug 2020 16:34:04 +0300

> This series contains a few bug fixes for ethtool-netlink. These bugs are
> specific for the netlink interface, and the legacy ioctl interface is
> not affected. These patches aim to have the same behavior in
> ethtool-netlink as in the legacy ethtool.
> 
> Please also see the sibling series for the userspace tool.
> 
> v2 changes: Added Fixes tags.

Series applied and queued up for -stable, thank you.