linux-kernel - Re: [PATCH 5/9] firmware: add functions to load firmware without warnings v4

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]

Date:   Sat, 21 Apr 2018 19:36:50 +0200
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Kees Cook <keescook@...omium.org>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Andres Rodriguez <andresx7@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hans de Goede <hdegoede@...hat.com>,
        David Woodhouse <dwmw2@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Alex Deucher <alexdeucher@...il.com>,
        ckoenig.leichtzumerken@...il.com,
        Kalle Valo <kvalo@...eaurora.org>,
        Arend van Spriel <arend.vanspriel@...adcom.com>
Subject: Re: [PATCH 5/9] firmware: add functions to load firmware without
 warnings v4

On Sat, Apr 21, 2018 at 08:32:00AM -0700, Linus Torvalds wrote:
> On Sat, Apr 21, 2018 at 8:11 AM, Kees Cook <keescook@...omium.org> wrote:
> >
> > What was the objection to using parameters for this? i.e. something
> > like the gfp flags, but have a behavior flag FW_RQ_NOWAIT,
> > FW_RQ_NOWARN, etc?
> 
> The objection was that the patches that I think Luis refers to

There were different topics of discussion, people can conflate the topics
discussed easily as all these topics were discussed around the same time so
best to recap and split them up:

  a) bikeshedding on the name: I suggested long ago we've already passed usage
     of the fw API to things which are *not firmware*, so suggested we use a new
     naming scheme, the last one I recommended was a driver_data_*() prefix.
     Greg last suggested a firmware_*() prefix -- and to rename *all* existing 
     callers with this new prefix as well long term... I am done with bikeshedding
     and frankly don't care anymore. Whatever you folks decide, I just want to
     move on with life, so I am moving along with the firmware_*() prefix.

  b) evolving the API: data driven Vs functional -- where to draw the fine line
       -- this is the topic we are discussing now again --

  c) firmware signing: we've decided against it for now and folks who need it
     can open code it, mainly due to most hw supporting FW signing already

>  (a) passed in a union of random arguments

It split the API into two main routine calls, a sync and an async call,
and also accepted a *structure of parameters* which describe the request
requirements [0].

It turned out this approach was *too data driven*, and Greg advised against
it, asking for a new call *per new functionality*, as is typically done...

[0] https://lkml.kernel.org/r/20170605213937.26215-2-mcgrof@kernel.org

>  (b) changed all the users, even the ones that didn't want to be changed.

That *never happened* actually. The SmPL grammar I provided long ago was for
those who *did* want to change the new API if they had a new *feature* they
wanted to take advantage of. The patches you may have seen were *examples* of
*two drivers* which were converted over just an example with the grammar.

On the last iteration of my series I skipped adding the SmPL helpers, and
only converted *one* driver for *new functionality*.

In the last driver data patch series I only converted iwlwifi as I was changing
it to use the new functionality to replace its own internal recursion set of
calls with an internal deterministic solution which lets drivers call for a
range of API files and lets it use the first one in a range it finds [1],
with this diff stat:

 drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 91 ++++++++++------------------
 1 file changed, 31 insertions(+), 60 deletions(-)

[1] https://lkml.kernel.org/r/20170605213937.26215-6-mcgrof@kernel.org

> they were nasty and illegible and pointless.

Clearly request_firmware_nowait2() is *not* much better... right? So it illustrates
the problem I was hinting which we'd eventually cross...

> Using some single flag field for an extended function, and leaving the
> existing functions alone so that you don't have to convert existing
> users - that would have been fine. That's not what was tried and
> rejected.

Actually it was tried, however the divide was perhaps *too broad* and split
all possible *new* functionality into two calls, a sync and a async call.

A flag based mechanism *is* reasonable to me given I have been an advocate
of such type of mechanism for a long while. It however is against what
Greg requested -- to have a new call *per functionality*.

So feel free to advise... I really just want us to move on.

  Luis