* [PATCH net] ethtool: add and use message type for tunnel info reply
@ 2020-09-16 23:04 Michal Kubecek
2020-09-17 0:36 ` Jakub Kicinski
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Michal Kubecek @ 2020-09-16 23:04 UTC (permalink / raw)
To: David Miller, Jakub Kicinski, netdev; +Cc: linux-kernel
Tunnel offload info code uses ETHTOOL_MSG_TUNNEL_INFO_GET message type (cmd
field in genetlink header) for replies to tunnel info netlink request, i.e.
the same value as the request have. This is a problem because we are using
two separate enums for userspace to kernel and kernel to userspace message
types so that this ETHTOOL_MSG_TUNNEL_INFO_GET (28) collides with
ETHTOOL_MSG_CABLE_TEST_TDR_NTF which is what message type 28 means for
kernel to userspace messages.
As the tunnel info request reached mainline in 5.9 merge window, we should
still be able to fix the reply message type without breaking backward
compatibility.
Fixes: c7d759eb7b12 ("ethtool: add tunnel info interface")
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
Documentation/networking/ethtool-netlink.rst | 3 +++
include/uapi/linux/ethtool_netlink.h | 1 +
net/ethtool/tunnels.c | 4 ++--
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index d53bcb31645a..b5a79881551f 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -206,6 +206,7 @@ Userspace to kernel:
``ETHTOOL_MSG_TSINFO_GET`` get timestamping info
``ETHTOOL_MSG_CABLE_TEST_ACT`` action start cable test
``ETHTOOL_MSG_CABLE_TEST_TDR_ACT`` action start raw TDR cable test
+ ``ETHTOOL_MSG_TUNNEL_INFO_GET`` get tunnel offload info
===================================== ================================
Kernel to userspace:
@@ -239,6 +240,7 @@ Kernel to userspace:
``ETHTOOL_MSG_TSINFO_GET_REPLY`` timestamping info
``ETHTOOL_MSG_CABLE_TEST_NTF`` Cable test results
``ETHTOOL_MSG_CABLE_TEST_TDR_NTF`` Cable test TDR results
+ ``ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY`` tunnel offload info
===================================== =================================
``GET`` requests are sent by userspace applications to retrieve device
@@ -1363,4 +1365,5 @@ are netlink only.
``ETHTOOL_SFECPARAM`` n/a
n/a ''ETHTOOL_MSG_CABLE_TEST_ACT''
n/a ''ETHTOOL_MSG_CABLE_TEST_TDR_ACT''
+ n/a ``ETHTOOL_MSG_TUNNEL_INFO_GET``
=================================== =====================================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 5dcd24cb33ea..72ba36be9655 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -79,6 +79,7 @@ enum {
ETHTOOL_MSG_TSINFO_GET_REPLY,
ETHTOOL_MSG_CABLE_TEST_NTF,
ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
+ ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
/* add new constants above here */
__ETHTOOL_MSG_KERNEL_CNT,
diff --git a/net/ethtool/tunnels.c b/net/ethtool/tunnels.c
index 84f23289475b..d93bf2da0f34 100644
--- a/net/ethtool/tunnels.c
+++ b/net/ethtool/tunnels.c
@@ -200,7 +200,7 @@ int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info)
reply_len = ret + ethnl_reply_header_size();
rskb = ethnl_reply_init(reply_len, req_info.dev,
- ETHTOOL_MSG_TUNNEL_INFO_GET,
+ ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
ETHTOOL_A_TUNNEL_INFO_HEADER,
info, &reply_payload);
if (!rskb) {
@@ -273,7 +273,7 @@ int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
goto cont;
ehdr = ethnl_dump_put(skb, cb,
- ETHTOOL_MSG_TUNNEL_INFO_GET);
+ ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY);
if (!ehdr) {
ret = -EMSGSIZE;
goto out;
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: add and use message type for tunnel info reply
2020-09-16 23:04 [PATCH net] ethtool: add and use message type for tunnel info reply Michal Kubecek
@ 2020-09-17 0:36 ` Jakub Kicinski
2020-09-17 1:41 ` Andrew Lunn
2020-09-17 23:43 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2020-09-17 0:36 UTC (permalink / raw)
To: Michal Kubecek; +Cc: David Miller, netdev, linux-kernel
On Thu, 17 Sep 2020 01:04:10 +0200 (CEST) Michal Kubecek wrote:
> Tunnel offload info code uses ETHTOOL_MSG_TUNNEL_INFO_GET message type (cmd
> field in genetlink header) for replies to tunnel info netlink request, i.e.
> the same value as the request have. This is a problem because we are using
> two separate enums for userspace to kernel and kernel to userspace message
> types so that this ETHTOOL_MSG_TUNNEL_INFO_GET (28) collides with
> ETHTOOL_MSG_CABLE_TEST_TDR_NTF which is what message type 28 means for
> kernel to userspace messages.
>
> As the tunnel info request reached mainline in 5.9 merge window, we should
> still be able to fix the reply message type without breaking backward
> compatibility.
>
> Fixes: c7d759eb7b12 ("ethtool: add tunnel info interface")
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
Ouch, sorry & thanks!
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: add and use message type for tunnel info reply
2020-09-16 23:04 [PATCH net] ethtool: add and use message type for tunnel info reply Michal Kubecek
2020-09-17 0:36 ` Jakub Kicinski
@ 2020-09-17 1:41 ` Andrew Lunn
2020-09-17 21:29 ` Michal Kubecek
2020-09-17 23:43 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2020-09-17 1:41 UTC (permalink / raw)
To: Michal Kubecek; +Cc: David Miller, Jakub Kicinski, netdev, linux-kernel
On Thu, Sep 17, 2020 at 01:04:10AM +0200, Michal Kubecek wrote:
> Tunnel offload info code uses ETHTOOL_MSG_TUNNEL_INFO_GET message type (cmd
> field in genetlink header) for replies to tunnel info netlink request, i.e.
> the same value as the request have. This is a problem because we are using
> two separate enums for userspace to kernel and kernel to userspace message
> types so that this ETHTOOL_MSG_TUNNEL_INFO_GET (28) collides with
> ETHTOOL_MSG_CABLE_TEST_TDR_NTF which is what message type 28 means for
> kernel to userspace messages.
>
> rskb = ethnl_reply_init(reply_len, req_info.dev,
> - ETHTOOL_MSG_TUNNEL_INFO_GET,
> + ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
> ETHTOOL_A_TUNNEL_INFO_HEADER,
> info, &reply_payload);
Michal
Maybe it would make sense to change the two enums from anonymous to
tagged. We can then make ethnl_reply_init() do type checking and
hopefully catch such problems earlier?
I just wonder if we then run into ABI problems?
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: add and use message type for tunnel info reply
2020-09-17 1:41 ` Andrew Lunn
@ 2020-09-17 21:29 ` Michal Kubecek
2020-09-17 21:42 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Michal Kubecek @ 2020-09-17 21:29 UTC (permalink / raw)
To: Andrew Lunn; +Cc: David Miller, Jakub Kicinski, netdev, linux-kernel
On Thu, Sep 17, 2020 at 03:41:51AM +0200, Andrew Lunn wrote:
> On Thu, Sep 17, 2020 at 01:04:10AM +0200, Michal Kubecek wrote:
> > Tunnel offload info code uses ETHTOOL_MSG_TUNNEL_INFO_GET message type (cmd
> > field in genetlink header) for replies to tunnel info netlink request, i.e.
> > the same value as the request have. This is a problem because we are using
> > two separate enums for userspace to kernel and kernel to userspace message
> > types so that this ETHTOOL_MSG_TUNNEL_INFO_GET (28) collides with
> > ETHTOOL_MSG_CABLE_TEST_TDR_NTF which is what message type 28 means for
> > kernel to userspace messages.
>
> >
> > rskb = ethnl_reply_init(reply_len, req_info.dev,
> > - ETHTOOL_MSG_TUNNEL_INFO_GET,
> > + ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
> > ETHTOOL_A_TUNNEL_INFO_HEADER,
> > info, &reply_payload);
>
> Michal
>
> Maybe it would make sense to change the two enums from anonymous to
> tagged. We can then make ethnl_reply_init() do type checking and
> hopefully catch such problems earlier?
This sounds like a good idea, it should prevent similar mistakes.
> I just wonder if we then run into ABI problems?
I don't think there would be a problem with ABI, binary representation
of the values shouldn't change and ethnl_reply_init() is not even
exported; ethtool_notify() which would also benefit from stricter type
checking is exported but it is still kernel API which is not guaranteed
to be stable.
On the other hand, the enums are part of userspace API so I better take
a closer look to make sure we don't run into some trouble there.
Michal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: add and use message type for tunnel info reply
2020-09-17 21:29 ` Michal Kubecek
@ 2020-09-17 21:42 ` Andrew Lunn
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2020-09-17 21:42 UTC (permalink / raw)
To: Michal Kubecek; +Cc: David Miller, Jakub Kicinski, netdev, linux-kernel
> On the other hand, the enums are part of userspace API so I better take
> a closer look to make sure we don't run into some trouble there.
Hi Michal
Yes, that is what i was thinking about. But i guess you can pass a
tagged enum to a function expecting an int and the compiler will
silently cast it. Which is what we should have at the moment. So i'm
expecting it to be O.K.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: add and use message type for tunnel info reply
2020-09-16 23:04 [PATCH net] ethtool: add and use message type for tunnel info reply Michal Kubecek
2020-09-17 0:36 ` Jakub Kicinski
2020-09-17 1:41 ` Andrew Lunn
@ 2020-09-17 23:43 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2020-09-17 23:43 UTC (permalink / raw)
To: mkubecek; +Cc: kuba, netdev, linux-kernel
From: Michal Kubecek <mkubecek@suse.cz>
Date: Thu, 17 Sep 2020 01:04:10 +0200 (CEST)
> Tunnel offload info code uses ETHTOOL_MSG_TUNNEL_INFO_GET message type (cmd
> field in genetlink header) for replies to tunnel info netlink request, i.e.
> the same value as the request have. This is a problem because we are using
> two separate enums for userspace to kernel and kernel to userspace message
> types so that this ETHTOOL_MSG_TUNNEL_INFO_GET (28) collides with
> ETHTOOL_MSG_CABLE_TEST_TDR_NTF which is what message type 28 means for
> kernel to userspace messages.
>
> As the tunnel info request reached mainline in 5.9 merge window, we should
> still be able to fix the reply message type without breaking backward
> compatibility.
>
> Fixes: c7d759eb7b12 ("ethtool: add tunnel info interface")
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
Applied, thank you.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-17 23:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 23:04 [PATCH net] ethtool: add and use message type for tunnel info reply Michal Kubecek
2020-09-17 0:36 ` Jakub Kicinski
2020-09-17 1:41 ` Andrew Lunn
2020-09-17 21:29 ` Michal Kubecek
2020-09-17 21:42 ` Andrew Lunn
2020-09-17 23:43 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.