Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180421144911.GV14440@wotan.suse.de>
References: <20180417153307.3693-1-andresx7@gmail.com> <20180417153307.3693-6-andresx7@gmail.com>
 <20180421143206.GQ14440@wotan.suse.de> <20180421144911.GV14440@wotan.suse.de>
From:   Kees Cook <keescook@chromium.org>
Date:   Sat, 21 Apr 2018 08:11:04 -0700
Message-ID: <CAGXu5jKVv8R1yVcuCCPJb1YD9u8tiZyj86wdz043PKxKPZj4Lg@mail.gmail.com>
Subject: Re: [PATCH 5/9] firmware: add functions to load firmware without
 warnings v4
To:     "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc:     Andres Rodriguez <andresx7@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        David Woodhouse <dwmw2@infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Alex Deucher <alexdeucher@gmail.com>,
        ckoenig.leichtzumerken@gmail.com,
        Kalle Valo <kvalo@codeaurora.org>,
        Arend van Spriel <arend.vanspriel@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Apr 21, 2018 at 7:49 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Sat, Apr 21, 2018 at 04:32:06PM +0200, Luis R. Rodriguez wrote:
>> On Tue, Apr 17, 2018 at 11:33:03AM -0400, Andres Rodriguez wrote:
>> > @@ -755,10 +779,11 @@ static void firmware_request_work_func(struct work_struct *work)
>> >  }
>> >
>> >  /**
>> > - * firmware_request_nowait() - asynchronous version of firmware_request
>> > + * firmware_request_nowait2() - asynchronous version of firmware_request
>> >   * @module: module requesting the firmware
>> >   * @uevent: sends uevent to copy the firmware image if this flag
>> >   * is non-zero else the firmware copy must be done manually.
>> > + * @warn: enable warnings
>> >   * @name: name of firmware file
>> >   * @device: device for which firmware is being loaded
>> >   * @gfp: allocation flags
>> > @@ -778,8 +803,8 @@ static void firmware_request_work_func(struct work_struct *work)
>> >   *         - can't sleep at all if @gfp is GFP_ATOMIC.
>> >   **/
>> >  int
>> > -firmware_request_nowait(
>> > -   struct module *module, bool uevent,
>> > +firmware_request_nowait2(
>> > +   struct module *module, bool uevent, bool warn,
>> >     const char *name, struct device *device, gfp_t gfp, void *context,
>> >     void (*cont)(const struct firmware *fw, void *context))
>> >  {
>> > @@ -799,7 +824,8 @@ firmware_request_nowait(
>> >     fw_work->context = context;
>> >     fw_work->cont = cont;
>> >     fw_work->opt_flags = FW_OPT_NOWAIT |
>> > -           (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
>> > +           (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER) |
>> > +           (warn ? 0 : FW_OPT_NO_WARN);
>> >
>> >     if (!uevent && fw_cache_is_setup(device, name)) {
>> >             kfree_const(fw_work->name);
>> > @@ -818,6 +844,24 @@ firmware_request_nowait(
>> >     schedule_work(&fw_work->work);
>> >     return 0;
>> >  }
>> > +EXPORT_SYMBOL_GPL(firmware_request_nowait2);
>> > +
>> > +/**
>> > + * firmware_request_nowait() - compatibility version of firmware_request_nowait2
>> > + *
>> > + * This is equivalent to calling firmware_request_nowait2 with warnings enabled.
>> > + *
>> > + * Refer to firmware_request_nowait2 for further details.
>> > + **/
>> > +int
>> > +firmware_request_nowait(
>> > +   struct module *module, bool uevent,
>> > +   const char *name, struct device *device, gfp_t gfp, void *context,
>> > +   void (*cont)(const struct firmware *fw, void *context))
>> > +{
>> > +   return firmware_request_nowait2(module, uevent, true, name, device,
>> > +                                   gfp, context, cont);
>> > +}
>> >  EXPORT_SYMBOL(firmware_request_nowait);
>> >
>> >  #ifdef CONFIG_PM_SLEEP
>>
>> Ugh this is precisely the type of naming issue I predicted *years ago*
>> about the unflexibility of the naming scheme we used. Greg, since you had
>> sent us this rabbit hole, any name preference here? Please review what is
>> proposed and also suggest a scheme which you do prefer. I'm done with
>> the bikeshedding and just want to move on, but in a way that scales.
>
> I'll side for now with Kalle's suggestion of having:
>
> firmware_request_nowait_nowarn()
>
> as nasty as it may seem. And this is just because we embarked on
> the path to not have parameters passed to modify the calls site.

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?

-Kees

-- 
Kees Cook
Pixel Security