All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michal Kubecek <mkubecek@suse.cz>
To: Andrew Lunn <andrew@lunn.ch>
Cc: David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] ethtool: add and use message type for tunnel info reply
Date: Thu, 17 Sep 2020 23:29:19 +0200	[thread overview]
Message-ID: <20200917212919.3n6f3zdegjeyhfud@lion.mk-sys.cz> (raw)
In-Reply-To: <20200917014151.GK3463198@lunn.ch>

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

  reply	other threads:[~2020-09-17 21:29 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2020-09-17 21:42     ` Andrew Lunn
2020-09-17 23:43 ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200917212919.3n6f3zdegjeyhfud@lion.mk-sys.cz \
    --to=mkubecek@suse.cz \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.