Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1468993 > unrolled thread

[PATCH v3 0/5] firmware: add SmPL grammar to avoid issues

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 0/5] firmware: add SmPL grammar to avoid issues mcgrof@kernel.org - 2016-08-24 03:00 +0200
    [PATCH v3 3/5] firmware: update usermode helper docs and add SmPL report mcgrof@kernel.org - 2016-08-24 03:00 +0200
    [PATCH v3 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation mcgrof@kernel.org - 2016-08-24 03:00 +0200
    [PATCH v3 1/5] MAINTAINERS: extend firmware_class maintainer list mcgrof@kernel.org - 2016-08-24 03:00 +0200

#1468993 — [PATCH v3 0/5] firmware: add SmPL grammar to avoid issues

From: "Luis R. Rodriguez" <mcgrof@kernel.org>

Greg,

This v3 is a simple respin without any modifications other than a rebase
from the last v2 series [0] to apply to today's linux-next tre. The v2 series
did not make it in in time.

[0] https://lkml.kernel.org/r/1466117661-22075-1-git-send-email-mcgrof@kernel.org

Luis R. Rodriguez (5):
  MAINTAINERS: extend firmware_class maintainer list
  firmware: annotate thou shalt not request fw on init or probe
  firmware: update usermode helper docs and add SmPL report
  firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation
  firmware: fix fw cache to avoid usermode helper on suspend

 Documentation/firmware_class/README                |  59 +++++++++-
 MAINTAINERS                                        |   1 +
 drivers/base/Kconfig                               |   2 +-
 drivers/base/firmware_class.c                      |   2 +-
 drivers/firmware/dell_rbu.c                        |   1 +
 drivers/leds/leds-lp55xx-common.c                  |   1 +
 include/linux/firmware.h                           |   7 ++
 .../request_firmware-avoid-init-probe-init.cocci   | 130 +++++++++++++++++++++
 .../coccinelle/api/request_firmware-usermode.cocci |  44 +++++++
 9 files changed, 240 insertions(+), 7 deletions(-)
 create mode 100644 scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
 create mode 100644 scripts/coccinelle/api/request_firmware-usermode.cocci

-- 
2.9.2

[toc] | [next] | [standalone]

#1468995 — [PATCH v3 3/5] firmware: update usermode helper docs and add SmPL report

From: "Luis R. Rodriguez" <mcgrof@kernel.org>

We've determined that very sadly we cannot nuke the usermode helper.
The firmware code is a bit hard to follow, and so is the history,
document the reasons for why we cannot remove the usermode helper and
the only thing we can do is compartamentalize it.

While it, add a Coccinelle SmPL patch to help maintainers police
for *infidels* still using the usermode helper. Its accepted that perhaps
we can't get rid of all use cases, as otherwise we'd break userspace, so
best we can do is go on a hunt and fix incorrect users of it. Drivers
can only be transitioned out of the usermode helper once we know old
userspace cannot be not be broken by a kernel change.

The current SmPL patch reports:

$ export COCCI=scripts/coccinelle/api/request_firmware-usermode.cocci
$ make coccicheck MODE=report

drivers/leds/leds-lp55xx-common.c:227:8-31: WARNING: please check if user really needs the usermode helper
drivers/firmware/dell_rbu.c:622:17-40: WARNING: please check if user really needs the usermode helper

Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: linux-leds@vger.kernel.org
Cc: Abhay Salunke <Abhay_Salunke@dell.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 Documentation/firmware_class/README                | 36 ++++++++++++++++++---
 .../coccinelle/api/request_firmware-usermode.cocci | 37 ++++++++++++++++++++++
 2 files changed, 68 insertions(+), 5 deletions(-)
 create mode 100644 scripts/coccinelle/api/request_firmware-usermode.cocci

diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
index 056d1cb9d365..00604b6d7675 100644
--- a/Documentation/firmware_class/README
+++ b/Documentation/firmware_class/README
@@ -33,7 +33,7 @@
 	than 256, user should pass 'firmware_class.path=$CUSTOMIZED_PATH'
 	if firmware_class is built in kernel(the general situation)
 
- 2), userspace:
+ 2), userspace: (AVOID)
  	- /sys/class/firmware/xxx/{loading,data} appear.
 	- hotplug gets called with a firmware identifier in $FIRMWARE
 	  and the usual hotplug environment.
@@ -41,14 +41,14 @@
 
  3), kernel: Discard any previous partial load.
 
- 4), userspace:
+ 4), userspace: (AVOID)
 		- hotplug: cat appropriate_firmware_image > \
 					/sys/class/firmware/xxx/data
 
  5), kernel: grows a buffer in PAGE_SIZE increments to hold the image as it
 	 comes in.
 
- 6), userspace:
+ 6), userspace: (AVOID)
 		- hotplug: echo 0 > /sys/class/firmware/xxx/loading
 
  7), kernel: request_firmware() returns and the driver has the firmware
@@ -66,8 +66,8 @@
 	 	copy_fw_to_device(fw_entry->data, fw_entry->size);
 	 release_firmware(fw_entry);
 
- Sample/simple hotplug script:
- ============================
+ Sample/simple hotplug script: (AVOID)
+ ==========================================
 
 	# Both $DEVPATH and $FIRMWARE are already provided in the environment.
 
@@ -114,6 +114,32 @@ If you're a maintainer you can help police this with:
 $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
 $ make coccicheck MODE=report
 
+Usermode helper
+===============
+
+The firmware API supports a usermode helper that lets userspace fetch and
+hand off firmware to a driver through sysfs. While in theory this was a
+nice feature it also presented many issues, so in practice it should be
+avoided at all costs now.
+
+Only drivers that have a really good reason for a usermode helper should
+use it. This use case must be well documented. You should try really hard
+to avoid it, at all costs.
+
+CONFIG_FW_LOADER_USER_HELPER_FALLBACK: disabled by most distributions now
+CONFIG_FW_LOADER_USER_HELPER: still enabled by most disributions
+
+When CONFIG_FW_LOADER_USER_HELPER_FALLBACK is enabled request_firmware()
+*always* calls the usermode helpers. When CONFIG_FW_LOADER_USER_HELPER is
+enabled, only if you specifically ask for it wil the usermode helper be
+called. If CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled drivers can
+still require the usermode helper by using request_firmware_nowait().
+
+To help police for user of the usermode helper you can use:
+
+$ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
+$ make coccicheck MODE=report
+
  about in-kernel persistence:
  ---------------------------
  Under some circumstances, as explained below, it would be interesting to keep
diff --git a/scripts/coccinelle/api/request_firmware-usermode.cocci b/scripts/coccinelle/api/request_firmware-usermode.cocci
new file mode 100644
index 000000000000..94ab95cb7c75
--- /dev/null
+++ b/scripts/coccinelle/api/request_firmware-usermode.cocci
@@ -0,0 +1,37 @@
+// Avoid the usermode helper at all costs
+//
+// request_firmware_nowait() API enables explicit request for the
+// usermode helper. Chances are high its use is just a copy and
+// paste bug. Before you fix the driver be sure to *verify* no
+// usermode helper tool exists that would otherwise break if
+// we replace the driver to use a non-usermode helper variant.
+//
+// Confidence: High
+//
+// Reason for low confidence:
+//
+// Copyright: (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org> GPLv2.
+//
+// Options: --include-headers
+
+virtual report
+virtual context
+
+@ r1 depends on report || context @
+expression mod, name, dev, gfp, drv, cb;
+position p;
+@@
+
+(
+*request_firmware_nowait@p(mod, false, name, dev, gfp, drv, cb)
+|
+*request_firmware_nowait@p(mod, 0, name, dev, gfp, drv, cb)
+|
+*request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
+)
+
+@script:python depends on report@
+p << r1.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING: please check if user really needs the usermode helper")
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1468998 — [PATCH v3 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation

From: "Luis R. Rodriguez" <mcgrof@kernel.org>

We need to ensure no one else adds *anything* that requests the
usermode helper without really meaning it, to police it we now have
an SmPL script but no formal annotation is present to help us ensure
the call has been validated.

Add a dummy DECLARE_FW_LOADER_USER() which we can use to annotate
validated calls and also clearly outline the documentation over where
the required usermode helper is documented.

All other future callers can be reported via 0-day by making use of the
Coccinelle SmPL patch.

Cc: Fengguang Wu <fengguang.wu@intel.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: linux-leds@vger.kernel.org
Cc: Abhay Salunke <Abhay_Salunke@dell.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/firmware/dell_rbu.c                            | 1 +
 drivers/leds/leds-lp55xx-common.c                      | 1 +
 include/linux/firmware.h                               | 7 +++++++
 scripts/coccinelle/api/request_firmware-usermode.cocci | 9 ++++++++-
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..53b3869c0900 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
 	return size;
 }
 
+DECLARE_FW_LOADER_USER("Documentation/dell_rbu.txt");
 static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
 				    struct bin_attribute *bin_attr,
 				    char *buffer, loff_t pos, size_t count)
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 5377f22ff994..ce1d087979a1 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -219,6 +219,7 @@ out:
 	release_firmware(chip->fw);
 }
 
+DECLARE_FW_LOADER_USER("Documentation/leds/leds-lp55xx.txt");
 static int lp55xx_request_firmware(struct lp55xx_chip *chip)
 {
 	const char *name = chip->cl->name;
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..147b3062039b 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -8,6 +8,13 @@
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 
+/*
+ * Helper for scripts/coccinelle/api/request_firmware-usermode.cocci
+ * and so users can also easily search for the respectively needed
+ * usermode helper.
+ */
+#define DECLARE_FW_LOADER_USER(__usermode_helper)
+
 struct firmware {
 	size_t size;
 	const u8 *data;
diff --git a/scripts/coccinelle/api/request_firmware-usermode.cocci b/scripts/coccinelle/api/request_firmware-usermode.cocci
index 94ab95cb7c75..1775fd3067e8 100644
--- a/scripts/coccinelle/api/request_firmware-usermode.cocci
+++ b/scripts/coccinelle/api/request_firmware-usermode.cocci
@@ -17,6 +17,13 @@
 virtual report
 virtual context
 
+@ r0 depends on report || context @
+declarer name DECLARE_FW_LOADER_USER;
+expression E;
+@@
+
+DECLARE_FW_LOADER_USER(E);
+
 @ r1 depends on report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;
@@ -30,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
 )
 
-@script:python depends on report@
+@script:python depends on report && !r0 @
 p << r1.p;
 @@
 
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1468999 — [PATCH v3 1/5] MAINTAINERS: extend firmware_class maintainer list

From: "Luis R. Rodriguez" <mcgrof@kernel.org>

I've been reviewing changes proactively, and plan on doing
more of this work. I'm doing this early as I should be getting
e-mailed about proposed changes.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 320cce87d6f8..38895e8b7b42 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4865,6 +4865,7 @@ F:	tools/firewire/
 
 FIRMWARE LOADER (request_firmware)
 M:	Ming Lei <ming.lei@canonical.com>
+M:	Luis R. Rodriguez <mcgrof@kernel.org>
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	Documentation/firmware_class/
-- 
2.9.2

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web