linux-kernel - Re: [PATCH] firmware loader: allow disabling of udev as firmware loader

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: <s5hr43421un.wl%tiwai@suse.de>
Date:	Wed, 04 Jun 2014 16:20:16 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	Tom Gundersen <teg@...m.no>
Cc:	linux-kernel@...r.kernel.org, Ming Lei <ming.lei@...onical.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Abhay Salunke <Abhay_Salunke@...l.com>,
	Stefan Roese <sr@...x.de>, Arnd Bergmann <arnd@...db.de>,
	Kay Sievers <kay@...y.org>
Subject: Re: [PATCH] firmware loader: allow disabling of udev as firmware loader

At Mon,  2 Jun 2014 20:24:34 +0200,
Tom Gundersen wrote:
> 
> Currently (at least) the dell-rbu driver selects FW_LOADER_USER_HELPER,
> which means that distros can't really stop loading firmware through udev
> without breaking other users (though some have).
> 
> Ideally we would remove/disable the udev firmware helper in both the kernel
> and in udev, but if we were to disable it in udev and not the kernel, the result
> would be (seemingly) hung kernels as no one would be around to cancel firmware
> requests.
> 
> This patch allows udev firmware loading to be disabled while still allowing
> non-udev firmware loading, as done by the dell-rbu driver, to continue
> working. This is achieved by only using the fallback mechanism when the
> uevent is suppressed.
> 
> Tested with
>     FW_LOADER_USER_HELPER=n
>     LATTICE_ECP3_CONFIG=y
>     DELL_RBU=y
> and udev without the firmware loading support, but I don't have the hardware
> to test the lattice/dell drivers, so additional testing would be appreciated.

The logic of this patch looks good to me, but the Kconfig items become
confusing by this.  Basically what we'd need is a Kconfig item
deciding whether to build the user helper or not, in addition to a
Kconfig item for deciding the fallback mode of request_firmware().

What about the patch like below instead?  It's smaller and the meaning
of Kconfig items are clearer.  (In the final form, the help text
change you added should be included there, too.)

The only (and biggest) drawback is, however, that the user-selectable
Kconfig would be actually renamed from CONFIG_FW_LOADER_USER_HELPER to
CONFIG_FW_LOADER_USER_HELPER_FALLBACK.


thanks,

Takashi

---
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 8fa8deab6449..195b08f49209 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -144,8 +144,12 @@ config EXTRA_FIRMWARE_DIR
 	  some other directory containing the firmware files.
 
 config FW_LOADER_USER_HELPER
+	bool
+
+config FW_LOADER_USER_HELPER_FALLBACK
 	bool "Fallback user-helper invocation for firmware loading"
 	depends on FW_LOADER
+	select FW_LOADER_USER_HELPER
 	default y
 	help
 	  This option enables / disables the invocation of user-helper
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d276e33880be..e98fd78c5c40 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -100,9 +100,14 @@ static inline long firmware_loading_timeout(void)
 #define FW_OPT_UEVENT	(1U << 0)
 #define FW_OPT_NOWAIT	(1U << 1)
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-#define FW_OPT_FALLBACK	(1U << 2)
+#define FW_OPT_USERHELPER	(1U << 2)
 #else
-#define FW_OPT_FALLBACK	0
+#define FW_OPT_USERHELPER	0
+#endif
+#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
+#define FW_OPT_FALLBACK		FW_OPT_USERHELPER
+#else
+#define FW_OPT_FALLBACK		0
 #endif
 
 struct firmware_cache {
@@ -1111,7 +1116,7 @@ _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_FALLBACK) {
+		if (opt_flags & FW_OPT_USERHELPER) {
 			dev_warn(device,
 				 "Direct firmware load failed with error %d\n",
 				 ret);
@@ -1277,7 +1282,7 @@ request_firmware_nowait(
 	fw_work->context = context;
 	fw_work->cont = cont;
 	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
-		(uevent ? FW_OPT_UEVENT : 0);
+		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
 
 	if (!try_module_get(module)) {
 		kfree(fw_work);
--
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/