[PATCH net] ethtool: Fix link extended state for big endian

[PATCH net] ethtool: Fix link extended state for big endian

[Moshe Tal]

The link extended sub-states are assigned as enum that is an integer
size but read from a union as u8, this is working for small values on
little endian systems but for big endian this always give 0. Fix the
variable in the union to match the enum size.

Fixes: ecc31c60240b ("ethtool: Add link extended state")
Signed-off-by: Moshe Tal <moshet@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Tested-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Gal Pressman <gal@nvidia.com>
Reviewed-by: Amit Cohen <amcohen@nvidia.com>
---
 include/linux/ethtool.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index a26f37a27167..11efc45de66a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -111,7 +111,7 @@ struct ethtool_link_ext_state_info {
 		enum ethtool_link_ext_substate_bad_signal_integrity bad_signal_integrity;
 		enum ethtool_link_ext_substate_cable_issue cable_issue;
 		enum ethtool_link_ext_substate_module module;
-		u8 __link_ext_substate;
+		u32 __link_ext_substate;
 	};
 };
 
-- 
2.21.0

Re: [PATCH net] ethtool: Fix link extended state for big endian

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu, 20 Jan 2022 11:55:50 +0200 you wrote:
> The link extended sub-states are assigned as enum that is an integer
> size but read from a union as u8, this is working for small values on
> little endian systems but for big endian this always give 0. Fix the
> variable in the union to match the enum size.
> 
> Fixes: ecc31c60240b ("ethtool: Add link extended state")
> Signed-off-by: Moshe Tal <moshet@nvidia.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Tested-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Gal Pressman <gal@nvidia.com>
> Reviewed-by: Amit Cohen <amcohen@nvidia.com>
> 
> [...]

Here is the summary with links:
  - [net] ethtool: Fix link extended state for big endian
    https://git.kernel.org/netdev/net/c/e2f08207c558

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] ethtool: Fix link extended state for big endian

[Andrew Lunn]

On Thu, Jan 20, 2022 at 11:55:50AM +0200, Moshe Tal wrote:
> The link extended sub-states are assigned as enum that is an integer
> size but read from a union as u8, this is working for small values on
> little endian systems but for big endian this always give 0. Fix the
> variable in the union to match the enum size.
> 
> Fixes: ecc31c60240b ("ethtool: Add link extended state")
> Signed-off-by: Moshe Tal <moshet@nvidia.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Tested-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Gal Pressman <gal@nvidia.com>
> Reviewed-by: Amit Cohen <amcohen@nvidia.com>
> ---
>  include/linux/ethtool.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index a26f37a27167..11efc45de66a 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -111,7 +111,7 @@ struct ethtool_link_ext_state_info {
>  		enum ethtool_link_ext_substate_bad_signal_integrity bad_signal_integrity;
>  		enum ethtool_link_ext_substate_cable_issue cable_issue;
>  		enum ethtool_link_ext_substate_module module;
> -		u8 __link_ext_substate;
> +		u32 __link_ext_substate;

Not my area of expertise, but:

static int linkstate_reply_size(const struct ethnl_req_info *req_base,
                                const struct ethnl_reply_data *reply_base)
{
        struct linkstate_reply_data *data = LINKSTATE_REPDATA(reply_base);
        int len;

       if (data->ethtool_link_ext_state_info.__link_ext_substate)
                len += nla_total_size(sizeof(u8)); /* LINKSTATE_EXT_SUBSTATE */

and

static int linkstate_fill_reply(struct sk_buff *skb,
                                const struct ethnl_req_info *req_base,
                                const struct ethnl_reply_data *reply_base)
{

		if (data->ethtool_link_ext_state_info.__link_ext_substate &&
                    nla_put_u8(skb, ETHTOOL_A_LINKSTATE_EXT_SUBSTATE,
                               data->ethtool_link_ext_state_info.__link_ext_substate))
                        return -EMSGSIZE;

This seems to suggest it is a u8, not a u32.

I guess i don't understand something here...

  Andrew

Re: [PATCH net] ethtool: Fix link extended state for big endian

[Moshe Tal]

On 20/01/2022 16:43, Andrew Lunn wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Jan 20, 2022 at 11:55:50AM +0200, Moshe Tal wrote:
>> The link extended sub-states are assigned as enum that is an integer
>> size but read from a union as u8, this is working for small values on
>> little endian systems but for big endian this always give 0. Fix the
>> variable in the union to match the enum size.
>>
>> Fixes: ecc31c60240b ("ethtool: Add link extended state")
>> Signed-off-by: Moshe Tal <moshet@nvidia.com>
>> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
>> Tested-by: Ido Schimmel <idosch@nvidia.com>
>> Reviewed-by: Gal Pressman <gal@nvidia.com>
>> Reviewed-by: Amit Cohen <amcohen@nvidia.com>
>> ---
>>   include/linux/ethtool.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index a26f37a27167..11efc45de66a 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -111,7 +111,7 @@ struct ethtool_link_ext_state_info {
>>                enum ethtool_link_ext_substate_bad_signal_integrity bad_signal_integrity;
>>                enum ethtool_link_ext_substate_cable_issue cable_issue;
>>                enum ethtool_link_ext_substate_module module;
>> -             u8 __link_ext_substate;
>> +             u32 __link_ext_substate;
> 
> Not my area of expertise, but:
> 
> static int linkstate_reply_size(const struct ethnl_req_info *req_base,
>                                  const struct ethnl_reply_data *reply_base)
> {
>          struct linkstate_reply_data *data = LINKSTATE_REPDATA(reply_base);
>          int len;
> 
>         if (data->ethtool_link_ext_state_info.__link_ext_substate)
>                  len += nla_total_size(sizeof(u8)); /* LINKSTATE_EXT_SUBSTATE */
> 
> and
> 
> static int linkstate_fill_reply(struct sk_buff *skb,
>                                  const struct ethnl_req_info *req_base,
>                                  const struct ethnl_reply_data *reply_base)
> {
> 
>                  if (data->ethtool_link_ext_state_info.__link_ext_substate &&
>                      nla_put_u8(skb, ETHTOOL_A_LINKSTATE_EXT_SUBSTATE,
>                                 data->ethtool_link_ext_state_info.__link_ext_substate))
>                          return -EMSGSIZE;
> 
> This seems to suggest it is a u8, not a u32.
> 
> I guess i don't understand something here...
> 
>    Andrew

The Netlink message was defined to only pass u8 and we can't change the 
message format without causing incompatibility issues.
So, we are assuming that values will be under 255.

Still, the compiler is storing enum as int, this isn't matter what the 
size of the other members of the union.
If it will be read into u8 - on BE systems the MSB will be read and so 
it will always pass a zero.
So this was solved by reading it as u32. Later, when the compiler will 
pass it to the function as u8 parameter, it will take the right part - 
the LSB on either system.

Reading enum by u8 from a union:
==================================
|             enum               |
==================================
|  u8   |
==================================
|  MSB  |        |       |  LSB  | On BE systems
==================================
|  LSB  |        |       |  MSB  | On LE systems
==================================

Converting u32 to u8:
  =========       =======
| u32 LSB |  => |   u8  |  On all the systems
  =========       =======

Re: [PATCH net] ethtool: Fix link extended state for big endian

[Andrew Lunn]

> The Netlink message was defined to only pass u8 and we can't change the 
> message format without causing incompatibility issues.
> So, we are assuming that values will be under 255.
> 
> Still, the compiler is storing enum as int, this isn't matter what the 
> size of the other members of the union.
> If it will be read into u8 - on BE systems the MSB will be read and so 
> it will always pass a zero.

It sounds to me like the type system is being bypassed somewhere. If
the compiler knows we are assigning an emum to a u8, it should perform
a cast and i would expect it to get it correct, independent of big or
little endian. When that u8 is assigned back to an enum, the compiler
should do another cast, and everything works out.

I assume there are no compiler warnings? The enum -> u8 is an
assignment to a smaller type, which is something you sometimes see
compilers warn about. So it might be there is an explicit cast
somewhere?

But you are saying this is not actually happening, the wrong end is
being discarded. Should we not actually be trying to find where the
type system is broken?

Maybe we as a community already do know, and i'm just ignorant. But
maybe there is nothing we can do about it, this being a KAPI? But then
the commit message should actually explain this, to avoid questions
like mine, and to spread knowledge in the community.

	Andrew

Re: [PATCH net] ethtool: Fix link extended state for big endian

[Petr Machata]

Andrew Lunn <andrew@lunn.ch> writes:

>> The Netlink message was defined to only pass u8 and we can't change the 
>> message format without causing incompatibility issues.
>> So, we are assuming that values will be under 255.
>> 
>> Still, the compiler is storing enum as int, this isn't matter what the 
>> size of the other members of the union.
>> If it will be read into u8 - on BE systems the MSB will be read and so 
>> it will always pass a zero.
>
> It sounds to me like the type system is being bypassed somewhere. If
> the compiler knows we are assigning an emum to a u8, it should perform
> a cast and i would expect it to get it correct, independent of big or
> little endian. When that u8 is assigned back to an enum, the compiler
> should do another cast, and everything works out.
>
> I assume there are no compiler warnings? The enum -> u8 is an
> assignment to a smaller type, which is something you sometimes see
> compilers warn about. So it might be there is an explicit cast
> somewhere?

The only cast I'm aware of is the implicit cast in the call to
nla_put_u8(). The C standard has this to say about the conversions:

    When a value with integer type is converted to another integer type
    other than _Bool, if the value can be represented by the new type,
    it is unchanged.

There's more verbiage about what happens when it doesn't fit, but we are
on a happy path here.

I'm not especially well-versed in various warnings GCC gives, but I
think it only warns about expressions that involve literals. So
assigning an overlarge literal to a narrow type, or literal-only
expressions that would overflow the type of the expression.

> But you are saying this is not actually happening, the wrong end is
> being discarded. Should we not actually be trying to find where the
> type system is broken?

The mistake was in the union. Both the u8 and the enums are laid out to
start on the same address. When an enumerator is stored into an enum
field on a little-endian machine, the LSB is stored first, and that's
where u8 is laid out in the enum as well, so you get the enumerator
value back when reading the u8.

On big-endian machines however, the byte that u8 is laid out on is the
MSB, which is a zero in our case.

The underlying type for an enum is an integer. So de iure the fix should
have been u8->int, not u8->u32. Practically I suspect it does not
matter.