| Message ID | 20231009133645.44503-1-kory.maincent@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 108a36d07c01edbc5942d27c92494d1c6e4d45a0 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3,1/1] ethtool: Fix mod state of verbose no_mask bitset | expand |
On Mon, Oct 09, 2023 at 03:36:45PM +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> > --- > > Changes in v2: > - Fix the allocated size. > > Changes in v3: > - Add comment. > - Updated variable naming. > - Add orig_bitmap variable to avoid n_mask condition in the > nla_for_each_nested() loop. Hi Köry, thanks for the updates. This one looks good to me. Reviewed-by: Simon Horman <horms@kernel.org>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 9 Oct 2023 15:36:45 +0200 you 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. > > [...] Here is the summary with links: - [net,v3,1/1] ethtool: Fix mod state of verbose no_mask bitset https://git.kernel.org/netdev/net/c/108a36d07c01 You are awesome, thank you!
diff --git a/net/ethtool/bitset.c b/net/ethtool/bitset.c index 0515d6604b3b..883ed9be81f9 100644 --- a/net/ethtool/bitset.c +++ b/net/ethtool/bitset.c @@ -431,8 +431,10 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits, ethnl_string_array_t names, struct netlink_ext_ack *extack, bool *mod) { + u32 *orig_bitmap, *saved_bitmap = NULL; struct nlattr *bit_attr; bool no_mask; + bool dummy; int rem; int ret; @@ -448,8 +450,22 @@ 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); + + /* The bitmap size is only the size of the map part without + * its mask part. + */ + 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; + } nla_for_each_nested(bit_attr, tb[ETHTOOL_A_BITSET_BITS], rem) { bool old_val, new_val; @@ -458,13 +474,14 @@ 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; + old_val = orig_bitmap[idx / 32] & ((u32)1 << (idx % 32)); if (new_val != old_val) { if (new_val) bitmap[idx / 32] |= ((u32)1 << (idx % 32)); @@ -474,7 +491,10 @@ ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits, } } - return 0; + ret = 0; +out: + kfree(saved_bitmap); + return ret; } static int ethnl_compact_sanity_checks(unsigned int nbits,