Re: [PATCH net v2 1/1] ethtool: Fix mod state of verbose no_mask bitset

[Simon Horman]

On Fri, Oct 06, 2023 at 04:12:46PM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@xxxxxxxxxxx>
> 
> A bitset without mask in a _SET request means we want exactly the bits in
> the bitset to be set. This works correctly for compact format but when
> verbose format is parsed, ethnl_update_bitset32_verbose() only sets the
> bits present in the request bitset but does not clear the rest. The commit
> 6699170376ab fixes this issue by clearing the whole target bitmap before we
> start iterating. The solution proposed brought an issue with the behavior
> of the mod variable. As the bitset is always cleared the old val will
> always differ to the new val.
> 
> Fix it by adding a new temporary variable which save the state of the old
> bitmap.
> 
> Fixes: 6699170376ab ("ethtool: fix application of verbose no_mask bitset")
> Signed-off-by: Kory Maincent <kory.maincent@xxxxxxxxxxx>
> ---
> 
> Changes in v2:
> - Fix the allocated size.
> ---
>  net/ethtool/bitset.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ethtool/bitset.c b/net/ethtool/bitset.c
> index 0515d6604b3b..8a6b35c920cd 100644
> --- a/net/ethtool/bitset.c
> +++ b/net/ethtool/bitset.c
> @@ -432,7 +432,9 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits,
>  			      struct netlink_ext_ack *extack, bool *mod)
>  {
>  	struct nlattr *bit_attr;
> +	u32 *tmp = NULL;
>  	bool no_mask;
> +	bool dummy;
>  	int rem;
>  	int ret;
>  
> @@ -448,8 +450,16 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits,
>  	}
>  
>  	no_mask = tb[ETHTOOL_A_BITSET_NOMASK];
> -	if (no_mask)
> -		ethnl_bitmap32_clear(bitmap, 0, nbits, mod);
> +	if (no_mask) {
> +		unsigned int nwords = DIV_ROUND_UP(nbits, 32);
> +		unsigned int nbytes = nwords * sizeof(u32);

Hi Köry,

Thanks for addressing my concerns regarding the size calculations in v1.

I think that a comment is warranted describing the fact that only the map,
and not the mask part, is taken into account in the size calculations
above.

> +
> +		tmp = kcalloc(nwords, sizeof(u32), GFP_KERNEL);
> +		if (!tmp)
> +			return -ENOMEM;
> +		memcpy(tmp, bitmap, nbytes);
> +		ethnl_bitmap32_clear(bitmap, 0, nbits, &dummy);
> +	}

Perhaps we could consider something line the following.
Which would avoid the need for the n_mask condition
in the nla_for_each_nested() loop further below.

		...

                saved_bitmap = kcalloc(nwords, sizeof(u32), GFP_KERNEL);
                if (!saved_bitmap)
                        return -ENOMEM;
                memcpy(saved_bitmap, bitmap, nbytes);
                ethnl_bitmap32_clear(bitmap, 0, nbits, &dummy);

                orig_bitmap = saved_bitmap;
        } else {
                orig_bitmap = bitmap;
        }

(Choosing names for variables seems hard today.)

>  
>  	nla_for_each_nested(bit_attr, tb[ETHTOOL_A_BITSET_BITS], rem) {
>  		bool old_val, new_val;
> @@ -458,13 +468,19 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits,
>  		if (nla_type(bit_attr) != ETHTOOL_A_BITSET_BITS_BIT) {
>  			NL_SET_ERR_MSG_ATTR(extack, bit_attr,
>  					    "only ETHTOOL_A_BITSET_BITS_BIT allowed in ETHTOOL_A_BITSET_BITS");
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			goto out;
>  		}
>  		ret = ethnl_parse_bit(&idx, &new_val, nbits, bit_attr, no_mask,
>  				      names, extack);
>  		if (ret < 0)
> -			return ret;
> -		old_val = bitmap[idx / 32] & ((u32)1 << (idx % 32));
> +			goto out;
> +
> +		if (no_mask)
> +			old_val = tmp[idx / 32] & ((u32)1 << (idx % 32));
> +		else
> +			old_val = bitmap[idx / 32] & ((u32)1 << (idx % 32));
> +
>  		if (new_val != old_val) {
>  			if (new_val)
>  				bitmap[idx / 32] |= ((u32)1 << (idx % 32));
> @@ -474,7 +490,10 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits,
>  		}
>  	}
>  
> -	return 0;
> +	ret = 0;
> +out:
> +	kfree(tmp);
> +	return ret;
>  }
>  
>  static int ethnl_compact_sanity_checks(unsigned int nbits,
> -- 
> 2.25.1
>