* [PATCH net] ethtool: Fix link extended state for big endian @ 2022-01-20 9:55 Moshe Tal 2022-01-20 11:40 ` patchwork-bot+netdevbpf 2022-01-20 14:43 ` Andrew Lunn 0 siblings, 2 replies; 6+ messages in thread From: Moshe Tal @ 2022-01-20 9:55 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski, Ido Schimmel Cc: netdev, Amit Cohen, Jiri Pirko, Petr Machata, Gal Pressman, Tariq Toukan, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: Fix link extended state for big endian 2022-01-20 9:55 [PATCH net] ethtool: Fix link extended state for big endian Moshe Tal @ 2022-01-20 11:40 ` patchwork-bot+netdevbpf 2022-01-20 14:43 ` Andrew Lunn 1 sibling, 0 replies; 6+ messages in thread From: patchwork-bot+netdevbpf @ 2022-01-20 11:40 UTC (permalink / raw) To: Moshe Tal; +Cc: davem, kuba, idosch, netdev, amcohen, jiri, petrm, gal, tariqt 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: Fix link extended state for big endian 2022-01-20 9:55 [PATCH net] ethtool: Fix link extended state for big endian Moshe Tal 2022-01-20 11:40 ` patchwork-bot+netdevbpf @ 2022-01-20 14:43 ` Andrew Lunn 2022-01-23 9:08 ` Moshe Tal 1 sibling, 1 reply; 6+ messages in thread From: Andrew Lunn @ 2022-01-20 14:43 UTC (permalink / raw) To: Moshe Tal Cc: David S . Miller, Jakub Kicinski, Ido Schimmel, netdev, Amit Cohen, Jiri Pirko, Petr Machata, Gal Pressman, Tariq Toukan 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: Fix link extended state for big endian 2022-01-20 14:43 ` Andrew Lunn @ 2022-01-23 9:08 ` Moshe Tal 2022-01-23 15:19 ` Andrew Lunn 0 siblings, 1 reply; 6+ messages in thread From: Moshe Tal @ 2022-01-23 9:08 UTC (permalink / raw) To: Andrew Lunn Cc: David S . Miller, Jakub Kicinski, Ido Schimmel, netdev, Amit Cohen, Jiri Pirko, Petr Machata, Gal Pressman, Tariq Toukan 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 ========= ======= ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: Fix link extended state for big endian 2022-01-23 9:08 ` Moshe Tal @ 2022-01-23 15:19 ` Andrew Lunn 2022-01-24 11:58 ` Petr Machata 0 siblings, 1 reply; 6+ messages in thread From: Andrew Lunn @ 2022-01-23 15:19 UTC (permalink / raw) To: Moshe Tal Cc: David S . Miller, Jakub Kicinski, Ido Schimmel, netdev, Amit Cohen, Jiri Pirko, Petr Machata, Gal Pressman, Tariq Toukan > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: Fix link extended state for big endian 2022-01-23 15:19 ` Andrew Lunn @ 2022-01-24 11:58 ` Petr Machata 0 siblings, 0 replies; 6+ messages in thread From: Petr Machata @ 2022-01-24 11:58 UTC (permalink / raw) To: Andrew Lunn Cc: Moshe Tal, David S . Miller, Jakub Kicinski, Ido Schimmel, netdev, Amit Cohen, Jiri Pirko, Petr Machata, Gal Pressman, Tariq Toukan 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-24 16:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-20 9:55 [PATCH net] ethtool: Fix link extended state for big endian Moshe Tal 2022-01-20 11:40 ` patchwork-bot+netdevbpf 2022-01-20 14:43 ` Andrew Lunn 2022-01-23 9:08 ` Moshe Tal 2022-01-23 15:19 ` Andrew Lunn 2022-01-24 11:58 ` Petr Machata
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).