diff mbox series

[net,1/1] ethtool: Fix mod state of verbose no_mask bitset

Message ID 20231003085653.3104411-1-kory.maincent@bootlin.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net,1/1] ethtool: Fix mod state of verbose no_mask bitset | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1342 this patch: 1342
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1364 this patch: 1364
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1365 this patch: 1365
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 54 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kory Maincent Oct. 3, 2023, 8:56 a.m. UTC 
From: Kory Maincent <kory.maincent@bootlin.com>

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@bootlin.com>
Cc: stable@vger.kernel.org
---
 net/ethtool/bitset.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Comments

Simon Horman Oct. 4, 2023, 11:07 a.m. UTC | #1 
On Tue, Oct 03, 2023 at 10:56:52AM +0200, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> 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@bootlin.com>
> Cc: stable@vger.kernel.org
> ---
>  net/ethtool/bitset.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ethtool/bitset.c b/net/ethtool/bitset.c
> index 0515d6604b3b..95f11b0a38b4 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,11 @@ 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) {
> +		tmp = kcalloc(nbits, sizeof(u32), GFP_KERNEL);
> +		memcpy(tmp, bitmap, nbits);

Hi Köry,

I'm no expert on etnhl bitmaps. But the above doesn't seem correct to me.
Given that sizeof(u32) == 4:

* The allocation is for nbits * 4 bytes
* The copy is for its for nbits bytes
* I believe that bitmap contains space for the value followed by a mask.
  So it seems to me the size of bitmap, in words, is
  DIV_ROUND_UP(nbits, 32) * 2
  And in bytes: DIV_ROUND_UP(nbits, 32) * 16
  But perhaps only half is needed if only the value part of tmp is used.

If I'm on the right track here I'd suggest helpers might be in order.

> +		ethnl_bitmap32_clear(bitmap, 0, nbits, &dummy);
> +	}
>  
>  	nla_for_each_nested(bit_attr, tb[ETHTOOL_A_BITSET_BITS], rem) {
>  		bool old_val, new_val;
> @@ -458,13 +463,18 @@ 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 +484,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
> 
>
Kory Maincent Oct. 5, 2023, 8:03 a.m. UTC | #2 
Hello Simon,

Thank for your review.

On Wed, 4 Oct 2023 13:07:14 +0200
Simon Horman <horms@kernel.org> wrote:

> On Tue, Oct 03, 2023 at 10:56:52AM +0200, Köry Maincent wrote:
> > From: Kory Maincent <kory.maincent@bootlin.com>
>
> > @@ -448,8 +450,11 @@ 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) {
> > +		tmp = kcalloc(nbits, sizeof(u32), GFP_KERNEL);
> > +		memcpy(tmp, bitmap, nbits);  
> 
> Hi Köry,
> 
> I'm no expert on etnhl bitmaps. But the above doesn't seem correct to me.
> Given that sizeof(u32) == 4:
> 
> * The allocation is for nbits * 4 bytes
> * The copy is for its for nbits bytes
> * I believe that bitmap contains space for the value followed by a mask.
>   So it seems to me the size of bitmap, in words, is
>   DIV_ROUND_UP(nbits, 32) * 2
>   And in bytes: DIV_ROUND_UP(nbits, 32) * 16
>   But perhaps only half is needed if only the value part of tmp is used.
> 
> If I'm on the right track here I'd suggest helpers might be in order.

You are right I should use the same alloc as ethnl_update_bitset with tmp
instead of bitmap32:

        u32 small_bitmap32[ETHNL_SMALL_BITMAP_WORDS];                      
        u32 *bitmap32 = small_bitmap32; 
        if (nbits > ETHNL_SMALL_BITMAP_BITS) {                             
                unsigned int dst_words = DIV_ROUND_UP(nbits, 32);          
                                                                           
                bitmap32 = kmalloc_array(dst_words, sizeof(u32), GFP_KERNEL);
                if (!bitmap32)                                             
                        return -ENOMEM;                                    
        }   

But I am still wondering if it needs to be double as you said for the size of
the value followed by the mask. Not sure about it, as ethnl_update_bitset does
not do it. 

Regards,
Simon Horman Oct. 5, 2023, 1:36 p.m. UTC | #3 
On Thu, Oct 05, 2023 at 10:03:49AM +0200, Köry Maincent wrote:
> Hello Simon,
> 
> Thank for your review.
> 
> On Wed, 4 Oct 2023 13:07:14 +0200
> Simon Horman <horms@kernel.org> wrote:
> 
> > On Tue, Oct 03, 2023 at 10:56:52AM +0200, Köry Maincent wrote:
> > > From: Kory Maincent <kory.maincent@bootlin.com>
> >
> > > @@ -448,8 +450,11 @@ 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) {
> > > +		tmp = kcalloc(nbits, sizeof(u32), GFP_KERNEL);
> > > +		memcpy(tmp, bitmap, nbits);  
> > 
> > Hi Köry,
> > 
> > I'm no expert on etnhl bitmaps. But the above doesn't seem correct to me.
> > Given that sizeof(u32) == 4:
> > 
> > * The allocation is for nbits * 4 bytes
> > * The copy is for its for nbits bytes
> > * I believe that bitmap contains space for the value followed by a mask.
> >   So it seems to me the size of bitmap, in words, is
> >   DIV_ROUND_UP(nbits, 32) * 2
> >   And in bytes: DIV_ROUND_UP(nbits, 32) * 16
> >   But perhaps only half is needed if only the value part of tmp is used.
> > 
> > If I'm on the right track here I'd suggest helpers might be in order.
> 
> You are right I should use the same alloc as ethnl_update_bitset with tmp
> instead of bitmap32:
> 
>         u32 small_bitmap32[ETHNL_SMALL_BITMAP_WORDS];                      
>         u32 *bitmap32 = small_bitmap32; 
>         if (nbits > ETHNL_SMALL_BITMAP_BITS) {                             
>                 unsigned int dst_words = DIV_ROUND_UP(nbits, 32);          
>                                                                            
>                 bitmap32 = kmalloc_array(dst_words, sizeof(u32), GFP_KERNEL);
>                 if (!bitmap32)                                             
>                         return -ENOMEM;                                    
>         }   
> 
> But I am still wondering if it needs to be double as you said for the size of
> the value followed by the mask. Not sure about it, as ethnl_update_bitset does
> not do it. 

If you only need the value, then I don' think you need to x2 the allocation.
But I could be wrong.
diff mbox series

Patch

diff --git a/net/ethtool/bitset.c b/net/ethtool/bitset.c
index 0515d6604b3b..95f11b0a38b4 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,11 @@  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) {
+		tmp = kcalloc(nbits, sizeof(u32), GFP_KERNEL);
+		memcpy(tmp, bitmap, nbits);
+		ethnl_bitmap32_clear(bitmap, 0, nbits, &dummy);
+	}
 
 	nla_for_each_nested(bit_attr, tb[ETHTOOL_A_BITSET_BITS], rem) {
 		bool old_val, new_val;
@@ -458,13 +463,18 @@  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 +484,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,