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] [day] [month] [year] [list]

Message-ID: <s5hsj5hwuad.wl%tiwai@suse.de>
Date:	Thu, 31 Jan 2013 10:55:54 +0100
From:	Takashi Iwai <tiwai@...e.de>
To:	Ming Lei <ming.lei@...onical.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

At Thu, 31 Jan 2013 17:48:34 +0800,
Ming Lei wrote:
> 
(snip)
> > +/* called from request_firmware() and request_firmware_work_func() */
> > +static int
> > +_request_firmware(const struct firmware **firmware_p, const char *name,
> > +                 struct device *device, bool uevent, bool nowait)
> > +{
> > +       struct firmware *fw;
> > +       long timeout;
> > +       int ret;
> > +
> > +       if (!firmware_p)
> > +               return -EINVAL;
> > +
> > +       ret = _request_firmware_prepare(&fw, name, device);
> > +       if (ret <= 0) /* error or already assigned */
> > +               goto out;
> > +
> > +       ret = 0;
> > +       timeout = firmware_loading_timeout();
> 
> The above line may be moved below the line of 'if (nowait) '.

I thought of that, too, but later found that the timeout is used in
the call of fw_load_from_user_helper() below.

> > +       if (nowait) {
> > +               timeout = usermodehelper_read_lock_wait(timeout);
> > +               if (!timeout) {
> > +                       dev_dbg(device, "firmware: %s loading timed out\n",
> > +                               name);
> > +                       ret = -EBUSY;
> > +               }
> > +       } else {
> > +               ret = usermodehelper_read_trylock();
> > +               if (WARN_ON(ret)) {
> > +                       dev_err(device, "firmware: %s will not be loaded\n",
> > +                               name);
> > +               }
> > +       }
> > +
> > +       if (!ret) {
> > +               if (!fw_get_filesystem_firmware(device, fw->priv))
> > +                       ret = fw_load_from_user_helper(fw, name, device,
> > +                                                      uevent, nowait,
> > +                                                      timeout);
> > +               if (!ret)
> > +                       ret = assign_firmware_buf(fw, device);
> > +       }
> > +
> > +       usermodehelper_read_unlock();
> 
> The above should be move inside the above 'if (!ret) {...}'

Yep, but it would make the code less understandable.
I'll make rather the error path of usermodehelper lock goto's.

> 
> Except for the above two comments, looks the patch v4 is fine.
> 
> Please CC greg-kh when you send v5, and you may add my ack
> for the patchset.
> 
>             Acked-by: Ming Lei <ming.lei@...onical.com>

OK, thanks!


Takashi
--
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/