* [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
@ 2020-07-27 21:47 Jacob Keller
2020-07-27 21:51 ` Jacob Keller
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Jacob Keller @ 2020-07-27 21:47 UTC (permalink / raw)
To: Andrew Lunn, Michal Kubecek, netdev; +Cc: Jacob Keller, Jamie Gloudon
The ethtool netlink API can send bitsets without an associated bitmask.
These do not get displayed properly, because the dump_link_modes, and
bitset_get_bit to not check whether the provided bitset is a NOMASK
bitset. This results in the inability to display peer advertised link
modes.
The dump_link_modes and bitset_get_bit functions are designed so they
can print either the values or the mask. For a nomask bitmap, this
doesn't make sense. There is no mask.
Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
regardless of the request to display the mask or the value. For full
size bitmaps, the set of provided bits indicates the valid values,
without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
bits without this attribute if nomask is set. This essentially means
that dump_link_modes will treat a NOMASK bitset as having a mask
equivalent to all of its set bits.
For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
For full bitmaps, if nomask is set, then always return true of the bit
is in the set, rather than only if it provides an
ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
bits.
This fixes display of link partner advertised fields when using the
netlink API.
Reported-by: Jamie Gloudon <jamie.gloudon@gmx.fr>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
netlink/bitset.c | 9 ++++++---
netlink/settings.c | 8 +++++---
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/netlink/bitset.c b/netlink/bitset.c
index 130bcdb5b52c..ba5d3ea77ff7 100644
--- a/netlink/bitset.c
+++ b/netlink/bitset.c
@@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
DECLARE_ATTR_TB_INFO(bitset_tb);
const struct nlattr *bits;
const struct nlattr *bit;
+ bool nomask;
int ret;
*retptr = 0;
@@ -57,8 +58,10 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
if (ret < 0)
goto err;
- bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
- bitset_tb[ETHTOOL_A_BITSET_VALUE];
+ nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
+
+ bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
+ bitset_tb[ETHTOOL_A_BITSET_VALUE];
if (bits) {
const uint32_t *bitmap =
(const uint32_t *)mnl_attr_get_payload(bits);
@@ -87,7 +90,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
my_idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
if (my_idx == idx)
- return mask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
+ return mask || nomask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
}
return false;
diff --git a/netlink/settings.c b/netlink/settings.c
index 35ba2f5dd6d5..29557653336e 100644
--- a/netlink/settings.c
+++ b/netlink/settings.c
@@ -280,9 +280,11 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
const struct nlattr *bit;
bool first = true;
int prev = -2;
+ bool nomask;
int ret;
ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info);
+ nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
bits = bitset_tb[ETHTOOL_A_BITSET_BITS];
if (ret < 0)
goto err_nonl;
@@ -297,8 +299,8 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
goto err_nonl;
lm_strings = global_stringset(ETH_SS_LINK_MODES,
nlctx->ethnl2_socket);
- bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
- bitset_tb[ETHTOOL_A_BITSET_VALUE];
+ bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
+ bitset_tb[ETHTOOL_A_BITSET_VALUE];
ret = -EFAULT;
if (!bits || !bitset_tb[ETHTOOL_A_BITSET_SIZE])
goto err_nonl;
@@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] ||
!tb[ETHTOOL_A_BITSET_BIT_NAME])
goto err;
- if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
+ if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
continue;
idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
--
2.26.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 21:47 [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK Jacob Keller
@ 2020-07-27 21:51 ` Jacob Keller
2020-07-27 22:02 ` Jamie Gloudon
2020-07-27 22:11 ` Andrew Lunn
2020-07-27 22:21 ` Michal Kubecek
2 siblings, 1 reply; 12+ messages in thread
From: Jacob Keller @ 2020-07-27 21:51 UTC (permalink / raw)
To: Andrew Lunn, Michal Kubecek, netdev; +Cc: Jamie Gloudon
On 7/27/2020 2:47 PM, Jacob Keller wrote:
> The ethtool netlink API can send bitsets without an associated bitmask.
> These do not get displayed properly, because the dump_link_modes, and
> bitset_get_bit to not check whether the provided bitset is a NOMASK
> bitset. This results in the inability to display peer advertised link
> modes.
>
> The dump_link_modes and bitset_get_bit functions are designed so they
> can print either the values or the mask. For a nomask bitmap, this
> doesn't make sense. There is no mask.
>
> Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
> bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
> regardless of the request to display the mask or the value. For full
> size bitmaps, the set of provided bits indicates the valid values,
> without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
> bits without this attribute if nomask is set. This essentially means
> that dump_link_modes will treat a NOMASK bitset as having a mask
> equivalent to all of its set bits.
>
> For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
> bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
> For full bitmaps, if nomask is set, then always return true of the bit
> is in the set, rather than only if it provides an
> ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
> bits.
>
> This fixes display of link partner advertised fields when using the
> netlink API.
>
> Reported-by: Jamie Gloudon <jamie.gloudon@gmx.fr>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Andrew, could you kindly test this in your setup? I believe it will
fully resolve the issue.
Michal, I think this is complete based on the docs in
ethtool-netlink.rst and some tests. Any further insight?
Thanks,
Jake
> ---
> netlink/bitset.c | 9 ++++++---
> netlink/settings.c | 8 +++++---
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/netlink/bitset.c b/netlink/bitset.c
> index 130bcdb5b52c..ba5d3ea77ff7 100644
> --- a/netlink/bitset.c
> +++ b/netlink/bitset.c
> @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
> DECLARE_ATTR_TB_INFO(bitset_tb);
> const struct nlattr *bits;
> const struct nlattr *bit;
> + bool nomask;
> int ret;
>
> *retptr = 0;
> @@ -57,8 +58,10 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
> if (ret < 0)
> goto err;
>
> - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> - bitset_tb[ETHTOOL_A_BITSET_VALUE];
> + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> +
> + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> + bitset_tb[ETHTOOL_A_BITSET_VALUE];
> if (bits) {
> const uint32_t *bitmap =
> (const uint32_t *)mnl_attr_get_payload(bits);
> @@ -87,7 +90,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
>
> my_idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
> if (my_idx == idx)
> - return mask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
> + return mask || nomask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
> }
>
> return false;
> diff --git a/netlink/settings.c b/netlink/settings.c
> index 35ba2f5dd6d5..29557653336e 100644
> --- a/netlink/settings.c
> +++ b/netlink/settings.c
> @@ -280,9 +280,11 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
> const struct nlattr *bit;
> bool first = true;
> int prev = -2;
> + bool nomask;
> int ret;
>
> ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info);
> + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> bits = bitset_tb[ETHTOOL_A_BITSET_BITS];
> if (ret < 0)
> goto err_nonl;
> @@ -297,8 +299,8 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
> goto err_nonl;
> lm_strings = global_stringset(ETH_SS_LINK_MODES,
> nlctx->ethnl2_socket);
> - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> - bitset_tb[ETHTOOL_A_BITSET_VALUE];
> + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> + bitset_tb[ETHTOOL_A_BITSET_VALUE];
> ret = -EFAULT;
> if (!bits || !bitset_tb[ETHTOOL_A_BITSET_SIZE])
> goto err_nonl;
> @@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
> if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] ||
> !tb[ETHTOOL_A_BITSET_BIT_NAME])
> goto err;
> - if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
> + if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
> continue;
>
> idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 21:51 ` Jacob Keller
@ 2020-07-27 22:02 ` Jamie Gloudon
0 siblings, 0 replies; 12+ messages in thread
From: Jamie Gloudon @ 2020-07-27 22:02 UTC (permalink / raw)
To: Jacob Keller; +Cc: Andrew Lunn, Michal Kubecek, netdev
On Mon, Jul 27, 2020 at 02:51:28PM -0700, Jacob Keller wrote:
>
>
> On 7/27/2020 2:47 PM, Jacob Keller wrote:
> > The ethtool netlink API can send bitsets without an associated bitmask.
> > These do not get displayed properly, because the dump_link_modes, and
> > bitset_get_bit to not check whether the provided bitset is a NOMASK
> > bitset. This results in the inability to display peer advertised link
> > modes.
> >
> > The dump_link_modes and bitset_get_bit functions are designed so they
> > can print either the values or the mask. For a nomask bitmap, this
> > doesn't make sense. There is no mask.
> >
> > Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
> > bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
> > regardless of the request to display the mask or the value. For full
> > size bitmaps, the set of provided bits indicates the valid values,
> > without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
> > bits without this attribute if nomask is set. This essentially means
> > that dump_link_modes will treat a NOMASK bitset as having a mask
> > equivalent to all of its set bits.
> >
> > For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
> > bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
> > For full bitmaps, if nomask is set, then always return true of the bit
> > is in the set, rather than only if it provides an
> > ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
> > bits.
> >
> > This fixes display of link partner advertised fields when using the
> > netlink API.
> >
> > Reported-by: Jamie Gloudon <jamie.gloudon@gmx.fr>
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>
> Andrew, could you kindly test this in your setup? I believe it will
> fully resolve the issue.
>
> Michal, I think this is complete based on the docs in
> ethtool-netlink.rst and some tests. Any further insight?
>
> Thanks,
> Jake
>
> > ---
> > netlink/bitset.c | 9 ++++++---
> > netlink/settings.c | 8 +++++---
> > 2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/netlink/bitset.c b/netlink/bitset.c
> > index 130bcdb5b52c..ba5d3ea77ff7 100644
> > --- a/netlink/bitset.c
> > +++ b/netlink/bitset.c
> > @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
> > DECLARE_ATTR_TB_INFO(bitset_tb);
> > const struct nlattr *bits;
> > const struct nlattr *bit;
> > + bool nomask;
> > int ret;
> >
> > *retptr = 0;
> > @@ -57,8 +58,10 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
> > if (ret < 0)
> > goto err;
> >
> > - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> > - bitset_tb[ETHTOOL_A_BITSET_VALUE];
> > + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> > +
> > + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> > + bitset_tb[ETHTOOL_A_BITSET_VALUE];
> > if (bits) {
> > const uint32_t *bitmap =
> > (const uint32_t *)mnl_attr_get_payload(bits);
> > @@ -87,7 +90,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
> >
> > my_idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
> > if (my_idx == idx)
> > - return mask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
> > + return mask || nomask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
> > }
> >
> > return false;
> > diff --git a/netlink/settings.c b/netlink/settings.c
> > index 35ba2f5dd6d5..29557653336e 100644
> > --- a/netlink/settings.c
> > +++ b/netlink/settings.c
> > @@ -280,9 +280,11 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
> > const struct nlattr *bit;
> > bool first = true;
> > int prev = -2;
> > + bool nomask;
> > int ret;
> >
> > ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info);
> > + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> > bits = bitset_tb[ETHTOOL_A_BITSET_BITS];
> > if (ret < 0)
> > goto err_nonl;
> > @@ -297,8 +299,8 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
> > goto err_nonl;
> > lm_strings = global_stringset(ETH_SS_LINK_MODES,
> > nlctx->ethnl2_socket);
> > - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> > - bitset_tb[ETHTOOL_A_BITSET_VALUE];
> > + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> > + bitset_tb[ETHTOOL_A_BITSET_VALUE];
> > ret = -EFAULT;
> > if (!bits || !bitset_tb[ETHTOOL_A_BITSET_SIZE])
> > goto err_nonl;
> > @@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
> > if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] ||
> > !tb[ETHTOOL_A_BITSET_BIT_NAME])
> > goto err;
> > - if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
> > + if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
> > continue;
> >
> > idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
> >
I can confirm that your patch works. As you can see below:
Link partner advertised pause frame use: Symmetric Receive-only
Link partner advertised auto-negotiation: Yes
Link partner advertised FEC modes: No
You can tag this patch Tested-by me. Thanks!
Regards,
Jamie Gloudon
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 21:47 [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK Jacob Keller
2020-07-27 21:51 ` Jacob Keller
@ 2020-07-27 22:11 ` Andrew Lunn
2020-07-27 22:22 ` Jacob Keller
2020-07-27 22:26 ` Michal Kubecek
2020-07-27 22:21 ` Michal Kubecek
2 siblings, 2 replies; 12+ messages in thread
From: Andrew Lunn @ 2020-07-27 22:11 UTC (permalink / raw)
To: Jacob Keller; +Cc: Michal Kubecek, netdev, Jamie Gloudon
On Mon, Jul 27, 2020 at 02:47:00PM -0700, Jacob Keller wrote:
> The ethtool netlink API can send bitsets without an associated bitmask.
> These do not get displayed properly, because the dump_link_modes, and
> bitset_get_bit to not check whether the provided bitset is a NOMASK
> bitset. This results in the inability to display peer advertised link
> modes.
>
> The dump_link_modes and bitset_get_bit functions are designed so they
> can print either the values or the mask. For a nomask bitmap, this
> doesn't make sense. There is no mask.
>
> Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
> bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
> regardless of the request to display the mask or the value. For full
> size bitmaps, the set of provided bits indicates the valid values,
> without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
> bits without this attribute if nomask is set. This essentially means
> that dump_link_modes will treat a NOMASK bitset as having a mask
> equivalent to all of its set bits.
>
> For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
> bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
> For full bitmaps, if nomask is set, then always return true of the bit
> is in the set, rather than only if it provides an
> ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
> bits.
>
> This fixes display of link partner advertised fields when using the
> netlink API.
Hi Jacob
This is close
Netlink
Link partner advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Link partner advertised pause frame use: No
Link partner advertised auto-negotiation: Yes
Link partner advertised FEC modes: No
IOCTL
Link partner advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Link partner advertised pause frame use: No
Link partner advertised auto-negotiation: Yes
Link partner advertised FEC modes: Not reported
So just the FEC modes differ.
However, i don't think this was part of the original issue, so:
Tested-by: Andrew Lunn <andrew@lunn.ch>
It would be nice to get the FEC modes fixed.
Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 21:47 [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK Jacob Keller
2020-07-27 21:51 ` Jacob Keller
2020-07-27 22:11 ` Andrew Lunn
@ 2020-07-27 22:21 ` Michal Kubecek
2020-07-27 22:32 ` Jacob Keller
2 siblings, 1 reply; 12+ messages in thread
From: Michal Kubecek @ 2020-07-27 22:21 UTC (permalink / raw)
To: Jacob Keller; +Cc: Andrew Lunn, netdev, Jamie Gloudon
On Mon, Jul 27, 2020 at 02:47:00PM -0700, Jacob Keller wrote:
> The ethtool netlink API can send bitsets without an associated bitmask.
> These do not get displayed properly, because the dump_link_modes, and
> bitset_get_bit to not check whether the provided bitset is a NOMASK
> bitset. This results in the inability to display peer advertised link
> modes.
>
> The dump_link_modes and bitset_get_bit functions are designed so they
> can print either the values or the mask. For a nomask bitmap, this
> doesn't make sense. There is no mask.
>
> Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
> bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
> regardless of the request to display the mask or the value. For full
> size bitmaps, the set of provided bits indicates the valid values,
> without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
> bits without this attribute if nomask is set. This essentially means
> that dump_link_modes will treat a NOMASK bitset as having a mask
> equivalent to all of its set bits.
>
> For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
> bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
> For full bitmaps, if nomask is set, then always return true of the bit
> is in the set, rather than only if it provides an
> ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
> bits.
>
> This fixes display of link partner advertised fields when using the
> netlink API.
>
> Reported-by: Jamie Gloudon <jamie.gloudon@gmx.fr>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> netlink/bitset.c | 9 ++++++---
> netlink/settings.c | 8 +++++---
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/netlink/bitset.c b/netlink/bitset.c
> index 130bcdb5b52c..ba5d3ea77ff7 100644
> --- a/netlink/bitset.c
> +++ b/netlink/bitset.c
> @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
> DECLARE_ATTR_TB_INFO(bitset_tb);
> const struct nlattr *bits;
> const struct nlattr *bit;
> + bool nomask;
> int ret;
>
> *retptr = 0;
> @@ -57,8 +58,10 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
> if (ret < 0)
> goto err;
>
> - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> - bitset_tb[ETHTOOL_A_BITSET_VALUE];
> + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> +
> + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> + bitset_tb[ETHTOOL_A_BITSET_VALUE];
> if (bits) {
> const uint32_t *bitmap =
> (const uint32_t *)mnl_attr_get_payload(bits);
I don't like this part: (mask && nomask) is a situation which should
never happen as it would mean we are trying to get mask value from
a bitmap which does not any. In other words, if we ever see such
combination, it is a result of a bug either on ethtool side or on kernel
side.
Rather than silently returning something else than asked, we should
IMHO report an error. Which is easy in dump_link_modes() but it would
require rewriting bitset_get_bit().
Michal
> @@ -87,7 +90,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
>
> my_idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
> if (my_idx == idx)
> - return mask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
> + return mask || nomask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
> }
>
> return false;
> diff --git a/netlink/settings.c b/netlink/settings.c
> index 35ba2f5dd6d5..29557653336e 100644
> --- a/netlink/settings.c
> +++ b/netlink/settings.c
> @@ -280,9 +280,11 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
> const struct nlattr *bit;
> bool first = true;
> int prev = -2;
> + bool nomask;
> int ret;
>
> ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info);
> + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> bits = bitset_tb[ETHTOOL_A_BITSET_BITS];
> if (ret < 0)
> goto err_nonl;
> @@ -297,8 +299,8 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
> goto err_nonl;
> lm_strings = global_stringset(ETH_SS_LINK_MODES,
> nlctx->ethnl2_socket);
> - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> - bitset_tb[ETHTOOL_A_BITSET_VALUE];
> + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> + bitset_tb[ETHTOOL_A_BITSET_VALUE];
> ret = -EFAULT;
> if (!bits || !bitset_tb[ETHTOOL_A_BITSET_SIZE])
> goto err_nonl;
> @@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
> if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] ||
> !tb[ETHTOOL_A_BITSET_BIT_NAME])
> goto err;
> - if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
> + if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
> continue;
>
> idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 22:11 ` Andrew Lunn
@ 2020-07-27 22:22 ` Jacob Keller
2020-07-27 22:26 ` Michal Kubecek
1 sibling, 0 replies; 12+ messages in thread
From: Jacob Keller @ 2020-07-27 22:22 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Michal Kubecek, netdev, Jamie Gloudon
On 7/27/2020 3:11 PM, Andrew Lunn wrote:
> On Mon, Jul 27, 2020 at 02:47:00PM -0700, Jacob Keller wrote:
>> The ethtool netlink API can send bitsets without an associated bitmask.
>> These do not get displayed properly, because the dump_link_modes, and
>> bitset_get_bit to not check whether the provided bitset is a NOMASK
>> bitset. This results in the inability to display peer advertised link
>> modes.
>>
>> The dump_link_modes and bitset_get_bit functions are designed so they
>> can print either the values or the mask. For a nomask bitmap, this
>> doesn't make sense. There is no mask.
>>
>> Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
>> bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
>> regardless of the request to display the mask or the value. For full
>> size bitmaps, the set of provided bits indicates the valid values,
>> without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
>> bits without this attribute if nomask is set. This essentially means
>> that dump_link_modes will treat a NOMASK bitset as having a mask
>> equivalent to all of its set bits.
>>
>> For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
>> bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
>> For full bitmaps, if nomask is set, then always return true of the bit
>> is in the set, rather than only if it provides an
>> ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
>> bits.
>>
>> This fixes display of link partner advertised fields when using the
>> netlink API.
>
> Hi Jacob
>
> This is close
>
> Netlink
> Link partner advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Link partner advertised pause frame use: No
> Link partner advertised auto-negotiation: Yes
> Link partner advertised FEC modes: No
>
> IOCTL
> Link partner advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Link partner advertised pause frame use: No
> Link partner advertised auto-negotiation: Yes
> Link partner advertised FEC modes: Not reported
>
> So just the FEC modes differ.
>
> However, i don't think this was part of the original issue, so:
>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
>
> It would be nice to get the FEC modes fixed.
>
> Andrew
>
Yea, it looks like FEC modes is because FEC actually send a "None" flag,
as opposed to using an empty set as "none". I can follow-up this fix
with a change I think will resolve this as well.
Thanks,
Jake
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 22:11 ` Andrew Lunn
2020-07-27 22:22 ` Jacob Keller
@ 2020-07-27 22:26 ` Michal Kubecek
2020-07-27 22:32 ` Keller, Jacob E
1 sibling, 1 reply; 12+ messages in thread
From: Michal Kubecek @ 2020-07-27 22:26 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Jacob Keller, netdev, Jamie Gloudon
On Tue, Jul 28, 2020 at 12:11:04AM +0200, Andrew Lunn wrote:
> On Mon, Jul 27, 2020 at 02:47:00PM -0700, Jacob Keller wrote:
> > The ethtool netlink API can send bitsets without an associated bitmask.
> > These do not get displayed properly, because the dump_link_modes, and
> > bitset_get_bit to not check whether the provided bitset is a NOMASK
> > bitset. This results in the inability to display peer advertised link
> > modes.
> >
> > The dump_link_modes and bitset_get_bit functions are designed so they
> > can print either the values or the mask. For a nomask bitmap, this
> > doesn't make sense. There is no mask.
> >
> > Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
> > bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
> > regardless of the request to display the mask or the value. For full
> > size bitmaps, the set of provided bits indicates the valid values,
> > without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
> > bits without this attribute if nomask is set. This essentially means
> > that dump_link_modes will treat a NOMASK bitset as having a mask
> > equivalent to all of its set bits.
> >
> > For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
> > bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
> > For full bitmaps, if nomask is set, then always return true of the bit
> > is in the set, rather than only if it provides an
> > ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
> > bits.
> >
> > This fixes display of link partner advertised fields when using the
> > netlink API.
>
> Hi Jacob
>
> This is close
>
> Netlink
> Link partner advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Link partner advertised pause frame use: No
> Link partner advertised auto-negotiation: Yes
> Link partner advertised FEC modes: No
>
> IOCTL
> Link partner advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Link partner advertised pause frame use: No
> Link partner advertised auto-negotiation: Yes
> Link partner advertised FEC modes: Not reported
>
> So just the FEC modes differ.
This is a different issue, the last call to dump_link_modes() in
dump_peer_modes() should be
ret = dump_link_modes(nlctx, attr, false, LM_CLASS_FEC,
(third parameter needs to be false, not true).
Michal
>
> However, i don't think this was part of the original issue, so:
>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
>
> It would be nice to get the FEC modes fixed.
>
> Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 22:26 ` Michal Kubecek
@ 2020-07-27 22:32 ` Keller, Jacob E
2020-07-27 22:40 ` Michal Kubecek
0 siblings, 1 reply; 12+ messages in thread
From: Keller, Jacob E @ 2020-07-27 22:32 UTC (permalink / raw)
To: Michal Kubecek, Andrew Lunn; +Cc: netdev@vger.kernel.org, Jamie Gloudon
> -----Original Message-----
> From: Michal Kubecek <mkubecek@suse.cz>
> Sent: Monday, July 27, 2020 3:27 PM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; Jamie
> Gloudon <jamie.gloudon@gmx.fr>
> Subject: Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
>
> On Tue, Jul 28, 2020 at 12:11:04AM +0200, Andrew Lunn wrote:
> > On Mon, Jul 27, 2020 at 02:47:00PM -0700, Jacob Keller wrote:
> > > The ethtool netlink API can send bitsets without an associated bitmask.
> > > These do not get displayed properly, because the dump_link_modes, and
> > > bitset_get_bit to not check whether the provided bitset is a NOMASK
> > > bitset. This results in the inability to display peer advertised link
> > > modes.
> > >
> > > The dump_link_modes and bitset_get_bit functions are designed so they
> > > can print either the values or the mask. For a nomask bitmap, this
> > > doesn't make sense. There is no mask.
> > >
> > > Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For
> compact
> > > bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
> > > regardless of the request to display the mask or the value. For full
> > > size bitmaps, the set of provided bits indicates the valid values,
> > > without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
> > > bits without this attribute if nomask is set. This essentially means
> > > that dump_link_modes will treat a NOMASK bitset as having a mask
> > > equivalent to all of its set bits.
> > >
> > > For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
> > > bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
> > > For full bitmaps, if nomask is set, then always return true of the bit
> > > is in the set, rather than only if it provides an
> > > ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
> > > bits.
> > >
> > > This fixes display of link partner advertised fields when using the
> > > netlink API.
> >
> > Hi Jacob
> >
> > This is close
> >
> > Netlink
> > Link partner advertised link modes: 10baseT/Half 10baseT/Full
> > 100baseT/Half 100baseT/Full
> > 1000baseT/Full
> > Link partner advertised pause frame use: No
> > Link partner advertised auto-negotiation: Yes
> > Link partner advertised FEC modes: No
> >
> > IOCTL
> > Link partner advertised link modes: 10baseT/Half 10baseT/Full
> > 100baseT/Half 100baseT/Full
> > 1000baseT/Full
> > Link partner advertised pause frame use: No
> > Link partner advertised auto-negotiation: Yes
> > Link partner advertised FEC modes: Not reported
> >
> > So just the FEC modes differ.
>
> This is a different issue, the last call to dump_link_modes() in
> dump_peer_modes() should be
>
> ret = dump_link_modes(nlctx, attr, false, LM_CLASS_FEC,
>
> (third parameter needs to be false, not true).
>
> Michal
>
That's part of it, yea, but also it should use the string "Not Reported" instead of "No" I think?
> >
> > However, i don't think this was part of the original issue, so:
> >
> > Tested-by: Andrew Lunn <andrew@lunn.ch>
> >
> > It would be nice to get the FEC modes fixed.
> >
> > Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 22:21 ` Michal Kubecek
@ 2020-07-27 22:32 ` Jacob Keller
2020-07-27 22:53 ` Michal Kubecek
0 siblings, 1 reply; 12+ messages in thread
From: Jacob Keller @ 2020-07-27 22:32 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Andrew Lunn, netdev, Jamie Gloudon
On 7/27/2020 3:21 PM, Michal Kubecek wrote:
> On Mon, Jul 27, 2020 at 02:47:00PM -0700, Jacob Keller wrote:
>> The ethtool netlink API can send bitsets without an associated bitmask.
>> These do not get displayed properly, because the dump_link_modes, and
>> bitset_get_bit to not check whether the provided bitset is a NOMASK
>> bitset. This results in the inability to display peer advertised link
>> modes.
>>
>> The dump_link_modes and bitset_get_bit functions are designed so they
>> can print either the values or the mask. For a nomask bitmap, this
>> doesn't make sense. There is no mask.
>>
>> Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
>> bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
>> regardless of the request to display the mask or the value. For full
>> size bitmaps, the set of provided bits indicates the valid values,
>> without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
>> bits without this attribute if nomask is set. This essentially means
>> that dump_link_modes will treat a NOMASK bitset as having a mask
>> equivalent to all of its set bits.
>>
>> For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
>> bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
>> For full bitmaps, if nomask is set, then always return true of the bit
>> is in the set, rather than only if it provides an
>> ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
>> bits.
>>
>> This fixes display of link partner advertised fields when using the
>> netlink API.
>>
>> Reported-by: Jamie Gloudon <jamie.gloudon@gmx.fr>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>> netlink/bitset.c | 9 ++++++---
>> netlink/settings.c | 8 +++++---
>> 2 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/netlink/bitset.c b/netlink/bitset.c
>> index 130bcdb5b52c..ba5d3ea77ff7 100644
>> --- a/netlink/bitset.c
>> +++ b/netlink/bitset.c
>> @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
>> DECLARE_ATTR_TB_INFO(bitset_tb);
>> const struct nlattr *bits;
>> const struct nlattr *bit;
>> + bool nomask;
>> int ret;
>>
>> *retptr = 0;
>> @@ -57,8 +58,10 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
>> if (ret < 0)
>> goto err;
>>
>> - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
>> - bitset_tb[ETHTOOL_A_BITSET_VALUE];
>> + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
>> +
>> + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
>> + bitset_tb[ETHTOOL_A_BITSET_VALUE];
>> if (bits) {
>> const uint32_t *bitmap =
>> (const uint32_t *)mnl_attr_get_payload(bits);
>
> I don't like this part: (mask && nomask) is a situation which should
> never happen as it would mean we are trying to get mask value from
> a bitmap which does not any. In other words, if we ever see such
> combination, it is a result of a bug either on ethtool side or on kernel
> side.
>
> Rather than silently returning something else than asked, we should
> IMHO report an error. Which is easy in dump_link_modes() but it would
> require rewriting bitset_get_bit().
>
> Michal
The "mask" boolean is an indication that you want to print the mask for
a bitmap, rather than its value. I think treating a bitmap without a
predefined mask to have its mask be equivalent to its values is
reasonable. However, I could see the argument for not wanting this since
it is effectively a bug somewhere.
For dump_link_modes, it is trivial. If nomask is set, and mask is
requested, bail out of the function. It looks like we can also report an
error for the bitset_get_bit too. I'll take a look closer.
Thanks,
Jake
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 22:32 ` Keller, Jacob E
@ 2020-07-27 22:40 ` Michal Kubecek
0 siblings, 0 replies; 12+ messages in thread
From: Michal Kubecek @ 2020-07-27 22:40 UTC (permalink / raw)
To: Keller, Jacob E; +Cc: Andrew Lunn, netdev@vger.kernel.org, Jamie Gloudon
On Mon, Jul 27, 2020 at 10:32:03PM +0000, Keller, Jacob E wrote:
> > -----Original Message-----
> > From: Michal Kubecek <mkubecek@suse.cz>
> > Sent: Monday, July 27, 2020 3:27 PM
> > To: Andrew Lunn <andrew@lunn.ch>
> > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; Jamie
> > Gloudon <jamie.gloudon@gmx.fr>
> > Subject: Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
> >
> > On Tue, Jul 28, 2020 at 12:11:04AM +0200, Andrew Lunn wrote:
> > > Hi Jacob
> > >
> > > This is close
> > >
> > > Netlink
> > > Link partner advertised link modes: 10baseT/Half 10baseT/Full
> > > 100baseT/Half 100baseT/Full
> > > 1000baseT/Full
> > > Link partner advertised pause frame use: No
> > > Link partner advertised auto-negotiation: Yes
> > > Link partner advertised FEC modes: No
> > >
> > > IOCTL
> > > Link partner advertised link modes: 10baseT/Half 10baseT/Full
> > > 100baseT/Half 100baseT/Full
> > > 1000baseT/Full
> > > Link partner advertised pause frame use: No
> > > Link partner advertised auto-negotiation: Yes
> > > Link partner advertised FEC modes: Not reported
> > >
> > > So just the FEC modes differ.
> >
> > This is a different issue, the last call to dump_link_modes() in
> > dump_peer_modes() should be
> >
> > ret = dump_link_modes(nlctx, attr, false, LM_CLASS_FEC,
> >
> > (third parameter needs to be false, not true).
> >
> > Michal
> >
>
> That's part of it, yea, but also it should use the string "Not
> Reported" instead of "No" I think?
Right, both should be fixed.
Michal
>
> > >
> > > However, i don't think this was part of the original issue, so:
> > >
> > > Tested-by: Andrew Lunn <andrew@lunn.ch>
> > >
> > > It would be nice to get the FEC modes fixed.
> > >
> > > Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 22:32 ` Jacob Keller
@ 2020-07-27 22:53 ` Michal Kubecek
2020-07-27 23:18 ` Jacob Keller
0 siblings, 1 reply; 12+ messages in thread
From: Michal Kubecek @ 2020-07-27 22:53 UTC (permalink / raw)
To: Jacob Keller; +Cc: Andrew Lunn, netdev, Jamie Gloudon
On Mon, Jul 27, 2020 at 03:32:34PM -0700, Jacob Keller wrote:
> On 7/27/2020 3:21 PM, Michal Kubecek wrote:
> > On Mon, Jul 27, 2020 at 02:47:00PM -0700, Jacob Keller wrote:
> >> The ethtool netlink API can send bitsets without an associated bitmask.
> >> These do not get displayed properly, because the dump_link_modes, and
> >> bitset_get_bit to not check whether the provided bitset is a NOMASK
> >> bitset. This results in the inability to display peer advertised link
> >> modes.
> >>
> >> The dump_link_modes and bitset_get_bit functions are designed so they
> >> can print either the values or the mask. For a nomask bitmap, this
> >> doesn't make sense. There is no mask.
> >>
> >> Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
> >> bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
> >> regardless of the request to display the mask or the value. For full
> >> size bitmaps, the set of provided bits indicates the valid values,
> >> without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
> >> bits without this attribute if nomask is set. This essentially means
> >> that dump_link_modes will treat a NOMASK bitset as having a mask
> >> equivalent to all of its set bits.
> >>
> >> For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
> >> bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
> >> For full bitmaps, if nomask is set, then always return true of the bit
> >> is in the set, rather than only if it provides an
> >> ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
> >> bits.
> >>
> >> This fixes display of link partner advertised fields when using the
> >> netlink API.
> >>
> >> Reported-by: Jamie Gloudon <jamie.gloudon@gmx.fr>
> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> >> ---
> >> netlink/bitset.c | 9 ++++++---
> >> netlink/settings.c | 8 +++++---
> >> 2 files changed, 11 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/netlink/bitset.c b/netlink/bitset.c
> >> index 130bcdb5b52c..ba5d3ea77ff7 100644
> >> --- a/netlink/bitset.c
> >> +++ b/netlink/bitset.c
> >> @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
> >> DECLARE_ATTR_TB_INFO(bitset_tb);
> >> const struct nlattr *bits;
> >> const struct nlattr *bit;
> >> + bool nomask;
> >> int ret;
> >>
> >> *retptr = 0;
> >> @@ -57,8 +58,10 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
> >> if (ret < 0)
> >> goto err;
> >>
> >> - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> >> - bitset_tb[ETHTOOL_A_BITSET_VALUE];
> >> + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> >> +
> >> + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> >> + bitset_tb[ETHTOOL_A_BITSET_VALUE];
> >> if (bits) {
> >> const uint32_t *bitmap =
> >> (const uint32_t *)mnl_attr_get_payload(bits);
> >
> > I don't like this part: (mask && nomask) is a situation which should
> > never happen as it would mean we are trying to get mask value from
> > a bitmap which does not any. In other words, if we ever see such
> > combination, it is a result of a bug either on ethtool side or on kernel
> > side.
> >
> > Rather than silently returning something else than asked, we should
> > IMHO report an error. Which is easy in dump_link_modes() but it would
> > require rewriting bitset_get_bit().
> >
> > Michal
>
> The "mask" boolean is an indication that you want to print the mask for
> a bitmap, rather than its value. I think treating a bitmap without a
> predefined mask to have its mask be equivalent to its values is
> reasonable.
It depends on the context. In requests, value=0x1,mask=0x1 means "set
bit 0 and leave the rest untouched" while nomask bitmap with value=0x1
would mean "set bit 0 and clear the rest".
For kernel replies, it should be documented which variant is expected.
> However, I could see the argument for not wanting this since
> it is effectively a bug somewhere.
We actually had this kind of bug in FEC modes handling as we just found.
Michal
> For dump_link_modes, it is trivial. If nomask is set, and mask is
> requested, bail out of the function. It looks like we can also report an
> error for the bitset_get_bit too. I'll take a look closer.
>
> Thanks,
> Jake
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK
2020-07-27 22:53 ` Michal Kubecek
@ 2020-07-27 23:18 ` Jacob Keller
0 siblings, 0 replies; 12+ messages in thread
From: Jacob Keller @ 2020-07-27 23:18 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Andrew Lunn, netdev, Jamie Gloudon
On 7/27/2020 3:53 PM, Michal Kubecek wrote:
> On Mon, Jul 27, 2020 at 03:32:34PM -0700, Jacob Keller wrote:
>> On 7/27/2020 3:21 PM, Michal Kubecek wrote:
>>> On Mon, Jul 27, 2020 at 02:47:00PM -0700, Jacob Keller wrote:
>>>> The ethtool netlink API can send bitsets without an associated bitmask.
>>>> These do not get displayed properly, because the dump_link_modes, and
>>>> bitset_get_bit to not check whether the provided bitset is a NOMASK
>>>> bitset. This results in the inability to display peer advertised link
>>>> modes.
>>>>
>>>> The dump_link_modes and bitset_get_bit functions are designed so they
>>>> can print either the values or the mask. For a nomask bitmap, this
>>>> doesn't make sense. There is no mask.
>>>>
>>>> Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
>>>> bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
>>>> regardless of the request to display the mask or the value. For full
>>>> size bitmaps, the set of provided bits indicates the valid values,
>>>> without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
>>>> bits without this attribute if nomask is set. This essentially means
>>>> that dump_link_modes will treat a NOMASK bitset as having a mask
>>>> equivalent to all of its set bits.
>>>>
>>>> For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
>>>> bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
>>>> For full bitmaps, if nomask is set, then always return true of the bit
>>>> is in the set, rather than only if it provides an
>>>> ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
>>>> bits.
>>>>
>>>> This fixes display of link partner advertised fields when using the
>>>> netlink API.
>>>>
>>>> Reported-by: Jamie Gloudon <jamie.gloudon@gmx.fr>
>>>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>>>> ---
>>>> netlink/bitset.c | 9 ++++++---
>>>> netlink/settings.c | 8 +++++---
>>>> 2 files changed, 11 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/netlink/bitset.c b/netlink/bitset.c
>>>> index 130bcdb5b52c..ba5d3ea77ff7 100644
>>>> --- a/netlink/bitset.c
>>>> +++ b/netlink/bitset.c
>>>> @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
>>>> DECLARE_ATTR_TB_INFO(bitset_tb);
>>>> const struct nlattr *bits;
>>>> const struct nlattr *bit;
>>>> + bool nomask;
>>>> int ret;
>>>>
>>>> *retptr = 0;
>>>> @@ -57,8 +58,10 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
>>>> if (ret < 0)
>>>> goto err;
>>>>
>>>> - bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
>>>> - bitset_tb[ETHTOOL_A_BITSET_VALUE];
>>>> + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
>>>> +
>>>> + bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
>>>> + bitset_tb[ETHTOOL_A_BITSET_VALUE];
>>>> if (bits) {
>>>> const uint32_t *bitmap =
>>>> (const uint32_t *)mnl_attr_get_payload(bits);
>>>
>>> I don't like this part: (mask && nomask) is a situation which should
>>> never happen as it would mean we are trying to get mask value from
>>> a bitmap which does not any. In other words, if we ever see such
>>> combination, it is a result of a bug either on ethtool side or on kernel
>>> side.
>>>
>>> Rather than silently returning something else than asked, we should
>>> IMHO report an error. Which is easy in dump_link_modes() but it would
>>> require rewriting bitset_get_bit().
>>>
>>> Michal
>>
>> The "mask" boolean is an indication that you want to print the mask for
>> a bitmap, rather than its value. I think treating a bitmap without a
>> predefined mask to have its mask be equivalent to its values is
>> reasonable.
>
> It depends on the context. In requests, value=0x1,mask=0x1 means "set
> bit 0 and leave the rest untouched" while nomask bitmap with value=0x1
> would mean "set bit 0 and clear the rest".
>
> For kernel replies, it should be documented which variant is expected.
>
Right, I was mostly referring to the reply side of things. I've updated
the patch in v2 to explicitly reject trying to print the mask of a NO
MASK bitset.
Thanks,
Jake
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-07-27 23:18 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-27 21:47 [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK Jacob Keller
2020-07-27 21:51 ` Jacob Keller
2020-07-27 22:02 ` Jamie Gloudon
2020-07-27 22:11 ` Andrew Lunn
2020-07-27 22:22 ` Jacob Keller
2020-07-27 22:26 ` Michal Kubecek
2020-07-27 22:32 ` Keller, Jacob E
2020-07-27 22:40 ` Michal Kubecek
2020-07-27 22:21 ` Michal Kubecek
2020-07-27 22:32 ` Jacob Keller
2020-07-27 22:53 ` Michal Kubecek
2020-07-27 23:18 ` Jacob Keller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).