Re: [PATCH v1 1/3] firmware loader: Introduce new API - request_firmware_abort()
On Wed, Oct 29, 2014 at 11:04 AM, Kweh, Hock Leong wrote: >> -Original Message- >> From: Ming Lei [mailto:ming@canonical.com] >> Sent: Monday, October 27, 2014 12:00 AM >> >> You can call fw_lookup_buf() directly, otherwise feel free to add: >> >> Acked-by: Ming Lei >> > > Hi Ming Lei, > > The fw_lookup_buf() is defined inside the conditional preprocessor directive > for CONFIG_PM_SLEEP. > Since the request_firmware_abort() may not only be used in PM_SLEEP, could I > move the fw_lookup_buf() > out from the CONFIG_PM_SLEEP block if we want to call fw_lookup_buf() instead > of __fw_lookup_buf()? Sure. Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] firmware loader: Introduce new API - request_firmware_abort()
On Fri, Oct 24, 2014 at 2:18 PM, Kweh Hock Leong
wrote:
> From: "Kweh, Hock Leong"
>
> Besides aborting through user helper interface, a new API
> request_firmware_abort() allows kernel driver module to abort the
> request_firmware() / request_firmware_nowait() when they are no
> longer needed. It is useful for cancelling an outstanding firmware
> load if initiated from inside a module where that module is in the
> process of being unloaded, since the unload function may not have
> a handle to the struct firmware_buf.
>
> Note for people who use request_firmware_nowait():
> You are required to do your own synchronization after you call
> request_firmware_abort() in order to continue unloading the
> module. The example synchronization code shows below:
>
> while (THIS_MODULE && module_refcount(THIS_MODULE))
> msleep(20);
>
> Cc: Matt Fleming
> Signed-off-by: Kweh, Hock Leong
> ---
> drivers/base/firmware_class.c | 30 ++
> include/linux/firmware.h |4
> 2 files changed, 34 insertions(+)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index d276e33..b2fe40b 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1291,6 +1291,36 @@ request_firmware_nowait(
> }
> EXPORT_SYMBOL(request_firmware_nowait);
>
> +/**
> + * request_firmware_abort - abort the waiting of firmware request
> + * @fw_name: name of firmware file
> + *
> + * @fw_name is the same name string while calling to request_firmware()
> + * or request_firmware_nowait().
> + **/
> +int request_firmware_abort(const char *fw_name)
> +{
> + struct firmware_buf *buf;
> + struct firmware_buf *entry;
> +
> + spin_lock(&fw_cache.lock);
> + buf = __fw_lookup_buf(fw_name);
> + spin_unlock(&fw_cache.lock);
You can call fw_lookup_buf() directly, otherwise feel free to add:
Acked-by: Ming Lei
> + if (!buf)
> + return -ENOENT;
> +
> + mutex_lock(&fw_lock);
> + list_for_each_entry(entry, &pending_fw_head, pending_list) {
> + if (entry == buf) {
> + __fw_load_abort(buf);
> + break;
> + }
> + }
> + mutex_unlock(&fw_lock);
> + return 0;
> +}
> +EXPORT_SYMBOL(request_firmware_abort);
> +
> #ifdef CONFIG_PM_SLEEP
> static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
>
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 5952933..ed90c8b 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -47,6 +47,7 @@ int request_firmware_nowait(
> void (*cont)(const struct firmware *fw, void *context));
>
> void release_firmware(const struct firmware *fw);
> +int request_firmware_abort(const char *fw_name);
> #else
> static inline int request_firmware(const struct firmware **fw,
>const char *name,
> @@ -66,6 +67,9 @@ static inline void release_firmware(const struct firmware
> *fw)
> {
> }
>
> +static inline int request_firmware_abort(const char *fw_name)
> +{
> +}
> #endif
>
> #ifdef CONFIG_FW_LOADER_USER_HELPER
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 2/3] firmware loader: fix hung task warning dump
On Fri, Oct 24, 2014 at 2:18 PM, Kweh Hock Leong wrote: > From: "Kweh, Hock Leong" > > When using request_firmware_nowait() with FW_ACTION_NOHOTPLUG param to > expose user helper interface, if the user do not react immediately, after > 120 seconds there will be a hung task warning message dumped as below: > > [ 3000.784235] INFO: task kworker/0:0:8259 blocked for more than 120 seconds. > [ 3000.791281] Tainted: GE 3.16.0-rc1-yocto-standard #41 > [ 3000.798082] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables > this message. > [ 3000.806072] kworker/0:0 D cd0075c8 0 8259 2 0x > [ 3000.812765] Workqueue: events request_firmware_work_func > [ 3000.818253] cd375e18 0046 000e cd0075c8 00f0 cd40ea00 > cd375fec 1b883e89 > [ 3000.826374] 026b cd40ea00 8000 0001 cd0075c8 > cd375de4 c119917f > [ 3000.834492] cd563360 cd375df4 c119a0ab cd563360 cd375e24 > c119a1d6 > [ 3000.842616] Call Trace: > [ 3000.845252] [] ? kernfs_next_descendant_post+0x3f/0x50 > [ 3000.851543] [] ? kernfs_activate+0x6b/0xc0 > [ 3000.856790] [] ? kernfs_add_one+0xd6/0x130 > [ 3000.862047] [] schedule+0x22/0x60 > [ 3000.866548] [] schedule_timeout+0x175/0x1d0 > [ 3000.871887] [] ? __kernfs_create_file+0x71/0xa0 > [ 3000.877574] [] ? sysfs_add_file_mode_ns+0xaa/0x180 > [ 3000.883533] [] wait_for_completion+0x6f/0xb0 > [ 3000.888961] [] ? wake_up_process+0x40/0x40 > [ 3000.894219] [] _request_firmware+0x750/0x9f0 > [ 3000.899666] [] ? n_tty_receive_buf2+0x1f/0x30 > [ 3000.905200] [] request_firmware_work_func+0x22/0x50 > [ 3000.911235] [] process_one_work+0x122/0x380 > [ 3000.916571] [] worker_thread+0xf9/0x470 > [ 3000.921555] [] ? create_and_start_worker+0x50/0x50 > [ 3000.927497] [] ? create_and_start_worker+0x50/0x50 > [ 3000.933448] [] kthread+0x9f/0xc0 > [ 3000.937850] [] ret_from_kernel_thread+0x20/0x30 > [ 3000.943548] [] ? kthread_worker_fn+0x100/0x100 > > This patch change the wait_for_completion() function call to > wait_for_completion_interruptible() function call for solving the issue. > > Cc: Matt Fleming > Signed-off-by: Kweh, Hock Leong Acked-by: Ming Lei Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 0/4] Add firmware signature file check
On Tue, Nov 6, 2012 at 6:53 PM, Takashi Iwai wrote: > At Tue, 6 Nov 2012 18:40:57 +0800, >> It is true if all firmwares are signed on safe boot. If firmware is allowed >> to be loaded from network or other non-fs place in secure distribution, >> your patch will break this loading. > > Do we already have such a secure mechanism? How is the security > assured? I don't know, and my comments are just on your patch and the condition. I understand secure guys should know if the condition may be true or false, :-) >> The clue can be found from debug message. > > Debug messages are turned off on normal machines, unfortunately. Kernel guys will put one eye on bug report, also enabling udev log can help the problem too. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 0/4] Add firmware signature file check
On Tue, Nov 6, 2012 at 6:17 PM, Takashi Iwai wrote: > At Tue, 6 Nov 2012 18:04:36 +0800, > Ming Lei wrote: >> >> On Tue, Nov 6, 2012 at 4:18 PM, Takashi Iwai wrote: >> > >> > Right, and it's intentionally dropped so. For the non-default fw >> > path, it can be added via proc dynamically or via kconfig statically. >> > If the firmware is generated via udev, then it doesn't make sense to >> > check a static signature file. >> >> kconfig should be better, and proc isn't a good way because it >> is a bit late. Also the firmware might be loaded dynamically from other >> where(network, ...). So it is better to fall back on user space if the >> firmware file isn't found by direct loading even firmware signature >> is enabled. > > It will even with my patch, when enforce_sig is false. It is true if all firmwares are signed on safe boot. If firmware is allowed to be loaded from network or other non-fs place in secure distribution, your patch will break this loading. > >> > A "regression" can't happen in this case because the secure boot is >> > a completely new stuff :) For normal boot, sig_enforce is false, so >> > no behavior change here (well my patch still applies the signature >> > check for direct fw loading, but it won't regress at least). >> >> Got it, so FIRMWARE_SIG and MODULE_SIG should be enabled only >> for secure boot. > > The kernels with these kconfigs set can run on normal systems. In > non-secure boot mode, however, sig_enable option are off, thus the > fallback is still applied. > >> The regression might still be triggered if falling back on user space is not >> supported, see above. >> >> > >> >> Also the default search path in firmware_class.c is from built-in path of >> >> udev, and distributions may customize their firmware path by udev >> >> configure option. >> > >> > Well, the default paths in kernel can be changed to follow that as >> > well, no? >> > >> >> > Honestly speaking, I have a feeling that we should rather go for >> >> > getting rid of udev fw loading. The fw loader code is overly complex >> >> >> >> Yes, I have the feeling too, but we need to make sure no regressions >> >> introduced. >> > >> > Right. And I guess the exceptional firmware case is better found by >> > checking udev. But it's a bit off topic from secure boot. >> >> Not sure, some distributions may not use udev at all. > > Hmm, I can't imagine a system without udev but still supporting the > firmware loading with user-space interaction... It is not so difficult, :-) Some embedded systems use mdev in busybox, and some can just parse the firmware event and run the below script: Documentation/firmware_class/hotplug-script on firmware ADD event. I remembered that android loading is very simple. > >> Some application >> might need the firmware add/remove event. > > Then this is already broken with the direct fw loading on 3.7, no? Maybe, the direct loading hasn't been tested widely, and depends on user space. >> Some may not store the >> firmware in fs. > > And these won't satisfy the firmware signing, so we don't need to care > too much. > >> So now it is difficult to say we can remove firmware loading from user >> space. Better to just keep it. > > Yeah, I don't mean to drop it now. But I meant to go for dropping > it. For example, put a deprecated flag and give a warning for udev fw > loading path so that user notices something to be fixed. Maybe we can do it until direct loading has been tested for some time. > >> > Obviously no distro releases using 3.7-rc since it's still rc. >> > But what's your point? >> >> I mean direct loading hasn't been tested completely, so we don't >> know which distributions may fallback on user space loading. > > The transition to direct fw loading is seamless, so I don't think > you can see which drivers use udev fw loading from the results of > distros... It might reveal some potential issues of direct fw loading > (like udev-trigger dependency), though. The clue can be found from debug message. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 0/4] Add firmware signature file check
On Tue, Nov 6, 2012 at 4:18 PM, Takashi Iwai wrote: > > Right, and it's intentionally dropped so. For the non-default fw > path, it can be added via proc dynamically or via kconfig statically. > If the firmware is generated via udev, then it doesn't make sense to > check a static signature file. kconfig should be better, and proc isn't a good way because it is a bit late. Also the firmware might be loaded dynamically from other where(network, ...). So it is better to fall back on user space if the firmware file isn't found by direct loading even firmware signature is enabled. > A "regression" can't happen in this case because the secure boot is > a completely new stuff :) For normal boot, sig_enforce is false, so > no behavior change here (well my patch still applies the signature > check for direct fw loading, but it won't regress at least). Got it, so FIRMWARE_SIG and MODULE_SIG should be enabled only for secure boot. The regression might still be triggered if falling back on user space is not supported, see above. > >> Also the default search path in firmware_class.c is from built-in path of >> udev, and distributions may customize their firmware path by udev >> configure option. > > Well, the default paths in kernel can be changed to follow that as > well, no? > >> > Honestly speaking, I have a feeling that we should rather go for >> > getting rid of udev fw loading. The fw loader code is overly complex >> >> Yes, I have the feeling too, but we need to make sure no regressions >> introduced. > > Right. And I guess the exceptional firmware case is better found by > checking udev. But it's a bit off topic from secure boot. Not sure, some distributions may not use udev at all. Some application might need the firmware add/remove event. Some may not store the firmware in fs. So now it is difficult to say we can remove firmware loading from user space. Better to just keep it. > Obviously no distro releases using 3.7-rc since it's still rc. > But what's your point? I mean direct loading hasn't been tested completely, so we don't know which distributions may fallback on user space loading. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 0/4] Add firmware signature file check
On Tue, Nov 6, 2012 at 3:32 PM, Takashi Iwai wrote:
> At Tue, 6 Nov 2012 15:16:43 +0800,
> Ming Lei wrote:
>>
>> On Tue, Nov 6, 2012 at 3:03 PM, Takashi Iwai wrote:
>> >
>> > Yeah, it's just uncovered in the patch. As a easy solution, apply the
>> > patch like below to disallow the udev fw loading when signature check
>> > is enforced.
>> >
>> >
>> > thanks,
>> >
>> > Takashi
>> >
>> > ---
>> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> > index 575bc4c..93121c3 100644
>> > --- a/drivers/base/firmware_class.c
>> > +++ b/drivers/base/firmware_class.c
>> > @@ -912,6 +912,13 @@ static int _request_firmware_load(struct
>> > firmware_priv *fw_priv, bool uevent,
>> > goto handle_fw;
>> > }
>> >
>> > + /* signature check isn't handled via udev fw loading */
>> > + if (sig_enforce) {
>> > + fw_load_abort(fw_priv);
>> > + direct_load = 1;
>> > + goto handle_fw;
>> > + }
>> > +
>>
>> The above might be wrong if the firmware file doesn't exist in default
>> search paths.
>
> Heh, I didn't call it's a perfect patch. It's just an easy solution,
> as mentioned.
>
>> You should skip loading from user space only if
>> verify_signature() returns false. And the udev loading should be
>> resorted to if there is no such firmware file in default search paths.
>
> ... and the kernel should ask udev again for the corresponding
> signature.
I mean you can't skip user space loading if there is no firmware file
in the default search path. And you can do it if verify_signature()
returns false. So you needn't have to implement the signature for
user space loading.
> I'm too lazy to implement that just for unknown corner
> cases, so put the patch like above.
There might be some distributions in which the firmwares aren't stored
under the default search path, so your change may cause regression
on these distributions. And, it is a easy change in your patch to make
the situation working.
Also the default search path in firmware_class.c is from built-in path of
udev, and distributions may customize their firmware path by udev
configure option.
>
> Honestly speaking, I have a feeling that we should rather go for
> getting rid of udev fw loading. The fw loader code is overly complex
Yes, I have the feeling too, but we need to make sure no regressions
introduced.
> just for udev handshaking.
>
> Do you know how many firmwares still rely on udev...?
Do you know how many distributions have switched to 3.7-rcX to
start using direct loading?
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 0/4] Add firmware signature file check
On Tue, Nov 6, 2012 at 1:36 PM, Li Joey wrote:
> The udev direct write firmware through data attribute, maybe we can do the
> same signature verification in firmware_data_write? The following patch
> didn't test yet.
> @@ -655,6 +656,23 @@ static ssize_t firmware_data_write(struct file *filp,
> struct kobject *kobj,
> }
>
> buf->size = max_t(size_t, offset, buf->size);
> +
> +#ifdef CONFIG_FIRMWARE_SIG
> + for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> + snprintf(path, PATH_MAX, "%s/%s.sig", fw_path[i],
> buf->fw_id);
> + if (verify_signature(buf, path))
> + success = true;
> + }
When direct loading failed, it means that the firmware isn't
under the default search path, so the above verification
might return false always.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 0/4] Add firmware signature file check
On Tue, Nov 6, 2012 at 3:03 PM, Takashi Iwai wrote:
>
> Yeah, it's just uncovered in the patch. As a easy solution, apply the
> patch like below to disallow the udev fw loading when signature check
> is enforced.
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 575bc4c..93121c3 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -912,6 +912,13 @@ static int _request_firmware_load(struct firmware_priv
> *fw_priv, bool uevent,
> goto handle_fw;
> }
>
> + /* signature check isn't handled via udev fw loading */
> + if (sig_enforce) {
> + fw_load_abort(fw_priv);
> + direct_load = 1;
> + goto handle_fw;
> + }
> +
The above might be wrong if the firmware file doesn't exist in default
search paths. You should skip loading from user space only if
verify_signature() returns false. And the udev loading should be
resorted to if there is no such firmware file in default search paths.
> /* fall back on userspace loading */
> buf->fmt = PAGE_BUF;
>
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 0/4] Add firmware signature file check
On Tue, Nov 6, 2012 at 1:18 AM, Takashi Iwai wrote: > > To be noted, it doesn't support the firmwares via udev but only the > direct loading, and the check for built-in firmware is missing, too. Generally, both direct loading and udev may request one same firmware image. And after check failed, current firmware load will fallback on udev to complete loading, so looks a check-failed firmware still can be loaded into kernel no matter if there is firmware signature check or not. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
