linux-kernel - Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

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: <CACVXFVMqxrjBKSnRENHYhd5=pfp3L2LchOzo8cLbp4eo1ph6FA@mail.gmail.com>
Date:	Wed, 30 Jan 2013 19:48:15 +0800
From:	Ming Lei <ming.lei@...onical.com>
To:	Takashi Iwai <tiwai@...e.de>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

On Wed, Jan 30, 2013 at 7:08 PM, Takashi Iwai <tiwai@...e.de> wrote:
> At Wed, 30 Jan 2013 11:53:14 +0100,
> Takashi Iwai wrote:
>>
>> At Wed, 30 Jan 2013 18:50:05 +0800,
>> Ming Lei wrote:
>> >
>> > On Wed, Jan 30, 2013 at 6:31 PM, Takashi Iwai <tiwai@...e.de> wrote:
>> > >
>> > > But it's supposed to be cached, no?
>> >
>> > Generally it will be cached, but some crazy devices might come as new
>> > device during resume, so we still need to handle the situation.
>>
>> In that case, shouldn't we simply return an error instead of
>> usermodehelper lock (at least for direct loading)?
>
> The patch below is what I have in my mind...
>
>
> Takashi
>
> ---
> From: Takashi Iwai <tiwai@...e.de>
> Subject: [PATCH] firmware: Skip usermodehelper_lock for direct loading
>
> We don't need a user mode helper lock for the direct fw loading.
> This also reduces the use of timeout when user mode helper is
> disabled, thus we can move the code into ifdef block, too.
>
> For avoiding the possible call of request_firmware() without caching,
> add a check of suspend state for the direct loading case, and returns
> immediately if it's called during suspend/resume.  Then it proceeds to
> the user mode helper if enabled, or returns the error.
>
> Signed-off-by: Takashi Iwai <tiwai@...e.de>
> ---
>  drivers/base/firmware_class.c | 97 ++++++++++++++++++++++++-------------------
>  1 file changed, 54 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index f1caad7..c973bb0 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -88,13 +88,6 @@ enum {
>         FW_STATUS_ABORT,
>  };
>
> -static int loading_timeout = 60;       /* In seconds */
> -
> -static inline long firmware_loading_timeout(void)
> -{
> -       return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
> -}
> -
>  struct firmware_cache {
>         /* firmware_buf instance will be added into the below list */
>         spinlock_t lock;
> @@ -315,6 +308,9 @@ static bool fw_get_filesystem_firmware(struct device *device,
>         bool success = false;
>         char *path = __getname();
>
> +       if (device->power.is_suspended)
> +               return false;

The device which is requesting firmware is resumed  does not mean the
storage device has been resumed, so this check isn't enough.

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/