| 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
| ||
|
Message-ID: <s5hha3058j2.wl%tiwai@suse.de>
Date: Wed, 02 Jul 2014 13:01:37 +0200
From: Takashi Iwai <tiwai@...e.de>
To: "Luis R. Rodriguez" <mcgrof@...not-panic.com>
Cc: ming.lei@...onical.com, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org,
"Luis R. Rodriguez" <mcgrof@...e.com>, Tom Gundersen <teg@...m.no>,
Abhay Salunke <Abhay_Salunke@...l.com>,
Stefan Roese <sr@...x.de>, Arnd Bergmann <arnd@...db.de>,
Kay Sievers <kay@...y.org>
Subject: Re: [PATCH v2] firmware loader: inform direct failure when udev loader is disabled
At Tue, 1 Jul 2014 20:07:53 -0700,
Luis R. Rodriguez wrote:
>
> From: "Luis R. Rodriguez" <mcgrof@...e.com>
>
> Now that the udev firmware loader is optional request_firmware()
> will not provide any information on the kernel ring buffer if
> direct firmware loading failed and udev firmware loading is disabled.
> If no information is needed request_firmware_direct() should be used
> for optional firmware, at which point drivers can take on the onus
> over informing of any failures, if udev firmware loading is disabled
> though we should at the very least provide some sort of information
> as when the udev loader was enabled by default back in the days.
>
> With this change with a simple firmware load test module [0]:
>
> Example output without FW_LOADER_USER_HELPER_FALLBACK
>
> platform fake-dev.0: Direct firmware load for fake.bin failed
> with error -2
>
> Example with FW_LOADER_USER_HELPER_FALLBACK
>
> platform fake-dev.0: Direct firmware load for fake.bin failed with error -2
> platform fake-dev.0: Falling back to user helper
>
> Without this change without FW_LOADER_USER_HELPER_FALLBACK we
> get no output logged upon failure.
>
> Cc: Tom Gundersen <teg@...m.no>
> Cc: Ming Lei <ming.lei@...onical.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Abhay Salunke <Abhay_Salunke@...l.com>
> Cc: Stefan Roese <sr@...x.de>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Kay Sievers <kay@...y.org>
> Cc: Takashi Iwai <tiwai@...e.de>
> Signed-off-by: Luis R. Rodriguez <mcgrof@...e.com>
> ---
>
> This v2 modifies the error to always be called and only in the
> request_firmware_direct case do we not send smoke signals.
>
> drivers/base/firmware_class.c | 13 +++++++------
> include/linux/firmware.h | 15 ++++++++-------
> 2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 46ea5f4..60d0e53 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -109,6 +109,7 @@ static inline long firmware_loading_timeout(void)
> #else
> #define FW_OPT_FALLBACK 0
> #endif
> +#define FW_OPT_DIRECT_ONLY (1U << 3)
I'd name it like FW_OPT_NO_WARN or such.
Other than that, looks good to me.
thanks,
Takashi
>
> struct firmware_cache {
> /* firmware_buf instance will be added into the below list */
> @@ -1116,10 +1117,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>
> ret = fw_get_filesystem_firmware(device, fw->priv);
> if (ret) {
> - if (opt_flags & FW_OPT_USERHELPER) {
> + if (!(opt_flags & FW_OPT_DIRECT_ONLY))
> dev_warn(device,
> - "Direct firmware load failed with error %d\n",
> - ret);
> + "Direct firmware load for %s failed with error %d\n",
> + name, ret);
> + if (opt_flags & FW_OPT_USERHELPER) {
> dev_warn(device, "Falling back to user helper\n");
> ret = fw_load_from_user_helper(fw, name, device,
> opt_flags, timeout);
> @@ -1176,7 +1178,6 @@ request_firmware(const struct firmware **firmware_p, const char *name,
> }
> EXPORT_SYMBOL(request_firmware);
>
> -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> /**
> * request_firmware: - load firmware directly without usermode helper
> * @firmware_p: pointer to firmware image
> @@ -1193,12 +1194,12 @@ int request_firmware_direct(const struct firmware **firmware_p,
> {
> int ret;
> __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware_p, name, device, FW_OPT_UEVENT);
> + ret = _request_firmware(firmware_p, name, device,
> + FW_OPT_UEVENT | FW_OPT_DIRECT_ONLY);
> module_put(THIS_MODULE);
> return ret;
> }
> EXPORT_SYMBOL_GPL(request_firmware_direct);
> -#endif
>
> /**
> * release_firmware: - release the resource associated with a firmware image
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 67e5b80..5c41c5e 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -45,6 +45,8 @@ int request_firmware_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));
> +int request_firmware_direct(const struct firmware **fw, const char *name,
> + struct device *device);
>
> void release_firmware(const struct firmware *fw);
> #else
> @@ -66,13 +68,12 @@ static inline void release_firmware(const struct firmware *fw)
> {
> }
>
> -#endif
> +static inline int request_firmware_direct(const struct firmware **fw,
> + const char *name,
> + struct device *device)
> +{
> + return -EINVAL;
> +}
>
> -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> -int request_firmware_direct(const struct firmware **fw, const char *name,
> - struct device *device);
> -#else
> -#define request_firmware_direct request_firmware
> #endif
> -
> #endif
> --
> 2.0.0
>
--
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/
Powered by blists - more mailing lists