netdev - Re: [PATCH net-next] net/core: Replace driver version to be kernel version

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: <20200126133353.77f5cb7e@cakuba>
Date:   Sun, 26 Jan 2020 13:33:53 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Shannon Nelson <snelson@...sando.io>,
        "David S . Miller" <davem@...emloft.net>,
        Michal Kalderon <michal.kalderon@...vell.com>,
        linux-netdev <netdev@...r.kernel.org>,
        RDMA mailing list <linux-rdma@...r.kernel.org>
Subject: Re: [PATCH net-next] net/core: Replace driver version to be kernel
 version

On Sun, 26 Jan 2020 23:08:50 +0200, Leon Romanovsky wrote:
> On Sun, Jan 26, 2020 at 12:49:57PM -0800, Jakub Kicinski wrote:
> > On Sun, 26 Jan 2020 21:41:10 +0200, Leon Romanovsky wrote:  
> > > > This will end up affecting out-of-tree drivers as well, where it is useful
> > > > to know what the version number is, most especially since it is different
> > > > from what the kernel provided driver is.  How else are we to get this
> > > > information out to the user?  If this feature gets squashed, we'll end up
> > > > having to abuse some other mechanism so we can get the live information from
> > > > the driver, and probably each vendor will find a different way to sneak it
> > > > out, giving us more chaos than where we started.  At least the ethtool
> > > > version field is a known and consistent place for the version info.

> > Shannon does have a point that out of tree drivers still make use of
> > this field. Perhaps it would be a more suitable first step to print the
> > kernel version as default and add a comment saying upstream modules
> > shouldn't overwrite it (perhaps one day CI can catch new violators).  
> 
> Shannon proposed to remove this field and it was me who said no :)

Obviously, we can't remove fields from UAPI structs.

> My plan is to overwrite ->version, delete all users and add
> WARN_ONEC(strcpy(..->version_)...) inside net/ethtool/ to catch
> abusers.

What I was thinking just now was: initialize ->version to utsname
before drivers are called, delete all upstream users, add a coccicheck
for upstream drivers which try to report the version.

> > The NFP reports the git hash of the driver source plus the string
> > "(oot)" for out-of-tree:
> >
> > https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L297
> > https://github.com/Netronome/nfp-drv-kmods/blob/master/src/Kbuild#L315  
> 
> I was inspired by upstream code.
> https://elixir.bootlin.com/linux/v5.5-rc7/source/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c#L184

Right, upstream nfp reports kernel version (both in modinfo and ethtool)
GitHub/compat/backport/out-of-tree reports kernel version in which the
code was expected to appear in modinfo:

https://github.com/Netronome/nfp-drv-kmods/commit/7ec15c47caf5dbdf1f9806410535ad5b7373ec34#diff-492d7fa4004d885a38cfa889ed1adbe7L1284

And git hash of the driver source plus out of tree marker in ethtool.

That means it's out-of-tree driver which has to carry the extra code
and require extra feeding. As backport should IMHO.

> > > Leaving to deal with driver version to vendors is not an option too,
> > > because they prove for more than once that they are not capable to
> > > define user visible interfaces. It comes due to their natural believe
> > > that their company is alone in the world and user visible interface
> > > should be suitable for them only.
> > >
> > > It is already impossible for users to distinguish properly versions
> > > of different vendors, because they use arbitrary strings with some
> > > numbers.  
> >
> > That is true. But reporting the kernel version without even as much as
> > in-tree/out-of-tree indication makes the field entirely meaningless.  
> 
> The long-standing policy in kernel that we don't really care about
> out-of-tree code.

Yeah... we all know it's not that simple :)

The in-tree driver versions are meaningless and cause annoying churn
when people arbitrarily bump them. If we can get people to stop doing
that we'll be happy, that's all there is to it. 

Out of tree the field is useful, so we don't have to take it away just
as a matter of principle. If we can't convince people to stop bringing
the versions into the tree that'll be another story...