netdev - Re: [RFCv2 net-next 000/167] net: extend the netdev_features

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: <b335852ecaba3c86d1745b5021bb500798fc843b.camel@kernel.org>
Date:   Mon, 04 Oct 2021 15:30:21 -0700
From:   Saeed Mahameed <saeed@...nel.org>
To:     Andrew Lunn <andrew@...n.ch>,
        Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     Jian Shen <shenjian15@...wei.com>, davem@...emloft.net,
        kuba@...nel.org, hkallweit1@...il.com, netdev@...r.kernel.org,
        linuxarm@...neuler.org
Subject: Re: [RFCv2 net-next 000/167] net: extend the netdev_features_t

On Mon, 2021-10-04 at 16:59 +0200, Andrew Lunn wrote:
> On Fri, Oct 01, 2021 at 05:17:10PM +0200, Alexander Lobakin wrote:
> > From: Jian Shen <shenjian15@...wei.com>
> > Date: Wed, 29 Sep 2021 23:50:47 +0800
> > 
> > Hi,
> > 
> > > For the prototype of netdev_features_t is u64, and the number
> > > of netdevice feature bits is 64 now. So there is no space to
> > > introduce new feature bit.
> > > 
> > > This patchset try to solve it by change the prototype of
> > > netdev_features_t from u64 to bitmap. With this change,
> > > it's necessary to introduce a set of bitmap operation helpers
> > > for netdev features. Meanwhile, the functions which use
> > > netdev_features_t as return value are also need to be changed,
> > > return the result as an output parameter.
> > > 
> > > With above changes, it will affect hundreds of files, and all the
> > > nic drivers. To make it easy to be reviewed, split the changes
> > > to 167 patches to 5 parts.
> > 
> > If you leave the current feature field set (features, hw_features
> > etc.) as is and just add new ones as bitmaps -- I mean, to place
> > only newly added features there -- you won't have to change this in
> > hundreds of drivers.
> 
> That makes things messy for the future. Two different ways to express
> the same thing. And it is a trap waiting for developers to fall
> into. Is this a new feature or an old feature bit? Should i add it to
> the old or new bitmap? Will the compiler error out if i get it wrong,
> or silently accept it?
> 
> > Another option is to introduce new fields as bitmaps and mirror all
> > features there, but also keep the current ones. This implies some
> > code duplication -- to keep both sets in sync -- but it will also
> > allow to avoid such diffstats. Developers could switch their
> > drivers
> > one-by-one then, and once they finish converting,
> 
> Which will never happen. Most developers will say, why bother, it
> works as it is, i'm too lazy. And many drivers don't have an active
> developer, and so won't get converted.
> 
> Yes it is a big patchset, but at the end, we get a uniform API which
> is future proof, and no traps waiting for developers to fall into.
> 

I agree, i had to visit this topic a year ago or so, and the only
conclusion was is to solve this the hard way, introduce a totally new
mechanism, the safest way is to remove old netdev_features_t fields
from netdev and add new ones (both names and types), so compiler will
catch you if you missed to convert a place.

maybe hide the implementation details and abstract it away from drivers
using getters and manipulation APIs, it is not that bad since drivers
are already not supposed to modify netdev_features directly.


>    Andrew