netdev - Re: [PATCH net] ethtool: add and use message type for tunnel info reply

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200917212919.3n6f3zdegjeyhfud@lion.mk-sys.cz>
Date:   Thu, 17 Sep 2020 23:29:19 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] ethtool: add and use message type for tunnel info
 reply

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