* [PATCH v2 00/11] firmware: cleanup for v4.17 @ 2018-02-24 2:46 Luis R. Rodriguez 2018-02-24 2:46 ` [PATCH v2 01/11] test_firmware: enable custom fallback testing on limited kernel configs Luis R. Rodriguez ` (10 more replies) 0 siblings, 11 replies; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez Greg, this v2 series addresses some corner case build issues. One was spotted late by 0-day after I used Linus' tree for build tests rather than linux-next, and another I found on my own. 0-day covers more ground with testing if you use Linus' tree. The one cought by 0-day was in that in the case when the firmware loader is modular (actually rare these days) the new sysctl table for the configuration of the fallback interface needs to be built in as well, as otherwise the proc sysctl interface can't access the configuration. The other build issue was a misplaced "static" on a fallback call. 0-day now gives its blessings for both, on linux-next and on Linus' tree. Questions, feedback, rants all equally welcomed. Luis Luis R. Rodriguez (11): test_firmware: enable custom fallback testing on limited kernel configs test_firmware: replace syfs fallback check with kconfig_has helper firmware: enable to split firmware_class into separate target files firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further firmware: use helpers for setting up a temporary cache timeout firmware: move loading timeout under struct firmware_fallback_config firmware: split firmware fallback functionality into its own file firmware: enable run time change of forcing fallback loader firmware: enable to force disable the fallback mechanism at run time test_firmware: add a library for shared helpers test_firmware: test three firmware kernel configs using a proc knob drivers/base/Makefile | 5 +- drivers/base/firmware_fallback.c | 667 ++++++++++++++++++ drivers/base/firmware_fallback.h | 67 ++ drivers/base/firmware_fallback_table.c | 55 ++ .../base/{firmware_class.c => firmware_loader.c} | 765 +-------------------- drivers/base/firmware_loader.h | 115 ++++ kernel/sysctl.c | 11 + tools/testing/selftests/firmware/Makefile | 2 +- tools/testing/selftests/firmware/config | 4 + tools/testing/selftests/firmware/fw_fallback.sh | 39 +- tools/testing/selftests/firmware/fw_filesystem.sh | 61 +- tools/testing/selftests/firmware/fw_lib.sh | 173 +++++ tools/testing/selftests/firmware/fw_run_tests.sh | 67 ++ 13 files changed, 1193 insertions(+), 838 deletions(-) create mode 100644 drivers/base/firmware_fallback.c create mode 100644 drivers/base/firmware_fallback.h create mode 100644 drivers/base/firmware_fallback_table.c rename drivers/base/{firmware_class.c => firmware_loader.c} (61%) create mode 100644 drivers/base/firmware_loader.h create mode 100755 tools/testing/selftests/firmware/fw_lib.sh create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh -- 2.16.2 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 01/11] test_firmware: enable custom fallback testing on limited kernel configs 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:07 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 02/11] test_firmware: replace syfs fallback check with kconfig_has helper Luis R. Rodriguez ` (9 subsequent siblings) 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez When a kernel is not built with: CONFIG_HAS_FW_LOADER_USER_HELPER_FALLBACK=y We don't currently enable testing fw_fallback.sh. For kernels that still enable the fallback mechanism, its possible to use the async request firmware API call request_firmware_nowait() using the custom interface to use the fallback mechanism, so we should be able to test this but we currently cannot. We can enable testing without CONFIG_HAS_FW_LOADER_USER_HELPER_FALLBACK=y by relying on /proc/config.gz (CONFIG_IKCONFIG_PROC), if present. If you don't have this we'll have no option but to rely on old heuristics for now. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- tools/testing/selftests/firmware/config | 4 +++ tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config index c8137f70e291..bf634dda0720 100644 --- a/tools/testing/selftests/firmware/config +++ b/tools/testing/selftests/firmware/config @@ -1 +1,5 @@ CONFIG_TEST_FIRMWARE=y +CONFIG_FW_LOADER=y +CONFIG_FW_LOADER_USER_HELPER=y +CONFIG_IKCONFIG=y +CONFIG_IKCONFIG_PROC=y diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh index 722cad91df74..a42e437363d9 100755 --- a/tools/testing/selftests/firmware/fw_fallback.sh +++ b/tools/testing/selftests/firmware/fw_fallback.sh @@ -6,7 +6,46 @@ # won't find so that we can do the load ourself manually. set -e +PROC_CONFIG="/proc/config.gz" +TEST_DIR=$(dirname $0) + modprobe test_firmware +if [ ! -f $PROC_CONFIG ]; then + if modprobe configs 2>/dev/null; then + echo "Loaded configs module" + if [ ! -f $PROC_CONFIG ]; then + echo "You must have the following enabled in your kernel:" >&2 + cat $TEST_DIR/config >&2 + echo "Resorting to old heuristics" >&2 + fi + else + echo "Failed to load configs module, using old heuristics" >&2 + fi +fi + +kconfig_has() +{ + if [ -f $PROC_CONFIG ]; then + if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then + echo "yes" + else + echo "no" + fi + else + # We currently don't have easy heuristics to infer this + # so best we can do is just try to use the kernel assuming + # you had enabled it. This matches the old behaviour. + if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then + echo "yes" + elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then + if [ -d /sys/class/firmware/ ]; then + echo yes + else + echo no + fi + fi + fi +} DIR=/sys/devices/virtual/misc/test_firmware @@ -14,6 +53,7 @@ DIR=/sys/devices/virtual/misc/test_firmware # These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that # as an indicator for CONFIG_FW_LOADER_USER_HELPER. HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi) +HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) @@ -286,7 +326,10 @@ run_sysfs_custom_load_tests() fi } -run_sysfs_main_tests +if [ "$HAS_FW_LOADER_USER_HELPER_FALLBACK" = "yes" ]; then + run_sysfs_main_tests +fi + run_sysfs_custom_load_tests exit 0 -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 01/11] test_firmware: enable custom fallback testing on limited kernel configs 2018-02-24 2:46 ` [PATCH v2 01/11] test_firmware: enable custom fallback testing on limited kernel configs Luis R. Rodriguez @ 2018-02-27 23:07 ` Kees Cook 0 siblings, 0 replies; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:07 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > When a kernel is not built with: > > CONFIG_HAS_FW_LOADER_USER_HELPER_FALLBACK=y > > We don't currently enable testing fw_fallback.sh. For kernels that > still enable the fallback mechanism, its possible to use the async > request firmware API call request_firmware_nowait() using the custom > interface to use the fallback mechanism, so we should be able to test > this but we currently cannot. > > We can enable testing without CONFIG_HAS_FW_LOADER_USER_HELPER_FALLBACK=y > by relying on /proc/config.gz (CONFIG_IKCONFIG_PROC), if present. If you > don't have this we'll have no option but to rely on old heuristics for now. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > tools/testing/selftests/firmware/config | 4 +++ > tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++- > 2 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config > index c8137f70e291..bf634dda0720 100644 > --- a/tools/testing/selftests/firmware/config > +++ b/tools/testing/selftests/firmware/config > @@ -1 +1,5 @@ > CONFIG_TEST_FIRMWARE=y > +CONFIG_FW_LOADER=y > +CONFIG_FW_LOADER_USER_HELPER=y > +CONFIG_IKCONFIG=y > +CONFIG_IKCONFIG_PROC=y > diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh > index 722cad91df74..a42e437363d9 100755 > --- a/tools/testing/selftests/firmware/fw_fallback.sh > +++ b/tools/testing/selftests/firmware/fw_fallback.sh > @@ -6,7 +6,46 @@ > # won't find so that we can do the load ourself manually. > set -e > > +PROC_CONFIG="/proc/config.gz" > +TEST_DIR=$(dirname $0) > + > modprobe test_firmware > +if [ ! -f $PROC_CONFIG ]; then > + if modprobe configs 2>/dev/null; then > + echo "Loaded configs module" > + if [ ! -f $PROC_CONFIG ]; then > + echo "You must have the following enabled in your kernel:" >&2 > + cat $TEST_DIR/config >&2 > + echo "Resorting to old heuristics" >&2 > + fi > + else > + echo "Failed to load configs module, using old heuristics" >&2 > + fi > +fi > + > +kconfig_has() > +{ > + if [ -f $PROC_CONFIG ]; then > + if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then > + echo "yes" > + else > + echo "no" > + fi > + else > + # We currently don't have easy heuristics to infer this > + # so best we can do is just try to use the kernel assuming > + # you had enabled it. This matches the old behaviour. > + if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then > + echo "yes" > + elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then > + if [ -d /sys/class/firmware/ ]; then > + echo yes > + else > + echo no > + fi > + fi > + fi > +} Some day when we have more formalized CONFIG-scanning for selftests, we can probably describe all of this better in a cleaner way. In the meantime, yeah, this seems fine. > > DIR=/sys/devices/virtual/misc/test_firmware > > @@ -14,6 +53,7 @@ DIR=/sys/devices/virtual/misc/test_firmware > # These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that > # as an indicator for CONFIG_FW_LOADER_USER_HELPER. > HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi) > +HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) > > if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) > @@ -286,7 +326,10 @@ run_sysfs_custom_load_tests() > fi > } > > -run_sysfs_main_tests > +if [ "$HAS_FW_LOADER_USER_HELPER_FALLBACK" = "yes" ]; then > + run_sysfs_main_tests > +fi > + > run_sysfs_custom_load_tests > > exit 0 > -- > 2.16.2 > Acked-by: Kees Cook <keescook@chromium.org> -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 02/11] test_firmware: replace syfs fallback check with kconfig_has helper 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez 2018-02-24 2:46 ` [PATCH v2 01/11] test_firmware: enable custom fallback testing on limited kernel configs Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:09 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 03/11] firmware: enable to split firmware_class into separate target files Luis R. Rodriguez ` (8 subsequent siblings) 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez Now that we have a kconfig checker just use that instead of relying on testing a sysfs directory being present, since our requirements are spelled out. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- tools/testing/selftests/firmware/fw_fallback.sh | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh index a42e437363d9..40b6c1d3e832 100755 --- a/tools/testing/selftests/firmware/fw_fallback.sh +++ b/tools/testing/selftests/firmware/fw_fallback.sh @@ -49,10 +49,7 @@ kconfig_has() DIR=/sys/devices/virtual/misc/test_firmware -# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/ -# These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that -# as an indicator for CONFIG_FW_LOADER_USER_HELPER. -HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi) +HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y) HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 02/11] test_firmware: replace syfs fallback check with kconfig_has helper 2018-02-24 2:46 ` [PATCH v2 02/11] test_firmware: replace syfs fallback check with kconfig_has helper Luis R. Rodriguez @ 2018-02-27 23:09 ` Kees Cook 0 siblings, 0 replies; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:09 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > Now that we have a kconfig checker just use that instead of relying > on testing a sysfs directory being present, since our requirements > are spelled out. I don't see the reason to depend on config.gz, but it's a reasonable requirement for a test build. > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > tools/testing/selftests/firmware/fw_fallback.sh | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh > index a42e437363d9..40b6c1d3e832 100755 > --- a/tools/testing/selftests/firmware/fw_fallback.sh > +++ b/tools/testing/selftests/firmware/fw_fallback.sh > @@ -49,10 +49,7 @@ kconfig_has() > > DIR=/sys/devices/virtual/misc/test_firmware > > -# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/ > -# These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that > -# as an indicator for CONFIG_FW_LOADER_USER_HELPER. > -HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi) > +HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y) > HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) > > if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > -- > 2.16.2 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 03/11] firmware: enable to split firmware_class into separate target files 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez 2018-02-24 2:46 ` [PATCH v2 01/11] test_firmware: enable custom fallback testing on limited kernel configs Luis R. Rodriguez 2018-02-24 2:46 ` [PATCH v2 02/11] test_firmware: replace syfs fallback check with kconfig_has helper Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-24 2:46 ` [PATCH v2 04/11] firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further Luis R. Rodriguez ` (7 subsequent siblings) 10 siblings, 0 replies; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez The firmware loader code has grown quite a bit over the years. The practice of stuffing everything we need into one file makes the code hard to follow. In order to split the firmware loader code into different components we must pick a module name and a first object target file. We must keep the firmware_class name to remain compatible with scripts which have been relying on the sysfs loader path for years, so the old module name stays. We can however rename the C file without affecting the module name. The firmware_class used to represent the idea that the code was a simple sysfs firmware loader, provided by the struct class firmware_class. The sysfs firmware loader used to be the default, today its only the fallback mechanism. This only renames the target code then to make emphasis of what the code does these days. With this change new features can also use a new object files. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/base/Makefile | 1 + drivers/base/{firmware_class.c => firmware_loader.c} | 0 2 files changed, 1 insertion(+) rename drivers/base/{firmware_class.c => firmware_loader.c} (100%) diff --git a/drivers/base/Makefile b/drivers/base/Makefile index e32a52490051..f261143fafbf 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_HAS_DMA) += dma-mapping.o obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o obj-$(CONFIG_ISA_BUS_API) += isa.o obj-$(CONFIG_FW_LOADER) += firmware_class.o +firmware_class-objs := firmware_loader.o obj-$(CONFIG_NUMA) += node.o obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o ifeq ($(CONFIG_SYSFS),y) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_loader.c similarity index 100% rename from drivers/base/firmware_class.c rename to drivers/base/firmware_loader.c -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 04/11] firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez ` (2 preceding siblings ...) 2018-02-24 2:46 ` [PATCH v2 03/11] firmware: enable to split firmware_class into separate target files Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:20 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 05/11] firmware: use helpers for setting up a temporary cache timeout Luis R. Rodriguez ` (6 subsequent siblings) 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez All CONFIG_FW_LOADER_USER_HELPER_FALLBACK really is, is just a bool, initailized at build time. Define it as such. This simplifies the logic even further, removing now all explicit #ifdefs around the code. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/base/firmware_loader.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c index 7dd36ace6152..59dba794ce1a 100644 --- a/drivers/base/firmware_loader.c +++ b/drivers/base/firmware_loader.c @@ -266,6 +266,22 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv) #ifdef CONFIG_FW_LOADER_USER_HELPER +/** + * struct firmware_fallback_config - firmware fallback configuratioon settings + * + * Helps describe and fine tune the fallback mechanism. + * + * @force_sysfs_fallback: force the sysfs fallback mechanism to be used + * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. + */ +struct firmware_fallback_config { + bool force_sysfs_fallback; +}; + +static const struct firmware_fallback_config fw_fallback_config = { + .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), +}; + static inline bool fw_sysfs_done(struct fw_priv *fw_priv) { return __fw_state_check(fw_priv, FW_STATUS_DONE); @@ -1151,19 +1167,14 @@ static int fw_load_from_user_helper(struct firmware *firmware, return ret; } -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK -static bool fw_force_sysfs_fallback(unsigned int opt_flags) -{ - return true; -} -#else static bool fw_force_sysfs_fallback(unsigned int opt_flags) { + if (fw_fallback_config.force_sysfs_fallback) + return true; if (!(opt_flags & FW_OPT_USERHELPER)) return false; return true; } -#endif static bool fw_run_sysfs_fallback(unsigned int opt_flags) { -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 04/11] firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further 2018-02-24 2:46 ` [PATCH v2 04/11] firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further Luis R. Rodriguez @ 2018-02-27 23:20 ` Kees Cook 0 siblings, 0 replies; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:20 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > All CONFIG_FW_LOADER_USER_HELPER_FALLBACK really is, is just a bool, > initailized at build time. Define it as such. This simplifies the > logic even further, removing now all explicit #ifdefs around the code. In the next revision, can you mention that struct firmware_fallback_config is going to grow in future patches. I spent too long trying to understand why this wasn't just a two line patch with only IS_ENABLED... added. :) Acked-by: Kees Cook <keescook@chromium.org> -Kees > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > drivers/base/firmware_loader.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c > index 7dd36ace6152..59dba794ce1a 100644 > --- a/drivers/base/firmware_loader.c > +++ b/drivers/base/firmware_loader.c > @@ -266,6 +266,22 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv) > > #ifdef CONFIG_FW_LOADER_USER_HELPER > > +/** > + * struct firmware_fallback_config - firmware fallback configuratioon settings > + * > + * Helps describe and fine tune the fallback mechanism. > + * > + * @force_sysfs_fallback: force the sysfs fallback mechanism to be used > + * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. > + */ > +struct firmware_fallback_config { > + bool force_sysfs_fallback; > +}; > + > +static const struct firmware_fallback_config fw_fallback_config = { > + .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), > +}; > + > static inline bool fw_sysfs_done(struct fw_priv *fw_priv) > { > return __fw_state_check(fw_priv, FW_STATUS_DONE); > @@ -1151,19 +1167,14 @@ static int fw_load_from_user_helper(struct firmware *firmware, > return ret; > } > > -#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK > -static bool fw_force_sysfs_fallback(unsigned int opt_flags) > -{ > - return true; > -} > -#else > static bool fw_force_sysfs_fallback(unsigned int opt_flags) > { > + if (fw_fallback_config.force_sysfs_fallback) > + return true; > if (!(opt_flags & FW_OPT_USERHELPER)) > return false; > return true; > } > -#endif > > static bool fw_run_sysfs_fallback(unsigned int opt_flags) > { > -- > 2.16.2 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 05/11] firmware: use helpers for setting up a temporary cache timeout 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez ` (3 preceding siblings ...) 2018-02-24 2:46 ` [PATCH v2 04/11] firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:20 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 06/11] firmware: move loading timeout under struct firmware_fallback_config Luis R. Rodriguez ` (5 subsequent siblings) 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez We only use the timeout for the firmware fallback mechanism except for trying to set the timeout during the cache setup for resume/suspend. For those cases, setting the timeout should be a no-op, so just reflect this in code by adding helpers for it. This change introduces no functional changes. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/base/firmware_loader.c | 49 ++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c index 59dba794ce1a..2d819875348d 100644 --- a/drivers/base/firmware_loader.c +++ b/drivers/base/firmware_loader.c @@ -191,13 +191,6 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw) } #endif -static int loading_timeout = 60; /* In seconds */ - -static inline long firmware_loading_timeout(void) -{ - return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET; -} - static void fw_state_init(struct fw_priv *fw_priv) { struct fw_state *fw_st = &fw_priv->fw_st; @@ -282,6 +275,32 @@ static const struct firmware_fallback_config fw_fallback_config = { .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), }; +static int old_timeout; +static int loading_timeout = 60; /* In seconds */ + +static inline long firmware_loading_timeout(void) +{ + return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET; +} + +/* + * use small loading timeout for caching devices' firmware because all these + * firmware images have been loaded successfully at lease once, also system is + * ready for completing firmware loading now. The maximum size of firmware in + * current distributions is about 2M bytes, so 10 secs should be enough. + */ +static void fw_fallback_set_cache_timeout(void) +{ + old_timeout = loading_timeout; + loading_timeout = 10; +} + +/* Restores the timeout to the value last configured during normal operation */ +static void fw_fallback_set_default_timeout(void) +{ + loading_timeout = old_timeout; +} + static inline bool fw_sysfs_done(struct fw_priv *fw_priv) { return __fw_state_check(fw_priv, FW_STATUS_DONE); @@ -1206,6 +1225,8 @@ static int fw_sysfs_fallback(struct firmware *fw, const char *name, } static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { } +static inline void fw_fallback_set_cache_timeout(void) { } +static inline void fw_fallback_set_default_timeout(void) { } static inline int register_sysfs_loader(void) { @@ -1752,7 +1773,6 @@ static void __device_uncache_fw_images(void) static void device_cache_fw_images(void) { struct firmware_cache *fwc = &fw_cache; - int old_timeout; DEFINE_WAIT(wait); pr_debug("%s\n", __func__); @@ -1760,16 +1780,7 @@ static void device_cache_fw_images(void) /* cancel uncache work */ cancel_delayed_work_sync(&fwc->work); - /* - * use small loading timeout for caching devices' firmware - * because all these firmware images have been loaded - * successfully at lease once, also system is ready for - * completing firmware loading now. The maximum size of - * firmware in current distributions is about 2M bytes, - * so 10 secs should be enough. - */ - old_timeout = loading_timeout; - loading_timeout = 10; + fw_fallback_set_cache_timeout(); mutex_lock(&fw_lock); fwc->state = FW_LOADER_START_CACHE; @@ -1779,7 +1790,7 @@ static void device_cache_fw_images(void) /* wait for completion of caching firmware for all devices */ async_synchronize_full_domain(&fw_cache_domain); - loading_timeout = old_timeout; + fw_fallback_set_default_timeout(); } /** -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 05/11] firmware: use helpers for setting up a temporary cache timeout 2018-02-24 2:46 ` [PATCH v2 05/11] firmware: use helpers for setting up a temporary cache timeout Luis R. Rodriguez @ 2018-02-27 23:20 ` Kees Cook 0 siblings, 0 replies; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:20 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > We only use the timeout for the firmware fallback mechanism > except for trying to set the timeout during the cache setup > for resume/suspend. For those cases, setting the timeout should > be a no-op, so just reflect this in code by adding helpers for it. > > This change introduces no functional changes. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > drivers/base/firmware_loader.c | 49 ++++++++++++++++++++++++++---------------- > 1 file changed, 30 insertions(+), 19 deletions(-) > > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c > index 59dba794ce1a..2d819875348d 100644 > --- a/drivers/base/firmware_loader.c > +++ b/drivers/base/firmware_loader.c > @@ -191,13 +191,6 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw) > } > #endif > > -static int loading_timeout = 60; /* In seconds */ > - > -static inline long firmware_loading_timeout(void) > -{ > - return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET; > -} > - > static void fw_state_init(struct fw_priv *fw_priv) > { > struct fw_state *fw_st = &fw_priv->fw_st; > @@ -282,6 +275,32 @@ static const struct firmware_fallback_config fw_fallback_config = { > .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), > }; > > +static int old_timeout; > +static int loading_timeout = 60; /* In seconds */ > + > +static inline long firmware_loading_timeout(void) > +{ > + return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET; > +} > + > +/* > + * use small loading timeout for caching devices' firmware because all these > + * firmware images have been loaded successfully at lease once, also system is > + * ready for completing firmware loading now. The maximum size of firmware in > + * current distributions is about 2M bytes, so 10 secs should be enough. > + */ > +static void fw_fallback_set_cache_timeout(void) > +{ > + old_timeout = loading_timeout; > + loading_timeout = 10; > +} > + > +/* Restores the timeout to the value last configured during normal operation */ > +static void fw_fallback_set_default_timeout(void) > +{ > + loading_timeout = old_timeout; > +} > + > static inline bool fw_sysfs_done(struct fw_priv *fw_priv) > { > return __fw_state_check(fw_priv, FW_STATUS_DONE); > @@ -1206,6 +1225,8 @@ static int fw_sysfs_fallback(struct firmware *fw, const char *name, > } > > static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { } > +static inline void fw_fallback_set_cache_timeout(void) { } > +static inline void fw_fallback_set_default_timeout(void) { } > > static inline int register_sysfs_loader(void) > { > @@ -1752,7 +1773,6 @@ static void __device_uncache_fw_images(void) > static void device_cache_fw_images(void) > { > struct firmware_cache *fwc = &fw_cache; > - int old_timeout; > DEFINE_WAIT(wait); > > pr_debug("%s\n", __func__); > @@ -1760,16 +1780,7 @@ static void device_cache_fw_images(void) > /* cancel uncache work */ > cancel_delayed_work_sync(&fwc->work); > > - /* > - * use small loading timeout for caching devices' firmware > - * because all these firmware images have been loaded > - * successfully at lease once, also system is ready for > - * completing firmware loading now. The maximum size of > - * firmware in current distributions is about 2M bytes, > - * so 10 secs should be enough. > - */ > - old_timeout = loading_timeout; > - loading_timeout = 10; > + fw_fallback_set_cache_timeout(); > > mutex_lock(&fw_lock); > fwc->state = FW_LOADER_START_CACHE; > @@ -1779,7 +1790,7 @@ static void device_cache_fw_images(void) > /* wait for completion of caching firmware for all devices */ > async_synchronize_full_domain(&fw_cache_domain); > > - loading_timeout = old_timeout; > + fw_fallback_set_default_timeout(); > } > > /** > -- > 2.16.2 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 06/11] firmware: move loading timeout under struct firmware_fallback_config 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez ` (4 preceding siblings ...) 2018-02-24 2:46 ` [PATCH v2 05/11] firmware: use helpers for setting up a temporary cache timeout Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:21 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file Luis R. Rodriguez ` (4 subsequent siblings) 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez The timeout is a fallback construct, so we can just stuff the timeout configuration under struct firmware_fallback_config. While at it, add a few helpers which vets the use of getting or setting the timeout as an int. The main use of the timeout is to set a timeout for completion, and that is used as an unsigned long. There a few cases however where it makes sense to get or set the timeout as an int, the helpers annotate these use cases have been properly vetted for. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/base/firmware_loader.c | 46 ++++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c index 2d819875348d..9757f9fff01e 100644 --- a/drivers/base/firmware_loader.c +++ b/drivers/base/firmware_loader.c @@ -266,21 +266,38 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv) * * @force_sysfs_fallback: force the sysfs fallback mechanism to be used * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. + * @old_timeout: for internal use + * @loading_timeout: the timeout to wait for the fallback mechanism before + * giving up, in seconds. */ struct firmware_fallback_config { - bool force_sysfs_fallback; + const bool force_sysfs_fallback; + int old_timeout; + int loading_timeout; }; -static const struct firmware_fallback_config fw_fallback_config = { +static struct firmware_fallback_config fw_fallback_config = { .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), + .loading_timeout = 60, + .old_timeout = 60, }; -static int old_timeout; -static int loading_timeout = 60; /* In seconds */ +/* These getters are vetted to use int properly */ +static inline int __firmware_loading_timeout(void) +{ + return fw_fallback_config.loading_timeout; +} + +/* These setters are vetted to use int properly */ +static void __fw_fallback_set_timeout(int timeout) +{ + fw_fallback_config.loading_timeout = timeout; +} static inline long firmware_loading_timeout(void) { - return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET; + return __firmware_loading_timeout() > 0 ? + __firmware_loading_timeout() * HZ : MAX_JIFFY_OFFSET; } /* @@ -291,14 +308,14 @@ static inline long firmware_loading_timeout(void) */ static void fw_fallback_set_cache_timeout(void) { - old_timeout = loading_timeout; - loading_timeout = 10; + fw_fallback_config.old_timeout = __firmware_loading_timeout(); + __fw_fallback_set_timeout(10); } /* Restores the timeout to the value last configured during normal operation */ static void fw_fallback_set_default_timeout(void) { - loading_timeout = old_timeout; + __fw_fallback_set_timeout(fw_fallback_config.old_timeout); } static inline bool fw_sysfs_done(struct fw_priv *fw_priv) @@ -677,7 +694,7 @@ static void kill_pending_fw_fallback_reqs(bool only_kill_custom) static ssize_t timeout_show(struct class *class, struct class_attribute *attr, char *buf) { - return sprintf(buf, "%d\n", loading_timeout); + return sprintf(buf, "%d\n", __firmware_loading_timeout()); } /** @@ -696,9 +713,12 @@ static ssize_t timeout_show(struct class *class, struct class_attribute *attr, static ssize_t timeout_store(struct class *class, struct class_attribute *attr, const char *buf, size_t count) { - loading_timeout = simple_strtol(buf, NULL, 10); - if (loading_timeout < 0) - loading_timeout = 0; + int tmp_loading_timeout = simple_strtol(buf, NULL, 10); + + if (tmp_loading_timeout < 0) + tmp_loading_timeout = 0; + + __fw_fallback_set_timeout(tmp_loading_timeout); return count; } @@ -721,7 +741,7 @@ static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, struct kobj_uevent_env { if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_name)) return -ENOMEM; - if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout)) + if (add_uevent_var(env, "TIMEOUT=%i", __firmware_loading_timeout())) return -ENOMEM; if (add_uevent_var(env, "ASYNC=%d", fw_sysfs->nowait)) return -ENOMEM; -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 06/11] firmware: move loading timeout under struct firmware_fallback_config 2018-02-24 2:46 ` [PATCH v2 06/11] firmware: move loading timeout under struct firmware_fallback_config Luis R. Rodriguez @ 2018-02-27 23:21 ` Kees Cook 0 siblings, 0 replies; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:21 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > The timeout is a fallback construct, so we can just stuff the > timeout configuration under struct firmware_fallback_config. > > While at it, add a few helpers which vets the use of getting or > setting the timeout as an int. The main use of the timeout is > to set a timeout for completion, and that is used as an unsigned > long. There a few cases however where it makes sense to get or > set the timeout as an int, the helpers annotate these use cases > have been properly vetted for. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > drivers/base/firmware_loader.c | 46 ++++++++++++++++++++++++++++++------------ > 1 file changed, 33 insertions(+), 13 deletions(-) > > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c > index 2d819875348d..9757f9fff01e 100644 > --- a/drivers/base/firmware_loader.c > +++ b/drivers/base/firmware_loader.c > @@ -266,21 +266,38 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv) > * > * @force_sysfs_fallback: force the sysfs fallback mechanism to be used > * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. > + * @old_timeout: for internal use > + * @loading_timeout: the timeout to wait for the fallback mechanism before > + * giving up, in seconds. > */ > struct firmware_fallback_config { > - bool force_sysfs_fallback; > + const bool force_sysfs_fallback; > + int old_timeout; > + int loading_timeout; > }; > > -static const struct firmware_fallback_config fw_fallback_config = { > +static struct firmware_fallback_config fw_fallback_config = { > .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), > + .loading_timeout = 60, > + .old_timeout = 60, > }; > > -static int old_timeout; > -static int loading_timeout = 60; /* In seconds */ > +/* These getters are vetted to use int properly */ > +static inline int __firmware_loading_timeout(void) > +{ > + return fw_fallback_config.loading_timeout; > +} > + > +/* These setters are vetted to use int properly */ > +static void __fw_fallback_set_timeout(int timeout) > +{ > + fw_fallback_config.loading_timeout = timeout; > +} > > static inline long firmware_loading_timeout(void) > { > - return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET; > + return __firmware_loading_timeout() > 0 ? > + __firmware_loading_timeout() * HZ : MAX_JIFFY_OFFSET; > } > > /* > @@ -291,14 +308,14 @@ static inline long firmware_loading_timeout(void) > */ > static void fw_fallback_set_cache_timeout(void) > { > - old_timeout = loading_timeout; > - loading_timeout = 10; > + fw_fallback_config.old_timeout = __firmware_loading_timeout(); > + __fw_fallback_set_timeout(10); > } > > /* Restores the timeout to the value last configured during normal operation */ > static void fw_fallback_set_default_timeout(void) > { > - loading_timeout = old_timeout; > + __fw_fallback_set_timeout(fw_fallback_config.old_timeout); > } > > static inline bool fw_sysfs_done(struct fw_priv *fw_priv) > @@ -677,7 +694,7 @@ static void kill_pending_fw_fallback_reqs(bool only_kill_custom) > static ssize_t timeout_show(struct class *class, struct class_attribute *attr, > char *buf) > { > - return sprintf(buf, "%d\n", loading_timeout); > + return sprintf(buf, "%d\n", __firmware_loading_timeout()); > } > > /** > @@ -696,9 +713,12 @@ static ssize_t timeout_show(struct class *class, struct class_attribute *attr, > static ssize_t timeout_store(struct class *class, struct class_attribute *attr, > const char *buf, size_t count) > { > - loading_timeout = simple_strtol(buf, NULL, 10); > - if (loading_timeout < 0) > - loading_timeout = 0; > + int tmp_loading_timeout = simple_strtol(buf, NULL, 10); > + > + if (tmp_loading_timeout < 0) > + tmp_loading_timeout = 0; > + > + __fw_fallback_set_timeout(tmp_loading_timeout); > > return count; > } > @@ -721,7 +741,7 @@ static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, struct kobj_uevent_env > { > if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_name)) > return -ENOMEM; > - if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout)) > + if (add_uevent_var(env, "TIMEOUT=%i", __firmware_loading_timeout())) > return -ENOMEM; > if (add_uevent_var(env, "ASYNC=%d", fw_sysfs->nowait)) > return -ENOMEM; > -- > 2.16.2 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez ` (5 preceding siblings ...) 2018-02-24 2:46 ` [PATCH v2 06/11] firmware: move loading timeout under struct firmware_fallback_config Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:14 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 08/11] firmware: enable run time change of forcing fallback loader Luis R. Rodriguez ` (3 subsequent siblings) 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez The firmware fallback code is optional. Split that code out to help distinguish the fallback functionlity from othere core firmware loader features. This should make it easier to maintain and review code changes. The reason for keeping the configuration onto a table which is built-in if you enable firmware loading is so that we can later enable the kernel after subsequent patches to tweak this configuration, even if the firmware loader is modular. This introduces no functional changes. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/base/Makefile | 4 +- drivers/base/firmware_fallback.c | 661 +++++++++++++++++++++++++++ drivers/base/firmware_fallback.h | 61 +++ drivers/base/firmware_fallback_table.c | 29 ++ drivers/base/firmware_loader.c | 803 +-------------------------------- drivers/base/firmware_loader.h | 115 +++++ 6 files changed, 874 insertions(+), 799 deletions(-) create mode 100644 drivers/base/firmware_fallback.c create mode 100644 drivers/base/firmware_fallback.h create mode 100644 drivers/base/firmware_fallback_table.c create mode 100644 drivers/base/firmware_loader.h diff --git a/drivers/base/Makefile b/drivers/base/Makefile index f261143fafbf..b946a408256d 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -5,7 +5,8 @@ obj-y := component.o core.o bus.o dd.o syscore.o \ driver.o class.o platform.o \ cpu.o firmware.o init.o map.o devres.o \ attribute_container.o transport_class.o \ - topology.o container.o property.o cacheinfo.o + topology.o container.o property.o cacheinfo.o \ + firmware_fallback_table.o obj-$(CONFIG_DEVTMPFS) += devtmpfs.o obj-$(CONFIG_DMA_CMA) += dma-contiguous.o obj-y += power/ @@ -14,6 +15,7 @@ obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o obj-$(CONFIG_ISA_BUS_API) += isa.o obj-$(CONFIG_FW_LOADER) += firmware_class.o firmware_class-objs := firmware_loader.o +firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += firmware_fallback.o obj-$(CONFIG_NUMA) += node.o obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o ifeq ($(CONFIG_SYSFS),y) diff --git a/drivers/base/firmware_fallback.c b/drivers/base/firmware_fallback.c new file mode 100644 index 000000000000..47690207e0ee --- /dev/null +++ b/drivers/base/firmware_fallback.c @@ -0,0 +1,661 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/types.h> +#include <linux/kconfig.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/security.h> +#include <linux/highmem.h> +#include <linux/umh.h> + +#include "firmware_fallback.h" +#include "firmware_loader.h" + +/* + * firmware fallback mechanism + */ + +extern struct firmware_fallback_config fw_fallback_config; + +/* These getters are vetted to use int properly */ +static inline int __firmware_loading_timeout(void) +{ + return fw_fallback_config.loading_timeout; +} + +/* These setters are vetted to use int properly */ +static void __fw_fallback_set_timeout(int timeout) +{ + fw_fallback_config.loading_timeout = timeout; +} + +/* + * use small loading timeout for caching devices' firmware because all these + * firmware images have been loaded successfully at lease once, also system is + * ready for completing firmware loading now. The maximum size of firmware in + * current distributions is about 2M bytes, so 10 secs should be enough. + */ +void fw_fallback_set_cache_timeout(void) +{ + fw_fallback_config.old_timeout = __firmware_loading_timeout(); + __fw_fallback_set_timeout(10); +} + +/* Restores the timeout to the value last configured during normal operation */ +void fw_fallback_set_default_timeout(void) +{ + __fw_fallback_set_timeout(fw_fallback_config.old_timeout); +} + +static long firmware_loading_timeout(void) +{ + return __firmware_loading_timeout() > 0 ? + __firmware_loading_timeout() * HZ : MAX_JIFFY_OFFSET; +} + +static inline bool fw_sysfs_done(struct fw_priv *fw_priv) +{ + return __fw_state_check(fw_priv, FW_STATUS_DONE); +} + +static inline bool fw_sysfs_loading(struct fw_priv *fw_priv) +{ + return __fw_state_check(fw_priv, FW_STATUS_LOADING); +} + +static inline int fw_sysfs_wait_timeout(struct fw_priv *fw_priv, long timeout) +{ + return __fw_state_wait_common(fw_priv, timeout); +} + +struct fw_sysfs { + bool nowait; + struct device dev; + struct fw_priv *fw_priv; + struct firmware *fw; +}; + +static struct fw_sysfs *to_fw_sysfs(struct device *dev) +{ + return container_of(dev, struct fw_sysfs, dev); +} + +static void __fw_load_abort(struct fw_priv *fw_priv) +{ + /* + * There is a small window in which user can write to 'loading' + * between loading done and disappearance of 'loading' + */ + if (fw_sysfs_done(fw_priv)) + return; + + list_del_init(&fw_priv->pending_list); + fw_state_aborted(fw_priv); +} + +static void fw_load_abort(struct fw_sysfs *fw_sysfs) +{ + struct fw_priv *fw_priv = fw_sysfs->fw_priv; + + __fw_load_abort(fw_priv); +} + +static LIST_HEAD(pending_fw_head); + +void kill_pending_fw_fallback_reqs(bool only_kill_custom) +{ + struct fw_priv *fw_priv; + struct fw_priv *next; + + mutex_lock(&fw_lock); + list_for_each_entry_safe(fw_priv, next, &pending_fw_head, + pending_list) { + if (!fw_priv->need_uevent || !only_kill_custom) + __fw_load_abort(fw_priv); + } + mutex_unlock(&fw_lock); +} + +static ssize_t timeout_show(struct class *class, struct class_attribute *attr, + char *buf) +{ + return sprintf(buf, "%d\n", __firmware_loading_timeout()); +} + +/** + * firmware_timeout_store - set number of seconds to wait for firmware + * @class: device class pointer + * @attr: device attribute pointer + * @buf: buffer to scan for timeout value + * @count: number of bytes in @buf + * + * Sets the number of seconds to wait for the firmware. Once + * this expires an error will be returned to the driver and no + * firmware will be provided. + * + * Note: zero means 'wait forever'. + **/ +static ssize_t timeout_store(struct class *class, struct class_attribute *attr, + const char *buf, size_t count) +{ + int tmp_loading_timeout = simple_strtol(buf, NULL, 10); + + if (tmp_loading_timeout < 0) + tmp_loading_timeout = 0; + + __fw_fallback_set_timeout(tmp_loading_timeout); + + return count; +} +static CLASS_ATTR_RW(timeout); + +static struct attribute *firmware_class_attrs[] = { + &class_attr_timeout.attr, + NULL, +}; +ATTRIBUTE_GROUPS(firmware_class); + +static void fw_dev_release(struct device *dev) +{ + struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); + + kfree(fw_sysfs); +} + +static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, struct kobj_uevent_env *env) +{ + if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_name)) + return -ENOMEM; + if (add_uevent_var(env, "TIMEOUT=%i", __firmware_loading_timeout())) + return -ENOMEM; + if (add_uevent_var(env, "ASYNC=%d", fw_sysfs->nowait)) + return -ENOMEM; + + return 0; +} + +static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env) +{ + struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); + int err = 0; + + mutex_lock(&fw_lock); + if (fw_sysfs->fw_priv) + err = do_firmware_uevent(fw_sysfs, env); + mutex_unlock(&fw_lock); + return err; +} + +static struct class firmware_class = { + .name = "firmware", + .class_groups = firmware_class_groups, + .dev_uevent = firmware_uevent, + .dev_release = fw_dev_release, +}; + +int register_sysfs_loader(void) +{ + return class_register(&firmware_class); +} + +void unregister_sysfs_loader(void) +{ + class_unregister(&firmware_class); +} + +static ssize_t firmware_loading_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); + int loading = 0; + + mutex_lock(&fw_lock); + if (fw_sysfs->fw_priv) + loading = fw_sysfs_loading(fw_sysfs->fw_priv); + mutex_unlock(&fw_lock); + + return sprintf(buf, "%d\n", loading); +} + +/* Some architectures don't have PAGE_KERNEL_RO */ +#ifndef PAGE_KERNEL_RO +#define PAGE_KERNEL_RO PAGE_KERNEL +#endif + +/* one pages buffer should be mapped/unmapped only once */ +static int map_fw_priv_pages(struct fw_priv *fw_priv) +{ + if (!fw_priv->is_paged_buf) + return 0; + + vunmap(fw_priv->data); + fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages, 0, + PAGE_KERNEL_RO); + if (!fw_priv->data) + return -ENOMEM; + return 0; +} + +/** + * firmware_loading_store - set value in the 'loading' control file + * @dev: device pointer + * @attr: device attribute pointer + * @buf: buffer to scan for loading control value + * @count: number of bytes in @buf + * + * The relevant values are: + * + * 1: Start a load, discarding any previous partial load. + * 0: Conclude the load and hand the data to the driver code. + * -1: Conclude the load with an error and discard any written data. + **/ +static ssize_t firmware_loading_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); + struct fw_priv *fw_priv; + ssize_t written = count; + int loading = simple_strtol(buf, NULL, 10); + int i; + + mutex_lock(&fw_lock); + fw_priv = fw_sysfs->fw_priv; + if (fw_state_is_aborted(fw_priv)) + goto out; + + switch (loading) { + case 1: + /* discarding any previous partial load */ + if (!fw_sysfs_done(fw_priv)) { + for (i = 0; i < fw_priv->nr_pages; i++) + __free_page(fw_priv->pages[i]); + vfree(fw_priv->pages); + fw_priv->pages = NULL; + fw_priv->page_array_size = 0; + fw_priv->nr_pages = 0; + fw_state_start(fw_priv); + } + break; + case 0: + if (fw_sysfs_loading(fw_priv)) { + int rc; + + /* + * Several loading requests may be pending on + * one same firmware buf, so let all requests + * see the mapped 'buf->data' once the loading + * is completed. + * */ + rc = map_fw_priv_pages(fw_priv); + if (rc) + dev_err(dev, "%s: map pages failed\n", + __func__); + else + rc = security_kernel_post_read_file(NULL, + fw_priv->data, fw_priv->size, + READING_FIRMWARE); + + /* + * Same logic as fw_load_abort, only the DONE bit + * is ignored and we set ABORT only on failure. + */ + list_del_init(&fw_priv->pending_list); + if (rc) { + fw_state_aborted(fw_priv); + written = rc; + } else { + fw_state_done(fw_priv); + } + break; + } + /* fallthrough */ + default: + dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading); + /* fallthrough */ + case -1: + fw_load_abort(fw_sysfs); + break; + } +out: + mutex_unlock(&fw_lock); + return written; +} + +static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store); + +static void firmware_rw_data(struct fw_priv *fw_priv, char *buffer, + loff_t offset, size_t count, bool read) +{ + if (read) + memcpy(buffer, fw_priv->data + offset, count); + else + memcpy(fw_priv->data + offset, buffer, count); +} + +static void firmware_rw(struct fw_priv *fw_priv, char *buffer, + loff_t offset, size_t count, bool read) +{ + while (count) { + void *page_data; + int page_nr = offset >> PAGE_SHIFT; + int page_ofs = offset & (PAGE_SIZE-1); + int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count); + + page_data = kmap(fw_priv->pages[page_nr]); + + if (read) + memcpy(buffer, page_data + page_ofs, page_cnt); + else + memcpy(page_data + page_ofs, buffer, page_cnt); + + kunmap(fw_priv->pages[page_nr]); + buffer += page_cnt; + offset += page_cnt; + count -= page_cnt; + } +} + +static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, + char *buffer, loff_t offset, size_t count) +{ + struct device *dev = kobj_to_dev(kobj); + struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); + struct fw_priv *fw_priv; + ssize_t ret_count; + + mutex_lock(&fw_lock); + fw_priv = fw_sysfs->fw_priv; + if (!fw_priv || fw_sysfs_done(fw_priv)) { + ret_count = -ENODEV; + goto out; + } + if (offset > fw_priv->size) { + ret_count = 0; + goto out; + } + if (count > fw_priv->size - offset) + count = fw_priv->size - offset; + + ret_count = count; + + if (fw_priv->data) + firmware_rw_data(fw_priv, buffer, offset, count, true); + else + firmware_rw(fw_priv, buffer, offset, count, true); + +out: + mutex_unlock(&fw_lock); + return ret_count; +} + +static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int min_size) +{ + struct fw_priv *fw_priv= fw_sysfs->fw_priv; + int pages_needed = PAGE_ALIGN(min_size) >> PAGE_SHIFT; + + /* If the array of pages is too small, grow it... */ + if (fw_priv->page_array_size < pages_needed) { + int new_array_size = max(pages_needed, + fw_priv->page_array_size * 2); + struct page **new_pages; + + new_pages = vmalloc(new_array_size * sizeof(void *)); + if (!new_pages) { + fw_load_abort(fw_sysfs); + return -ENOMEM; + } + memcpy(new_pages, fw_priv->pages, + fw_priv->page_array_size * sizeof(void *)); + memset(&new_pages[fw_priv->page_array_size], 0, sizeof(void *) * + (new_array_size - fw_priv->page_array_size)); + vfree(fw_priv->pages); + fw_priv->pages = new_pages; + fw_priv->page_array_size = new_array_size; + } + + while (fw_priv->nr_pages < pages_needed) { + fw_priv->pages[fw_priv->nr_pages] = + alloc_page(GFP_KERNEL | __GFP_HIGHMEM); + + if (!fw_priv->pages[fw_priv->nr_pages]) { + fw_load_abort(fw_sysfs); + return -ENOMEM; + } + fw_priv->nr_pages++; + } + return 0; +} + +/** + * firmware_data_write - write method for firmware + * @filp: open sysfs file + * @kobj: kobject for the device + * @bin_attr: bin_attr structure + * @buffer: buffer being written + * @offset: buffer offset for write in total data store area + * @count: buffer size + * + * Data written to the 'data' attribute will be later handed to + * the driver as a firmware image. + **/ +static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, + char *buffer, loff_t offset, size_t count) +{ + struct device *dev = kobj_to_dev(kobj); + struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); + struct fw_priv *fw_priv; + ssize_t retval; + + if (!capable(CAP_SYS_RAWIO)) + return -EPERM; + + mutex_lock(&fw_lock); + fw_priv = fw_sysfs->fw_priv; + if (!fw_priv || fw_sysfs_done(fw_priv)) { + retval = -ENODEV; + goto out; + } + + if (fw_priv->data) { + if (offset + count > fw_priv->allocated_size) { + retval = -ENOMEM; + goto out; + } + firmware_rw_data(fw_priv, buffer, offset, count, false); + retval = count; + } else { + retval = fw_realloc_pages(fw_sysfs, offset + count); + if (retval) + goto out; + + retval = count; + firmware_rw(fw_priv, buffer, offset, count, false); + } + + fw_priv->size = max_t(size_t, offset + count, fw_priv->size); +out: + mutex_unlock(&fw_lock); + return retval; +} + +static struct bin_attribute firmware_attr_data = { + .attr = { .name = "data", .mode = 0644 }, + .size = 0, + .read = firmware_data_read, + .write = firmware_data_write, +}; + +static struct attribute *fw_dev_attrs[] = { + &dev_attr_loading.attr, + NULL +}; + +static struct bin_attribute *fw_dev_bin_attrs[] = { + &firmware_attr_data, + NULL +}; + +static const struct attribute_group fw_dev_attr_group = { + .attrs = fw_dev_attrs, + .bin_attrs = fw_dev_bin_attrs, +}; + +static const struct attribute_group *fw_dev_attr_groups[] = { + &fw_dev_attr_group, + NULL +}; + +static struct fw_sysfs * +fw_create_instance(struct firmware *firmware, const char *fw_name, + struct device *device, unsigned int opt_flags) +{ + struct fw_sysfs *fw_sysfs; + struct device *f_dev; + + fw_sysfs = kzalloc(sizeof(*fw_sysfs), GFP_KERNEL); + if (!fw_sysfs) { + fw_sysfs = ERR_PTR(-ENOMEM); + goto exit; + } + + fw_sysfs->nowait = !!(opt_flags & FW_OPT_NOWAIT); + fw_sysfs->fw = firmware; + f_dev = &fw_sysfs->dev; + + device_initialize(f_dev); + dev_set_name(f_dev, "%s", fw_name); + f_dev->parent = device; + f_dev->class = &firmware_class; + f_dev->groups = fw_dev_attr_groups; +exit: + return fw_sysfs; +} + +/* load a firmware via user helper */ +static int _request_firmware_load(struct fw_sysfs *fw_sysfs, + unsigned int opt_flags, long timeout) +{ + int retval = 0; + struct device *f_dev = &fw_sysfs->dev; + struct fw_priv *fw_priv = fw_sysfs->fw_priv; + + /* fall back on userspace loading */ + if (!fw_priv->data) + fw_priv->is_paged_buf = true; + + dev_set_uevent_suppress(f_dev, true); + + retval = device_add(f_dev); + if (retval) { + dev_err(f_dev, "%s: device_register failed\n", __func__); + goto err_put_dev; + } + + mutex_lock(&fw_lock); + list_add(&fw_priv->pending_list, &pending_fw_head); + mutex_unlock(&fw_lock); + + if (opt_flags & FW_OPT_UEVENT) { + fw_priv->need_uevent = true; + dev_set_uevent_suppress(f_dev, false); + dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_name); + kobject_uevent(&fw_sysfs->dev.kobj, KOBJ_ADD); + } else { + timeout = MAX_JIFFY_OFFSET; + } + + retval = fw_sysfs_wait_timeout(fw_priv, timeout); + if (retval < 0) { + mutex_lock(&fw_lock); + fw_load_abort(fw_sysfs); + mutex_unlock(&fw_lock); + } + + if (fw_state_is_aborted(fw_priv)) { + if (retval == -ERESTARTSYS) + retval = -EINTR; + else + retval = -EAGAIN; + } else if (fw_priv->is_paged_buf && !fw_priv->data) + retval = -ENOMEM; + + device_del(f_dev); +err_put_dev: + put_device(f_dev); + return retval; +} + +static int fw_load_from_user_helper(struct firmware *firmware, + const char *name, struct device *device, + unsigned int opt_flags) +{ + struct fw_sysfs *fw_sysfs; + long timeout; + int ret; + + timeout = firmware_loading_timeout(); + if (opt_flags & FW_OPT_NOWAIT) { + timeout = usermodehelper_read_lock_wait(timeout); + if (!timeout) { + dev_dbg(device, "firmware: %s loading timed out\n", + name); + return -EBUSY; + } + } else { + ret = usermodehelper_read_trylock(); + if (WARN_ON(ret)) { + dev_err(device, "firmware: %s will not be loaded\n", + name); + return ret; + } + } + + fw_sysfs = fw_create_instance(firmware, name, device, opt_flags); + if (IS_ERR(fw_sysfs)) { + ret = PTR_ERR(fw_sysfs); + goto out_unlock; + } + + fw_sysfs->fw_priv = firmware->priv; + ret = _request_firmware_load(fw_sysfs, opt_flags, timeout); + + if (!ret) + ret = assign_fw(firmware, device, opt_flags); + +out_unlock: + usermodehelper_read_unlock(); + + return ret; +} + +static bool fw_force_sysfs_fallback(unsigned int opt_flags) +{ + if (fw_fallback_config.force_sysfs_fallback) + return true; + if (!(opt_flags & FW_OPT_USERHELPER)) + return false; + return true; +} + +static bool fw_run_sysfs_fallback(unsigned int opt_flags) +{ + if ((opt_flags & FW_OPT_NOFALLBACK)) + return false; + + return fw_force_sysfs_fallback(opt_flags); +} + +int fw_sysfs_fallback(struct firmware *fw, const char *name, + struct device *device, + unsigned int opt_flags, + int ret) +{ + if (!fw_run_sysfs_fallback(opt_flags)) + return ret; + + dev_warn(device, "Falling back to user helper\n"); + return fw_load_from_user_helper(fw, name, device, opt_flags); +} diff --git a/drivers/base/firmware_fallback.h b/drivers/base/firmware_fallback.h new file mode 100644 index 000000000000..550498c7fa4c --- /dev/null +++ b/drivers/base/firmware_fallback.h @@ -0,0 +1,61 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __FIRMWARE_FALLBACK_H +#define __FIRMWARE_FALLBACK_H + +#include <linux/firmware.h> +#include <linux/device.h> + +/** + * struct firmware_fallback_config - firmware fallback configuratioon settings + * + * Helps describe and fine tune the fallback mechanism. + * + * @force_sysfs_fallback: force the sysfs fallback mechanism to be used + * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. + * @old_timeout: for internal use + * @loading_timeout: the timeout to wait for the fallback mechanism before + * giving up, in seconds. + */ +struct firmware_fallback_config { + const bool force_sysfs_fallback; + int old_timeout; + int loading_timeout; +}; + +#ifdef CONFIG_FW_LOADER_USER_HELPER +int fw_sysfs_fallback(struct firmware *fw, const char *name, + struct device *device, + unsigned int opt_flags, + int ret); +void kill_pending_fw_fallback_reqs(bool only_kill_custom); + +void fw_fallback_set_cache_timeout(void); +void fw_fallback_set_default_timeout(void); + +int register_sysfs_loader(void); +void unregister_sysfs_loader(void); +#else /* CONFIG_FW_LOADER_USER_HELPER */ +static inline int fw_sysfs_fallback(struct firmware *fw, const char *name, + struct device *device, + unsigned int opt_flags, + int ret) +{ + /* Keep carrying over the same error */ + return ret; +} + +static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { } +static inline void fw_fallback_set_cache_timeout(void) { } +static inline void fw_fallback_set_default_timeout(void) { } + +static inline int register_sysfs_loader(void) +{ + return 0; +} + +static inline void unregister_sysfs_loader(void) +{ +} +#endif /* CONFIG_FW_LOADER_USER_HELPER */ + +#endif /* __FIRMWARE_FALLBACK_H */ diff --git a/drivers/base/firmware_fallback_table.c b/drivers/base/firmware_fallback_table.c new file mode 100644 index 000000000000..53cc4e492520 --- /dev/null +++ b/drivers/base/firmware_fallback_table.c @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/types.h> +#include <linux/kconfig.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/security.h> +#include <linux/highmem.h> +#include <linux/umh.h> +#include <linux/sysctl.h> + +#include "firmware_fallback.h" +#include "firmware_loader.h" + +/* + * firmware fallback configuration table + */ + +/* Module or buit-in */ +#ifdef CONFIG_FW_LOADER_USER_HELPER + +struct firmware_fallback_config fw_fallback_config = { + .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), + .loading_timeout = 60, + .old_timeout = 60, +}; +EXPORT_SYMBOL_GPL(fw_fallback_config); + +#endif diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c index 9757f9fff01e..21dd31ef08ae 100644 --- a/drivers/base/firmware_loader.c +++ b/drivers/base/firmware_loader.c @@ -37,36 +37,13 @@ #include <generated/utsrelease.h> #include "base.h" +#include "firmware_loader.h" +#include "firmware_fallback.h" MODULE_AUTHOR("Manuel Estrada Sainz"); MODULE_DESCRIPTION("Multi purpose firmware loading support"); MODULE_LICENSE("GPL"); -enum fw_status { - FW_STATUS_UNKNOWN, - FW_STATUS_LOADING, - FW_STATUS_DONE, - FW_STATUS_ABORTED, -}; - -/* - * Concurrent request_firmware() for the same firmware need to be - * serialized. struct fw_state is simple state machine which hold the - * state of the firmware loading. - */ -struct fw_state { - struct completion completion; - enum fw_status status; -}; - -/* firmware behavior options */ -#define FW_OPT_UEVENT (1U << 0) -#define FW_OPT_NOWAIT (1U << 1) -#define FW_OPT_USERHELPER (1U << 2) -#define FW_OPT_NO_WARN (1U << 3) -#define FW_OPT_NOCACHE (1U << 4) -#define FW_OPT_NOFALLBACK (1U << 5) - struct firmware_cache { /* firmware_buf instance will be added into the below list */ spinlock_t lock; @@ -89,25 +66,6 @@ struct firmware_cache { #endif }; -struct fw_priv { - struct kref ref; - struct list_head list; - struct firmware_cache *fwc; - struct fw_state fw_st; - void *data; - size_t size; - size_t allocated_size; -#ifdef CONFIG_FW_LOADER_USER_HELPER - bool is_paged_buf; - bool need_uevent; - struct page **pages; - int nr_pages; - int page_array_size; - struct list_head pending_list; -#endif - const char *fw_name; -}; - struct fw_cache_entry { struct list_head list; const char *name; @@ -128,7 +86,7 @@ static inline struct fw_priv *to_fw_priv(struct kref *ref) /* fw_lock could be moved to 'struct fw_sysfs' but since it is just * guarding for corner cases a global lock should be OK */ -static DEFINE_MUTEX(fw_lock); +DEFINE_MUTEX(fw_lock); static struct firmware_cache fw_cache; @@ -199,142 +157,11 @@ static void fw_state_init(struct fw_priv *fw_priv) fw_st->status = FW_STATUS_UNKNOWN; } -static int __fw_state_wait_common(struct fw_priv *fw_priv, long timeout) -{ - struct fw_state *fw_st = &fw_priv->fw_st; - long ret; - - ret = wait_for_completion_killable_timeout(&fw_st->completion, timeout); - if (ret != 0 && fw_st->status == FW_STATUS_ABORTED) - return -ENOENT; - if (!ret) - return -ETIMEDOUT; - - return ret < 0 ? ret : 0; -} - -static void __fw_state_set(struct fw_priv *fw_priv, - enum fw_status status) -{ - struct fw_state *fw_st = &fw_priv->fw_st; - - WRITE_ONCE(fw_st->status, status); - - if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED) - complete_all(&fw_st->completion); -} - -static inline void fw_state_start(struct fw_priv *fw_priv) -{ - __fw_state_set(fw_priv, FW_STATUS_LOADING); -} - -static inline void fw_state_done(struct fw_priv *fw_priv) -{ - __fw_state_set(fw_priv, FW_STATUS_DONE); -} - -static inline void fw_state_aborted(struct fw_priv *fw_priv) -{ - __fw_state_set(fw_priv, FW_STATUS_ABORTED); -} - static inline int fw_state_wait(struct fw_priv *fw_priv) { return __fw_state_wait_common(fw_priv, MAX_SCHEDULE_TIMEOUT); } -static bool __fw_state_check(struct fw_priv *fw_priv, - enum fw_status status) -{ - struct fw_state *fw_st = &fw_priv->fw_st; - - return fw_st->status == status; -} - -static inline bool fw_state_is_aborted(struct fw_priv *fw_priv) -{ - return __fw_state_check(fw_priv, FW_STATUS_ABORTED); -} - -#ifdef CONFIG_FW_LOADER_USER_HELPER - -/** - * struct firmware_fallback_config - firmware fallback configuratioon settings - * - * Helps describe and fine tune the fallback mechanism. - * - * @force_sysfs_fallback: force the sysfs fallback mechanism to be used - * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. - * @old_timeout: for internal use - * @loading_timeout: the timeout to wait for the fallback mechanism before - * giving up, in seconds. - */ -struct firmware_fallback_config { - const bool force_sysfs_fallback; - int old_timeout; - int loading_timeout; -}; - -static struct firmware_fallback_config fw_fallback_config = { - .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), - .loading_timeout = 60, - .old_timeout = 60, -}; - -/* These getters are vetted to use int properly */ -static inline int __firmware_loading_timeout(void) -{ - return fw_fallback_config.loading_timeout; -} - -/* These setters are vetted to use int properly */ -static void __fw_fallback_set_timeout(int timeout) -{ - fw_fallback_config.loading_timeout = timeout; -} - -static inline long firmware_loading_timeout(void) -{ - return __firmware_loading_timeout() > 0 ? - __firmware_loading_timeout() * HZ : MAX_JIFFY_OFFSET; -} - -/* - * use small loading timeout for caching devices' firmware because all these - * firmware images have been loaded successfully at lease once, also system is - * ready for completing firmware loading now. The maximum size of firmware in - * current distributions is about 2M bytes, so 10 secs should be enough. - */ -static void fw_fallback_set_cache_timeout(void) -{ - fw_fallback_config.old_timeout = __firmware_loading_timeout(); - __fw_fallback_set_timeout(10); -} - -/* Restores the timeout to the value last configured during normal operation */ -static void fw_fallback_set_default_timeout(void) -{ - __fw_fallback_set_timeout(fw_fallback_config.old_timeout); -} - -static inline bool fw_sysfs_done(struct fw_priv *fw_priv) -{ - return __fw_state_check(fw_priv, FW_STATUS_DONE); -} - -static inline bool fw_sysfs_loading(struct fw_priv *fw_priv) -{ - return __fw_state_check(fw_priv, FW_STATUS_LOADING); -} - -static inline int fw_sysfs_wait_timeout(struct fw_priv *fw_priv, long timeout) -{ - return __fw_state_wait_common(fw_priv, timeout); -} - -#endif /* CONFIG_FW_LOADER_USER_HELPER */ - static int fw_cache_piggyback_on_request(const char *name); static struct fw_priv *__allocate_fw_priv(const char *fw_name, @@ -600,8 +427,8 @@ static int fw_add_devm_name(struct device *dev, const char *name) } #endif -static int assign_fw(struct firmware *fw, struct device *device, - unsigned int opt_flags) +int assign_fw(struct firmware *fw, struct device *device, + unsigned int opt_flags) { struct fw_priv *fw_priv = fw->priv; @@ -639,626 +466,6 @@ static int assign_fw(struct firmware *fw, struct device *device, return 0; } -/* - * user-mode helper code - */ -#ifdef CONFIG_FW_LOADER_USER_HELPER -struct fw_sysfs { - bool nowait; - struct device dev; - struct fw_priv *fw_priv; - struct firmware *fw; -}; - -static struct fw_sysfs *to_fw_sysfs(struct device *dev) -{ - return container_of(dev, struct fw_sysfs, dev); -} - -static void __fw_load_abort(struct fw_priv *fw_priv) -{ - /* - * There is a small window in which user can write to 'loading' - * between loading done and disappearance of 'loading' - */ - if (fw_sysfs_done(fw_priv)) - return; - - list_del_init(&fw_priv->pending_list); - fw_state_aborted(fw_priv); -} - -static void fw_load_abort(struct fw_sysfs *fw_sysfs) -{ - struct fw_priv *fw_priv = fw_sysfs->fw_priv; - - __fw_load_abort(fw_priv); -} - -static LIST_HEAD(pending_fw_head); - -static void kill_pending_fw_fallback_reqs(bool only_kill_custom) -{ - struct fw_priv *fw_priv; - struct fw_priv *next; - - mutex_lock(&fw_lock); - list_for_each_entry_safe(fw_priv, next, &pending_fw_head, - pending_list) { - if (!fw_priv->need_uevent || !only_kill_custom) - __fw_load_abort(fw_priv); - } - mutex_unlock(&fw_lock); -} - -static ssize_t timeout_show(struct class *class, struct class_attribute *attr, - char *buf) -{ - return sprintf(buf, "%d\n", __firmware_loading_timeout()); -} - -/** - * firmware_timeout_store - set number of seconds to wait for firmware - * @class: device class pointer - * @attr: device attribute pointer - * @buf: buffer to scan for timeout value - * @count: number of bytes in @buf - * - * Sets the number of seconds to wait for the firmware. Once - * this expires an error will be returned to the driver and no - * firmware will be provided. - * - * Note: zero means 'wait forever'. - **/ -static ssize_t timeout_store(struct class *class, struct class_attribute *attr, - const char *buf, size_t count) -{ - int tmp_loading_timeout = simple_strtol(buf, NULL, 10); - - if (tmp_loading_timeout < 0) - tmp_loading_timeout = 0; - - __fw_fallback_set_timeout(tmp_loading_timeout); - - return count; -} -static CLASS_ATTR_RW(timeout); - -static struct attribute *firmware_class_attrs[] = { - &class_attr_timeout.attr, - NULL, -}; -ATTRIBUTE_GROUPS(firmware_class); - -static void fw_dev_release(struct device *dev) -{ - struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); - - kfree(fw_sysfs); -} - -static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, struct kobj_uevent_env *env) -{ - if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_name)) - return -ENOMEM; - if (add_uevent_var(env, "TIMEOUT=%i", __firmware_loading_timeout())) - return -ENOMEM; - if (add_uevent_var(env, "ASYNC=%d", fw_sysfs->nowait)) - return -ENOMEM; - - return 0; -} - -static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env) -{ - struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); - int err = 0; - - mutex_lock(&fw_lock); - if (fw_sysfs->fw_priv) - err = do_firmware_uevent(fw_sysfs, env); - mutex_unlock(&fw_lock); - return err; -} - -static struct class firmware_class = { - .name = "firmware", - .class_groups = firmware_class_groups, - .dev_uevent = firmware_uevent, - .dev_release = fw_dev_release, -}; - -static inline int register_sysfs_loader(void) -{ - return class_register(&firmware_class); -} - -static inline void unregister_sysfs_loader(void) -{ - class_unregister(&firmware_class); -} - -static ssize_t firmware_loading_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); - int loading = 0; - - mutex_lock(&fw_lock); - if (fw_sysfs->fw_priv) - loading = fw_sysfs_loading(fw_sysfs->fw_priv); - mutex_unlock(&fw_lock); - - return sprintf(buf, "%d\n", loading); -} - -/* Some architectures don't have PAGE_KERNEL_RO */ -#ifndef PAGE_KERNEL_RO -#define PAGE_KERNEL_RO PAGE_KERNEL -#endif - -/* one pages buffer should be mapped/unmapped only once */ -static int map_fw_priv_pages(struct fw_priv *fw_priv) -{ - if (!fw_priv->is_paged_buf) - return 0; - - vunmap(fw_priv->data); - fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages, 0, - PAGE_KERNEL_RO); - if (!fw_priv->data) - return -ENOMEM; - return 0; -} - -/** - * firmware_loading_store - set value in the 'loading' control file - * @dev: device pointer - * @attr: device attribute pointer - * @buf: buffer to scan for loading control value - * @count: number of bytes in @buf - * - * The relevant values are: - * - * 1: Start a load, discarding any previous partial load. - * 0: Conclude the load and hand the data to the driver code. - * -1: Conclude the load with an error and discard any written data. - **/ -static ssize_t firmware_loading_store(struct device *dev, - struct device_attribute *attr, - const char *buf, size_t count) -{ - struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); - struct fw_priv *fw_priv; - ssize_t written = count; - int loading = simple_strtol(buf, NULL, 10); - int i; - - mutex_lock(&fw_lock); - fw_priv = fw_sysfs->fw_priv; - if (fw_state_is_aborted(fw_priv)) - goto out; - - switch (loading) { - case 1: - /* discarding any previous partial load */ - if (!fw_sysfs_done(fw_priv)) { - for (i = 0; i < fw_priv->nr_pages; i++) - __free_page(fw_priv->pages[i]); - vfree(fw_priv->pages); - fw_priv->pages = NULL; - fw_priv->page_array_size = 0; - fw_priv->nr_pages = 0; - fw_state_start(fw_priv); - } - break; - case 0: - if (fw_sysfs_loading(fw_priv)) { - int rc; - - /* - * Several loading requests may be pending on - * one same firmware buf, so let all requests - * see the mapped 'buf->data' once the loading - * is completed. - * */ - rc = map_fw_priv_pages(fw_priv); - if (rc) - dev_err(dev, "%s: map pages failed\n", - __func__); - else - rc = security_kernel_post_read_file(NULL, - fw_priv->data, fw_priv->size, - READING_FIRMWARE); - - /* - * Same logic as fw_load_abort, only the DONE bit - * is ignored and we set ABORT only on failure. - */ - list_del_init(&fw_priv->pending_list); - if (rc) { - fw_state_aborted(fw_priv); - written = rc; - } else { - fw_state_done(fw_priv); - } - break; - } - /* fallthrough */ - default: - dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading); - /* fallthrough */ - case -1: - fw_load_abort(fw_sysfs); - break; - } -out: - mutex_unlock(&fw_lock); - return written; -} - -static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store); - -static void firmware_rw_data(struct fw_priv *fw_priv, char *buffer, - loff_t offset, size_t count, bool read) -{ - if (read) - memcpy(buffer, fw_priv->data + offset, count); - else - memcpy(fw_priv->data + offset, buffer, count); -} - -static void firmware_rw(struct fw_priv *fw_priv, char *buffer, - loff_t offset, size_t count, bool read) -{ - while (count) { - void *page_data; - int page_nr = offset >> PAGE_SHIFT; - int page_ofs = offset & (PAGE_SIZE-1); - int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count); - - page_data = kmap(fw_priv->pages[page_nr]); - - if (read) - memcpy(buffer, page_data + page_ofs, page_cnt); - else - memcpy(page_data + page_ofs, buffer, page_cnt); - - kunmap(fw_priv->pages[page_nr]); - buffer += page_cnt; - offset += page_cnt; - count -= page_cnt; - } -} - -static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj, - struct bin_attribute *bin_attr, - char *buffer, loff_t offset, size_t count) -{ - struct device *dev = kobj_to_dev(kobj); - struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); - struct fw_priv *fw_priv; - ssize_t ret_count; - - mutex_lock(&fw_lock); - fw_priv = fw_sysfs->fw_priv; - if (!fw_priv || fw_sysfs_done(fw_priv)) { - ret_count = -ENODEV; - goto out; - } - if (offset > fw_priv->size) { - ret_count = 0; - goto out; - } - if (count > fw_priv->size - offset) - count = fw_priv->size - offset; - - ret_count = count; - - if (fw_priv->data) - firmware_rw_data(fw_priv, buffer, offset, count, true); - else - firmware_rw(fw_priv, buffer, offset, count, true); - -out: - mutex_unlock(&fw_lock); - return ret_count; -} - -static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int min_size) -{ - struct fw_priv *fw_priv= fw_sysfs->fw_priv; - int pages_needed = PAGE_ALIGN(min_size) >> PAGE_SHIFT; - - /* If the array of pages is too small, grow it... */ - if (fw_priv->page_array_size < pages_needed) { - int new_array_size = max(pages_needed, - fw_priv->page_array_size * 2); - struct page **new_pages; - - new_pages = vmalloc(new_array_size * sizeof(void *)); - if (!new_pages) { - fw_load_abort(fw_sysfs); - return -ENOMEM; - } - memcpy(new_pages, fw_priv->pages, - fw_priv->page_array_size * sizeof(void *)); - memset(&new_pages[fw_priv->page_array_size], 0, sizeof(void *) * - (new_array_size - fw_priv->page_array_size)); - vfree(fw_priv->pages); - fw_priv->pages = new_pages; - fw_priv->page_array_size = new_array_size; - } - - while (fw_priv->nr_pages < pages_needed) { - fw_priv->pages[fw_priv->nr_pages] = - alloc_page(GFP_KERNEL | __GFP_HIGHMEM); - - if (!fw_priv->pages[fw_priv->nr_pages]) { - fw_load_abort(fw_sysfs); - return -ENOMEM; - } - fw_priv->nr_pages++; - } - return 0; -} - -/** - * firmware_data_write - write method for firmware - * @filp: open sysfs file - * @kobj: kobject for the device - * @bin_attr: bin_attr structure - * @buffer: buffer being written - * @offset: buffer offset for write in total data store area - * @count: buffer size - * - * Data written to the 'data' attribute will be later handed to - * the driver as a firmware image. - **/ -static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj, - struct bin_attribute *bin_attr, - char *buffer, loff_t offset, size_t count) -{ - struct device *dev = kobj_to_dev(kobj); - struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev); - struct fw_priv *fw_priv; - ssize_t retval; - - if (!capable(CAP_SYS_RAWIO)) - return -EPERM; - - mutex_lock(&fw_lock); - fw_priv = fw_sysfs->fw_priv; - if (!fw_priv || fw_sysfs_done(fw_priv)) { - retval = -ENODEV; - goto out; - } - - if (fw_priv->data) { - if (offset + count > fw_priv->allocated_size) { - retval = -ENOMEM; - goto out; - } - firmware_rw_data(fw_priv, buffer, offset, count, false); - retval = count; - } else { - retval = fw_realloc_pages(fw_sysfs, offset + count); - if (retval) - goto out; - - retval = count; - firmware_rw(fw_priv, buffer, offset, count, false); - } - - fw_priv->size = max_t(size_t, offset + count, fw_priv->size); -out: - mutex_unlock(&fw_lock); - return retval; -} - -static struct bin_attribute firmware_attr_data = { - .attr = { .name = "data", .mode = 0644 }, - .size = 0, - .read = firmware_data_read, - .write = firmware_data_write, -}; - -static struct attribute *fw_dev_attrs[] = { - &dev_attr_loading.attr, - NULL -}; - -static struct bin_attribute *fw_dev_bin_attrs[] = { - &firmware_attr_data, - NULL -}; - -static const struct attribute_group fw_dev_attr_group = { - .attrs = fw_dev_attrs, - .bin_attrs = fw_dev_bin_attrs, -}; - -static const struct attribute_group *fw_dev_attr_groups[] = { - &fw_dev_attr_group, - NULL -}; - -static struct fw_sysfs * -fw_create_instance(struct firmware *firmware, const char *fw_name, - struct device *device, unsigned int opt_flags) -{ - struct fw_sysfs *fw_sysfs; - struct device *f_dev; - - fw_sysfs = kzalloc(sizeof(*fw_sysfs), GFP_KERNEL); - if (!fw_sysfs) { - fw_sysfs = ERR_PTR(-ENOMEM); - goto exit; - } - - fw_sysfs->nowait = !!(opt_flags & FW_OPT_NOWAIT); - fw_sysfs->fw = firmware; - f_dev = &fw_sysfs->dev; - - device_initialize(f_dev); - dev_set_name(f_dev, "%s", fw_name); - f_dev->parent = device; - f_dev->class = &firmware_class; - f_dev->groups = fw_dev_attr_groups; -exit: - return fw_sysfs; -} - -/* load a firmware via user helper */ -static int _request_firmware_load(struct fw_sysfs *fw_sysfs, - unsigned int opt_flags, long timeout) -{ - int retval = 0; - struct device *f_dev = &fw_sysfs->dev; - struct fw_priv *fw_priv = fw_sysfs->fw_priv; - - /* fall back on userspace loading */ - if (!fw_priv->data) - fw_priv->is_paged_buf = true; - - dev_set_uevent_suppress(f_dev, true); - - retval = device_add(f_dev); - if (retval) { - dev_err(f_dev, "%s: device_register failed\n", __func__); - goto err_put_dev; - } - - mutex_lock(&fw_lock); - list_add(&fw_priv->pending_list, &pending_fw_head); - mutex_unlock(&fw_lock); - - if (opt_flags & FW_OPT_UEVENT) { - fw_priv->need_uevent = true; - dev_set_uevent_suppress(f_dev, false); - dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_name); - kobject_uevent(&fw_sysfs->dev.kobj, KOBJ_ADD); - } else { - timeout = MAX_JIFFY_OFFSET; - } - - retval = fw_sysfs_wait_timeout(fw_priv, timeout); - if (retval < 0) { - mutex_lock(&fw_lock); - fw_load_abort(fw_sysfs); - mutex_unlock(&fw_lock); - } - - if (fw_state_is_aborted(fw_priv)) { - if (retval == -ERESTARTSYS) - retval = -EINTR; - else - retval = -EAGAIN; - } else if (fw_priv->is_paged_buf && !fw_priv->data) - retval = -ENOMEM; - - device_del(f_dev); -err_put_dev: - put_device(f_dev); - return retval; -} - -static int fw_load_from_user_helper(struct firmware *firmware, - const char *name, struct device *device, - unsigned int opt_flags) -{ - struct fw_sysfs *fw_sysfs; - long timeout; - int ret; - - timeout = firmware_loading_timeout(); - if (opt_flags & FW_OPT_NOWAIT) { - timeout = usermodehelper_read_lock_wait(timeout); - if (!timeout) { - dev_dbg(device, "firmware: %s loading timed out\n", - name); - return -EBUSY; - } - } else { - ret = usermodehelper_read_trylock(); - if (WARN_ON(ret)) { - dev_err(device, "firmware: %s will not be loaded\n", - name); - return ret; - } - } - - fw_sysfs = fw_create_instance(firmware, name, device, opt_flags); - if (IS_ERR(fw_sysfs)) { - ret = PTR_ERR(fw_sysfs); - goto out_unlock; - } - - fw_sysfs->fw_priv = firmware->priv; - ret = _request_firmware_load(fw_sysfs, opt_flags, timeout); - - if (!ret) - ret = assign_fw(firmware, device, opt_flags); - -out_unlock: - usermodehelper_read_unlock(); - - return ret; -} - -static bool fw_force_sysfs_fallback(unsigned int opt_flags) -{ - if (fw_fallback_config.force_sysfs_fallback) - return true; - if (!(opt_flags & FW_OPT_USERHELPER)) - return false; - return true; -} - -static bool fw_run_sysfs_fallback(unsigned int opt_flags) -{ - if ((opt_flags & FW_OPT_NOFALLBACK)) - return false; - - return fw_force_sysfs_fallback(opt_flags); -} - -static int fw_sysfs_fallback(struct firmware *fw, const char *name, - struct device *device, - unsigned int opt_flags, - int ret) -{ - if (!fw_run_sysfs_fallback(opt_flags)) - return ret; - - dev_warn(device, "Falling back to user helper\n"); - return fw_load_from_user_helper(fw, name, device, opt_flags); -} -#else /* CONFIG_FW_LOADER_USER_HELPER */ -static int fw_sysfs_fallback(struct firmware *fw, const char *name, - struct device *device, - unsigned int opt_flags, - int ret) -{ - /* Keep carrying over the same error */ - return ret; -} - -static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { } -static inline void fw_fallback_set_cache_timeout(void) { } -static inline void fw_fallback_set_default_timeout(void) { } - -static inline int register_sysfs_loader(void) -{ - return 0; -} - -static inline void unregister_sysfs_loader(void) -{ -} - -#endif /* CONFIG_FW_LOADER_USER_HELPER */ - /* prepare firmware and firmware_buf structs; * return 0 if a firmware is already assigned, 1 if need to load one, * or a negative error code diff --git a/drivers/base/firmware_loader.h b/drivers/base/firmware_loader.h new file mode 100644 index 000000000000..64acbb1a392c --- /dev/null +++ b/drivers/base/firmware_loader.h @@ -0,0 +1,115 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __FIRMWARE_LOADER_H +#define __FIRMWARE_LOADER_H + +#include <linux/firmware.h> +#include <linux/types.h> +#include <linux/kref.h> +#include <linux/list.h> +#include <linux/completion.h> + +#include <generated/utsrelease.h> + +/* firmware behavior options */ +#define FW_OPT_UEVENT (1U << 0) +#define FW_OPT_NOWAIT (1U << 1) +#define FW_OPT_USERHELPER (1U << 2) +#define FW_OPT_NO_WARN (1U << 3) +#define FW_OPT_NOCACHE (1U << 4) +#define FW_OPT_NOFALLBACK (1U << 5) + +enum fw_status { + FW_STATUS_UNKNOWN, + FW_STATUS_LOADING, + FW_STATUS_DONE, + FW_STATUS_ABORTED, +}; + +/* + * Concurrent request_firmware() for the same firmware need to be + * serialized. struct fw_state is simple state machine which hold the + * state of the firmware loading. + */ +struct fw_state { + struct completion completion; + enum fw_status status; +}; + +struct fw_priv { + struct kref ref; + struct list_head list; + struct firmware_cache *fwc; + struct fw_state fw_st; + void *data; + size_t size; + size_t allocated_size; +#ifdef CONFIG_FW_LOADER_USER_HELPER + bool is_paged_buf; + bool need_uevent; + struct page **pages; + int nr_pages; + int page_array_size; + struct list_head pending_list; +#endif + const char *fw_name; +}; + +extern struct mutex fw_lock; + +static inline bool __fw_state_check(struct fw_priv *fw_priv, + enum fw_status status) +{ + struct fw_state *fw_st = &fw_priv->fw_st; + + return fw_st->status == status; +} + +static inline int __fw_state_wait_common(struct fw_priv *fw_priv, long timeout) +{ + struct fw_state *fw_st = &fw_priv->fw_st; + long ret; + + ret = wait_for_completion_killable_timeout(&fw_st->completion, timeout); + if (ret != 0 && fw_st->status == FW_STATUS_ABORTED) + return -ENOENT; + if (!ret) + return -ETIMEDOUT; + + return ret < 0 ? ret : 0; +} + +static inline void __fw_state_set(struct fw_priv *fw_priv, + enum fw_status status) +{ + struct fw_state *fw_st = &fw_priv->fw_st; + + WRITE_ONCE(fw_st->status, status); + + if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED) + complete_all(&fw_st->completion); +} + +static inline void fw_state_aborted(struct fw_priv *fw_priv) +{ + __fw_state_set(fw_priv, FW_STATUS_ABORTED); +} + +static inline bool fw_state_is_aborted(struct fw_priv *fw_priv) +{ + return __fw_state_check(fw_priv, FW_STATUS_ABORTED); +} + +static inline void fw_state_start(struct fw_priv *fw_priv) +{ + __fw_state_set(fw_priv, FW_STATUS_LOADING); +} + +static inline void fw_state_done(struct fw_priv *fw_priv) +{ + __fw_state_set(fw_priv, FW_STATUS_DONE); +} + +int assign_fw(struct firmware *fw, struct device *device, + unsigned int opt_flags); + +#endif /* __FIRMWARE_LOADER_H */ -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file 2018-02-24 2:46 ` [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file Luis R. Rodriguez @ 2018-02-27 23:14 ` Kees Cook 2018-02-28 1:28 ` Luis R. Rodriguez 0 siblings, 1 reply; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:14 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > The firmware fallback code is optional. Split that code out to help > distinguish the fallback functionlity from othere core firmware loader > features. This should make it easier to maintain and review code > changes. > > The reason for keeping the configuration onto a table which is built-in > if you enable firmware loading is so that we can later enable the kernel > after subsequent patches to tweak this configuration, even if the > firmware loader is modular. > > This introduces no functional changes. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > drivers/base/Makefile | 4 +- > drivers/base/firmware_fallback.c | 661 +++++++++++++++++++++++++++ > drivers/base/firmware_fallback.h | 61 +++ > drivers/base/firmware_fallback_table.c | 29 ++ > drivers/base/firmware_loader.c | 803 +-------------------------------- > drivers/base/firmware_loader.h | 115 +++++ > 6 files changed, 874 insertions(+), 799 deletions(-) > create mode 100644 drivers/base/firmware_fallback.c > create mode 100644 drivers/base/firmware_fallback.h > create mode 100644 drivers/base/firmware_fallback_table.c > create mode 100644 drivers/base/firmware_loader.h Does it make sense to have a separate subdirectory for firmware instead? I did this _ stuff with lkdtm and have regretted it. (I'm likely going to make a subdirectory for it this cycle...) -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file 2018-02-27 23:14 ` Kees Cook @ 2018-02-28 1:28 ` Luis R. Rodriguez 2018-02-28 5:33 ` Kees Cook 0 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-28 1:28 UTC (permalink / raw) To: Kees Cook Cc: Luis R. Rodriguez, Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Tue, Feb 27, 2018 at 03:14:53PM -0800, Kees Cook wrote: > On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > The firmware fallback code is optional. Split that code out to help > > distinguish the fallback functionlity from othere core firmware loader > > features. This should make it easier to maintain and review code > > changes. > > > > The reason for keeping the configuration onto a table which is built-in > > if you enable firmware loading is so that we can later enable the kernel > > after subsequent patches to tweak this configuration, even if the > > firmware loader is modular. > > > > This introduces no functional changes. > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > --- > > drivers/base/Makefile | 4 +- > > drivers/base/firmware_fallback.c | 661 +++++++++++++++++++++++++++ > > drivers/base/firmware_fallback.h | 61 +++ > > drivers/base/firmware_fallback_table.c | 29 ++ > > drivers/base/firmware_loader.c | 803 +-------------------------------- > > drivers/base/firmware_loader.h | 115 +++++ > > 6 files changed, 874 insertions(+), 799 deletions(-) > > create mode 100644 drivers/base/firmware_fallback.c > > create mode 100644 drivers/base/firmware_fallback.h > > create mode 100644 drivers/base/firmware_fallback_table.c > > create mode 100644 drivers/base/firmware_loader.h > > Does it make sense to have a separate subdirectory for firmware > instead? I did this _ stuff with lkdtm and have regretted it. (I'm > likely going to make a subdirectory for it this cycle...) Sure, the only eyesore is that drivers/base/firmware.c what is that for? drivers/base/firmware_loader/ ok? Luis ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file 2018-02-28 1:28 ` Luis R. Rodriguez @ 2018-02-28 5:33 ` Kees Cook 2018-02-28 7:11 ` Greg KH 0 siblings, 1 reply; 34+ messages in thread From: Kees Cook @ 2018-02-28 5:33 UTC (permalink / raw) To: Luis R. Rodriguez, Greg KH Cc: Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Tue, Feb 27, 2018 at 5:28 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Tue, Feb 27, 2018 at 03:14:53PM -0800, Kees Cook wrote: >> On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >> > The firmware fallback code is optional. Split that code out to help >> > distinguish the fallback functionlity from othere core firmware loader >> > features. This should make it easier to maintain and review code >> > changes. >> > >> > The reason for keeping the configuration onto a table which is built-in >> > if you enable firmware loading is so that we can later enable the kernel >> > after subsequent patches to tweak this configuration, even if the >> > firmware loader is modular. >> > >> > This introduces no functional changes. >> > >> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> >> > --- >> > drivers/base/Makefile | 4 +- >> > drivers/base/firmware_fallback.c | 661 +++++++++++++++++++++++++++ >> > drivers/base/firmware_fallback.h | 61 +++ >> > drivers/base/firmware_fallback_table.c | 29 ++ >> > drivers/base/firmware_loader.c | 803 +-------------------------------- >> > drivers/base/firmware_loader.h | 115 +++++ >> > 6 files changed, 874 insertions(+), 799 deletions(-) >> > create mode 100644 drivers/base/firmware_fallback.c >> > create mode 100644 drivers/base/firmware_fallback.h >> > create mode 100644 drivers/base/firmware_fallback_table.c >> > create mode 100644 drivers/base/firmware_loader.h >> >> Does it make sense to have a separate subdirectory for firmware >> instead? I did this _ stuff with lkdtm and have regretted it. (I'm >> likely going to make a subdirectory for it this cycle...) > > Sure, the only eyesore is that drivers/base/firmware.c what is that for? > > drivers/base/firmware_loader/ ok? Yeah? Seems fine to me. Greg, do you have thoughts on this? -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file 2018-02-28 5:33 ` Kees Cook @ 2018-02-28 7:11 ` Greg KH 2018-03-08 3:44 ` Luis R. Rodriguez 0 siblings, 1 reply; 34+ messages in thread From: Greg KH @ 2018-02-28 7:11 UTC (permalink / raw) To: Kees Cook Cc: Luis R. Rodriguez, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Tue, Feb 27, 2018 at 09:33:28PM -0800, Kees Cook wrote: > On Tue, Feb 27, 2018 at 5:28 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > On Tue, Feb 27, 2018 at 03:14:53PM -0800, Kees Cook wrote: > >> On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > >> > The firmware fallback code is optional. Split that code out to help > >> > distinguish the fallback functionlity from othere core firmware loader > >> > features. This should make it easier to maintain and review code > >> > changes. > >> > > >> > The reason for keeping the configuration onto a table which is built-in > >> > if you enable firmware loading is so that we can later enable the kernel > >> > after subsequent patches to tweak this configuration, even if the > >> > firmware loader is modular. > >> > > >> > This introduces no functional changes. > >> > > >> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > >> > --- > >> > drivers/base/Makefile | 4 +- > >> > drivers/base/firmware_fallback.c | 661 +++++++++++++++++++++++++++ > >> > drivers/base/firmware_fallback.h | 61 +++ > >> > drivers/base/firmware_fallback_table.c | 29 ++ > >> > drivers/base/firmware_loader.c | 803 +-------------------------------- > >> > drivers/base/firmware_loader.h | 115 +++++ > >> > 6 files changed, 874 insertions(+), 799 deletions(-) > >> > create mode 100644 drivers/base/firmware_fallback.c > >> > create mode 100644 drivers/base/firmware_fallback.h > >> > create mode 100644 drivers/base/firmware_fallback_table.c > >> > create mode 100644 drivers/base/firmware_loader.h > >> > >> Does it make sense to have a separate subdirectory for firmware > >> instead? I did this _ stuff with lkdtm and have regretted it. (I'm > >> likely going to make a subdirectory for it this cycle...) > > > > Sure, the only eyesore is that drivers/base/firmware.c what is that for? > > > > drivers/base/firmware_loader/ ok? > > Yeah? Seems fine to me. Greg, do you have thoughts on this? I don't care :) ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file 2018-02-28 7:11 ` Greg KH @ 2018-03-08 3:44 ` Luis R. Rodriguez 0 siblings, 0 replies; 34+ messages in thread From: Luis R. Rodriguez @ 2018-03-08 3:44 UTC (permalink / raw) To: Greg KH Cc: Kees Cook, Luis R. Rodriguez, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Wed, Feb 28, 2018 at 08:11:31AM +0100, Greg KH wrote: > On Tue, Feb 27, 2018 at 09:33:28PM -0800, Kees Cook wrote: > > On Tue, Feb 27, 2018 at 5:28 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > > On Tue, Feb 27, 2018 at 03:14:53PM -0800, Kees Cook wrote: > > >> On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > >> > The firmware fallback code is optional. Split that code out to help > > >> > distinguish the fallback functionlity from othere core firmware loader > > >> > features. This should make it easier to maintain and review code > > >> > changes. > > >> > > > >> > The reason for keeping the configuration onto a table which is built-in > > >> > if you enable firmware loading is so that we can later enable the kernel > > >> > after subsequent patches to tweak this configuration, even if the > > >> > firmware loader is modular. > > >> > > > >> > This introduces no functional changes. > > >> > > > >> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > >> > --- > > >> > drivers/base/Makefile | 4 +- > > >> > drivers/base/firmware_fallback.c | 661 +++++++++++++++++++++++++++ > > >> > drivers/base/firmware_fallback.h | 61 +++ > > >> > drivers/base/firmware_fallback_table.c | 29 ++ > > >> > drivers/base/firmware_loader.c | 803 +-------------------------------- > > >> > drivers/base/firmware_loader.h | 115 +++++ > > >> > 6 files changed, 874 insertions(+), 799 deletions(-) > > >> > create mode 100644 drivers/base/firmware_fallback.c > > >> > create mode 100644 drivers/base/firmware_fallback.h > > >> > create mode 100644 drivers/base/firmware_fallback_table.c > > >> > create mode 100644 drivers/base/firmware_loader.h > > >> > > >> Does it make sense to have a separate subdirectory for firmware > > >> instead? I did this _ stuff with lkdtm and have regretted it. (I'm > > >> likely going to make a subdirectory for it this cycle...) > > > > > > Sure, the only eyesore is that drivers/base/firmware.c what is that for? > > > > > > drivers/base/firmware_loader/ ok? > > > > Yeah? Seems fine to me. Greg, do you have thoughts on this? > > I don't care :) Alright, this was much easier done as a separate patch after. Luis ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 08/11] firmware: enable run time change of forcing fallback loader 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez ` (6 preceding siblings ...) 2018-02-24 2:46 ` [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:22 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 09/11] firmware: enable to force disable the fallback mechanism at run time Luis R. Rodriguez ` (2 subsequent siblings) 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez Currently one requires to test four kernel configurations to test the firmware API completely: 0) CONFIG_FW_LOADER=y 1) o CONFIG_FW_LOADER=y o CONFIG_FW_LOADER_USER_HELPER=y 2) o CONFIG_FW_LOADER=y o CONFIG_FW_LOADER_USER_HELPER=y o CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y 3) When CONFIG_FW_LOADER=m the built-in stuff is disabled, we have no current tests for this. We can reduce the requirements to three kernel configurations by making fw_config.force_sysfs_fallback a proc knob we flip on off. For kernels that disable CONFIG_IKCONFIG_PROC this can also enable one to inspect if CONFIG_FW_LOADER_USER_HELPER_FALLBACK was enabled at build time by checking the proc value at boot time. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/base/firmware_fallback.c | 1 + drivers/base/firmware_fallback.h | 4 +++- drivers/base/firmware_fallback_table.c | 17 +++++++++++++++++ kernel/sysctl.c | 11 +++++++++++ 4 files changed, 32 insertions(+), 1 deletion(-) diff --git a/drivers/base/firmware_fallback.c b/drivers/base/firmware_fallback.c index 47690207e0ee..cbce9a950cd8 100644 --- a/drivers/base/firmware_fallback.c +++ b/drivers/base/firmware_fallback.c @@ -7,6 +7,7 @@ #include <linux/security.h> #include <linux/highmem.h> #include <linux/umh.h> +#include <linux/sysctl.h> #include "firmware_fallback.h" #include "firmware_loader.h" diff --git a/drivers/base/firmware_fallback.h b/drivers/base/firmware_fallback.h index 550498c7fa4c..ca7e69a8417b 100644 --- a/drivers/base/firmware_fallback.h +++ b/drivers/base/firmware_fallback.h @@ -12,12 +12,14 @@ * * @force_sysfs_fallback: force the sysfs fallback mechanism to be used * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. + * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y + * functionality on a kernel where that config entry has been disabled. * @old_timeout: for internal use * @loading_timeout: the timeout to wait for the fallback mechanism before * giving up, in seconds. */ struct firmware_fallback_config { - const bool force_sysfs_fallback; + unsigned int force_sysfs_fallback; int old_timeout; int loading_timeout; }; diff --git a/drivers/base/firmware_fallback_table.c b/drivers/base/firmware_fallback_table.c index 53cc4e492520..77300d5e9c52 100644 --- a/drivers/base/firmware_fallback_table.c +++ b/drivers/base/firmware_fallback_table.c @@ -19,6 +19,9 @@ /* Module or buit-in */ #ifdef CONFIG_FW_LOADER_USER_HELPER +static unsigned int zero; +static unsigned int one = 1; + struct firmware_fallback_config fw_fallback_config = { .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), .loading_timeout = 60, @@ -26,4 +29,18 @@ struct firmware_fallback_config fw_fallback_config = { }; EXPORT_SYMBOL_GPL(fw_fallback_config); +struct ctl_table firmware_config_table[] = { + { + .procname = "force_sysfs_fallback", + .data = &fw_fallback_config.force_sysfs_fallback, + .maxlen = sizeof(unsigned int), + .mode = 0644, + .proc_handler = proc_douintvec_minmax, + .extra1 = &zero, + .extra2 = &one, + }, + { } +}; +EXPORT_SYMBOL_GPL(firmware_config_table); + #endif diff --git a/kernel/sysctl.c b/kernel/sysctl.c index acdf4e85c0a1..aa8355953fcf 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -253,6 +253,10 @@ extern struct ctl_table random_table[]; extern struct ctl_table epoll_table[]; #endif +#ifdef CONFIG_FW_LOADER_USER_HELPER +extern struct ctl_table firmware_config_table[]; +#endif + #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT int sysctl_legacy_va_layout; #endif @@ -748,6 +752,13 @@ static struct ctl_table kern_table[] = { .mode = 0555, .child = usermodehelper_table, }, +#ifdef CONFIG_FW_LOADER_USER_HELPER + { + .procname = "firmware_config", + .mode = 0555, + .child = firmware_config_table, + }, +#endif { .procname = "overflowuid", .data = &overflowuid, -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 08/11] firmware: enable run time change of forcing fallback loader 2018-02-24 2:46 ` [PATCH v2 08/11] firmware: enable run time change of forcing fallback loader Luis R. Rodriguez @ 2018-02-27 23:22 ` Kees Cook 0 siblings, 0 replies; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:22 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > Currently one requires to test four kernel configurations to test the > firmware API completely: > > 0) > CONFIG_FW_LOADER=y > > 1) > o CONFIG_FW_LOADER=y > o CONFIG_FW_LOADER_USER_HELPER=y > > 2) > o CONFIG_FW_LOADER=y > o CONFIG_FW_LOADER_USER_HELPER=y > o CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > > 3) When CONFIG_FW_LOADER=m the built-in stuff is disabled, we have > no current tests for this. > > We can reduce the requirements to three kernel configurations by making > fw_config.force_sysfs_fallback a proc knob we flip on off. For kernels that > disable CONFIG_IKCONFIG_PROC this can also enable one to inspect if > CONFIG_FW_LOADER_USER_HELPER_FALLBACK was enabled at build time by checking > the proc value at boot time. You'll still need compile tests of each configuration, but I love that this will give one kernel to test them all. Acked-by: Kees Cook <keescook@chromium.org> -Kees > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > drivers/base/firmware_fallback.c | 1 + > drivers/base/firmware_fallback.h | 4 +++- > drivers/base/firmware_fallback_table.c | 17 +++++++++++++++++ > kernel/sysctl.c | 11 +++++++++++ > 4 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/firmware_fallback.c b/drivers/base/firmware_fallback.c > index 47690207e0ee..cbce9a950cd8 100644 > --- a/drivers/base/firmware_fallback.c > +++ b/drivers/base/firmware_fallback.c > @@ -7,6 +7,7 @@ > #include <linux/security.h> > #include <linux/highmem.h> > #include <linux/umh.h> > +#include <linux/sysctl.h> > > #include "firmware_fallback.h" > #include "firmware_loader.h" > diff --git a/drivers/base/firmware_fallback.h b/drivers/base/firmware_fallback.h > index 550498c7fa4c..ca7e69a8417b 100644 > --- a/drivers/base/firmware_fallback.h > +++ b/drivers/base/firmware_fallback.h > @@ -12,12 +12,14 @@ > * > * @force_sysfs_fallback: force the sysfs fallback mechanism to be used > * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. > + * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > + * functionality on a kernel where that config entry has been disabled. > * @old_timeout: for internal use > * @loading_timeout: the timeout to wait for the fallback mechanism before > * giving up, in seconds. > */ > struct firmware_fallback_config { > - const bool force_sysfs_fallback; > + unsigned int force_sysfs_fallback; > int old_timeout; > int loading_timeout; > }; > diff --git a/drivers/base/firmware_fallback_table.c b/drivers/base/firmware_fallback_table.c > index 53cc4e492520..77300d5e9c52 100644 > --- a/drivers/base/firmware_fallback_table.c > +++ b/drivers/base/firmware_fallback_table.c > @@ -19,6 +19,9 @@ > /* Module or buit-in */ > #ifdef CONFIG_FW_LOADER_USER_HELPER > > +static unsigned int zero; > +static unsigned int one = 1; > + > struct firmware_fallback_config fw_fallback_config = { > .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK), > .loading_timeout = 60, > @@ -26,4 +29,18 @@ struct firmware_fallback_config fw_fallback_config = { > }; > EXPORT_SYMBOL_GPL(fw_fallback_config); > > +struct ctl_table firmware_config_table[] = { > + { > + .procname = "force_sysfs_fallback", > + .data = &fw_fallback_config.force_sysfs_fallback, > + .maxlen = sizeof(unsigned int), > + .mode = 0644, > + .proc_handler = proc_douintvec_minmax, > + .extra1 = &zero, > + .extra2 = &one, > + }, > + { } > +}; > +EXPORT_SYMBOL_GPL(firmware_config_table); > + > #endif > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index acdf4e85c0a1..aa8355953fcf 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -253,6 +253,10 @@ extern struct ctl_table random_table[]; > extern struct ctl_table epoll_table[]; > #endif > > +#ifdef CONFIG_FW_LOADER_USER_HELPER > +extern struct ctl_table firmware_config_table[]; > +#endif > + > #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT > int sysctl_legacy_va_layout; > #endif > @@ -748,6 +752,13 @@ static struct ctl_table kern_table[] = { > .mode = 0555, > .child = usermodehelper_table, > }, > +#ifdef CONFIG_FW_LOADER_USER_HELPER > + { > + .procname = "firmware_config", > + .mode = 0555, > + .child = firmware_config_table, > + }, > +#endif > { > .procname = "overflowuid", > .data = &overflowuid, > -- > 2.16.2 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/11] firmware: enable to force disable the fallback mechanism at run time 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez ` (7 preceding siblings ...) 2018-02-24 2:46 ` [PATCH v2 08/11] firmware: enable run time change of forcing fallback loader Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:23 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 10/11] test_firmware: add a library for shared helpers Luis R. Rodriguez 2018-02-24 2:46 ` [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob Luis R. Rodriguez 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez You currently need four different kernel builds to test the firmware API fully. By adding a proc knob to force disable the fallback mechanism completely we are able to reduce the amount of kernels you need built to test the firmware API down to two. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/base/firmware_fallback.c | 5 +++++ drivers/base/firmware_fallback.h | 4 ++++ drivers/base/firmware_fallback_table.c | 9 +++++++++ 3 files changed, 18 insertions(+) diff --git a/drivers/base/firmware_fallback.c b/drivers/base/firmware_fallback.c index cbce9a950cd8..13fa5ff2b46c 100644 --- a/drivers/base/firmware_fallback.c +++ b/drivers/base/firmware_fallback.c @@ -643,6 +643,11 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags) static bool fw_run_sysfs_fallback(unsigned int opt_flags) { + if (fw_fallback_config.ignore_sysfs_fallback) { + pr_info_once("Ignoring firmware sysfs fallback due to debugfs knob\n"); + return false; + } + if ((opt_flags & FW_OPT_NOFALLBACK)) return false; diff --git a/drivers/base/firmware_fallback.h b/drivers/base/firmware_fallback.h index ca7e69a8417b..dfebc644ed35 100644 --- a/drivers/base/firmware_fallback.h +++ b/drivers/base/firmware_fallback.h @@ -14,12 +14,16 @@ * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y * functionality on a kernel where that config entry has been disabled. + * @ignore_sysfs_fallback: force to disable the sysfs fallback mechanism. + * This emulates the behaviour as if we had set the kernel + * config CONFIG_FW_LOADER_USER_HELPER=n. * @old_timeout: for internal use * @loading_timeout: the timeout to wait for the fallback mechanism before * giving up, in seconds. */ struct firmware_fallback_config { unsigned int force_sysfs_fallback; + unsigned int ignore_sysfs_fallback; int old_timeout; int loading_timeout; }; diff --git a/drivers/base/firmware_fallback_table.c b/drivers/base/firmware_fallback_table.c index 77300d5e9c52..5e990b0330c7 100644 --- a/drivers/base/firmware_fallback_table.c +++ b/drivers/base/firmware_fallback_table.c @@ -39,6 +39,15 @@ struct ctl_table firmware_config_table[] = { .extra1 = &zero, .extra2 = &one, }, + { + .procname = "ignore_sysfs_fallback", + .data = &fw_fallback_config.ignore_sysfs_fallback, + .maxlen = sizeof(unsigned int), + .mode = 0644, + .proc_handler = proc_douintvec_minmax, + .extra1 = &zero, + .extra2 = &one, + }, { } }; EXPORT_SYMBOL_GPL(firmware_config_table); -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 09/11] firmware: enable to force disable the fallback mechanism at run time 2018-02-24 2:46 ` [PATCH v2 09/11] firmware: enable to force disable the fallback mechanism at run time Luis R. Rodriguez @ 2018-02-27 23:23 ` Kees Cook 0 siblings, 0 replies; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:23 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > You currently need four different kernel builds to test the firmware > API fully. By adding a proc knob to force disable the fallback mechanism > completely we are able to reduce the amount of kernels you need built > to test the firmware API down to two. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > drivers/base/firmware_fallback.c | 5 +++++ > drivers/base/firmware_fallback.h | 4 ++++ > drivers/base/firmware_fallback_table.c | 9 +++++++++ > 3 files changed, 18 insertions(+) > > diff --git a/drivers/base/firmware_fallback.c b/drivers/base/firmware_fallback.c > index cbce9a950cd8..13fa5ff2b46c 100644 > --- a/drivers/base/firmware_fallback.c > +++ b/drivers/base/firmware_fallback.c > @@ -643,6 +643,11 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags) > > static bool fw_run_sysfs_fallback(unsigned int opt_flags) > { > + if (fw_fallback_config.ignore_sysfs_fallback) { > + pr_info_once("Ignoring firmware sysfs fallback due to debugfs knob\n"); > + return false; > + } > + > if ((opt_flags & FW_OPT_NOFALLBACK)) > return false; > > diff --git a/drivers/base/firmware_fallback.h b/drivers/base/firmware_fallback.h > index ca7e69a8417b..dfebc644ed35 100644 > --- a/drivers/base/firmware_fallback.h > +++ b/drivers/base/firmware_fallback.h > @@ -14,12 +14,16 @@ > * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. > * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > * functionality on a kernel where that config entry has been disabled. > + * @ignore_sysfs_fallback: force to disable the sysfs fallback mechanism. > + * This emulates the behaviour as if we had set the kernel > + * config CONFIG_FW_LOADER_USER_HELPER=n. > * @old_timeout: for internal use > * @loading_timeout: the timeout to wait for the fallback mechanism before > * giving up, in seconds. > */ > struct firmware_fallback_config { > unsigned int force_sysfs_fallback; > + unsigned int ignore_sysfs_fallback; > int old_timeout; > int loading_timeout; > }; > diff --git a/drivers/base/firmware_fallback_table.c b/drivers/base/firmware_fallback_table.c > index 77300d5e9c52..5e990b0330c7 100644 > --- a/drivers/base/firmware_fallback_table.c > +++ b/drivers/base/firmware_fallback_table.c > @@ -39,6 +39,15 @@ struct ctl_table firmware_config_table[] = { > .extra1 = &zero, > .extra2 = &one, > }, > + { > + .procname = "ignore_sysfs_fallback", > + .data = &fw_fallback_config.ignore_sysfs_fallback, > + .maxlen = sizeof(unsigned int), > + .mode = 0644, > + .proc_handler = proc_douintvec_minmax, > + .extra1 = &zero, > + .extra2 = &one, > + }, > { } > }; > EXPORT_SYMBOL_GPL(firmware_config_table); > -- > 2.16.2 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 10/11] test_firmware: add a library for shared helpers 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez ` (8 preceding siblings ...) 2018-02-24 2:46 ` [PATCH v2 09/11] firmware: enable to force disable the fallback mechanism at run time Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:16 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob Luis R. Rodriguez 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez Both fw_fallback.sh and fw_filesystem.sh share a common set of boiler plate setup and tests. We can share these in a common place. While at it, move both test to use /bin/bash. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- tools/testing/selftests/firmware/fw_fallback.sh | 69 ++----------- tools/testing/selftests/firmware/fw_filesystem.sh | 61 ++--------- tools/testing/selftests/firmware/fw_lib.sh | 120 ++++++++++++++++++++++ 3 files changed, 137 insertions(+), 113 deletions(-) create mode 100755 tools/testing/selftests/firmware/fw_lib.sh diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh index 40b6c1d3e832..0db1c09a6bcc 100755 --- a/tools/testing/selftests/firmware/fw_fallback.sh +++ b/tools/testing/selftests/firmware/fw_fallback.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # SPDX-License-Identifier: GPL-2.0 # This validates that the kernel will fall back to using the fallback mechanism # to load firmware it can't find on disk itself. We must request a firmware @@ -6,68 +6,17 @@ # won't find so that we can do the load ourself manually. set -e -PROC_CONFIG="/proc/config.gz" +TEST_REQS_FW_SYSFS_FALLBACK="yes" +TEST_REQS_FW_SET_CUSTOM_PATH="no" TEST_DIR=$(dirname $0) +source $TEST_DIR/fw_lib.sh -modprobe test_firmware -if [ ! -f $PROC_CONFIG ]; then - if modprobe configs 2>/dev/null; then - echo "Loaded configs module" - if [ ! -f $PROC_CONFIG ]; then - echo "You must have the following enabled in your kernel:" >&2 - cat $TEST_DIR/config >&2 - echo "Resorting to old heuristics" >&2 - fi - else - echo "Failed to load configs module, using old heuristics" >&2 - fi -fi - -kconfig_has() -{ - if [ -f $PROC_CONFIG ]; then - if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then - echo "yes" - else - echo "no" - fi - else - # We currently don't have easy heuristics to infer this - # so best we can do is just try to use the kernel assuming - # you had enabled it. This matches the old behaviour. - if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then - echo "yes" - elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then - if [ -d /sys/class/firmware/ ]; then - echo yes - else - echo no - fi - fi - fi -} - -DIR=/sys/devices/virtual/misc/test_firmware - -HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y) -HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) - -if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then - OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) -else - echo "usermode helper disabled so ignoring test" - exit 0 -fi - -FWPATH=$(mktemp -d) -FW="$FWPATH/test-firmware.bin" +check_mods +check_setup +verify_reqs +setup_tmp_file -test_finish() -{ - echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout - rm -f "$FW" - rmdir "$FWPATH" -} +trap "test_finish" EXIT load_fw() { diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh index f9508e1a4058..7f47877fa7fa 100755 --- a/tools/testing/selftests/firmware/fw_filesystem.sh +++ b/tools/testing/selftests/firmware/fw_filesystem.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # SPDX-License-Identifier: GPL-2.0 # This validates that the kernel will load firmware out of its list of # firmware locations on disk. Since the user helper does similar work, @@ -6,52 +6,15 @@ # know so we can be sure we're not accidentally testing the user helper. set -e -DIR=/sys/devices/virtual/misc/test_firmware +TEST_REQS_FW_SYSFS_FALLBACK="no" +TEST_REQS_FW_SET_CUSTOM_PATH="yes" TEST_DIR=$(dirname $0) +source $TEST_DIR/fw_lib.sh -test_modprobe() -{ - if [ ! -d $DIR ]; then - echo "$0: $DIR not present" - echo "You must have the following enabled in your kernel:" - cat $TEST_DIR/config - exit 1 - fi -} - -trap "test_modprobe" EXIT - -if [ ! -d $DIR ]; then - modprobe test_firmware -fi - -# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/ -# These days most distros enable CONFIG_FW_LOADER_USER_HELPER but disable -# CONFIG_FW_LOADER_USER_HELPER_FALLBACK. We use /sys/class/firmware/ as an -# indicator for CONFIG_FW_LOADER_USER_HELPER. -HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi) - -if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then - OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) -fi - -OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path) - -FWPATH=$(mktemp -d) -FW="$FWPATH/test-firmware.bin" - -test_finish() -{ - if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then - echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout - fi - if [ "$OLD_FWPATH" = "" ]; then - OLD_FWPATH=" " - fi - echo -n "$OLD_FWPATH" >/sys/module/firmware_class/parameters/path - rm -f "$FW" - rmdir "$FWPATH" -} +check_mods +check_setup +verify_reqs +setup_tmp_file trap "test_finish" EXIT @@ -60,14 +23,6 @@ if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then echo 1 >/sys/class/firmware/timeout fi -# Set the kernel search path. -echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path - -# This is an unlikely real-world firmware content. :) -echo "ABCD0123" >"$FW" - -NAME=$(basename "$FW") - if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then echo "$0: empty filename should not succeed" >&2 exit 1 diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh new file mode 100755 index 000000000000..0702dbf0f06b --- /dev/null +++ b/tools/testing/selftests/firmware/fw_lib.sh @@ -0,0 +1,120 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +# Library of helpers for test scripts. +set -e + +DIR=/sys/devices/virtual/misc/test_firmware + +PROC_CONFIG="/proc/config.gz" +TEST_DIR=$(dirname $0) + +print_reqs_exit() +{ + echo "You must have the following enabled in your kernel:" >&2 + cat $TEST_DIR/config >&2 + exit 1 +} + +test_modprobe() +{ + if [ ! -d $DIR ]; then + print_reqs_exit + fi +} + +check_mods() +{ + trap "test_modprobe" EXIT + if [ ! -d $DIR ]; then + modprobe test_firmware + fi + if [ ! -f $PROC_CONFIG ]; then + if modprobe configs 2>/dev/null; then + echo "Loaded configs module" + if [ ! -f $PROC_CONFIG ]; then + echo "You must have the following enabled in your kernel:" >&2 + cat $TEST_DIR/config >&2 + echo "Resorting to old heuristics" >&2 + fi + else + echo "Failed to load configs module, using old heuristics" >&2 + fi + fi +} + +check_setup() +{ + HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y) + HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) + + if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then + OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) + fi + + OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path) +} + +verify_reqs() +{ + if [ "$TEST_REQS_FW_SYSFS_FALLBACK" = "yes" ]; then + if [ ! "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then + echo "usermode helper disabled so ignoring test" + exit 1 + fi + fi +} + +setup_tmp_file() +{ + FWPATH=$(mktemp -d) + FW="$FWPATH/test-firmware.bin" + echo "ABCD0123" >"$FW" + NAME=$(basename "$FW") + if [ "$TEST_REQS_FW_SET_CUSTOM_PATH" = "yes" ]; then + echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path + fi +} + +test_finish() +{ + if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then + echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout + fi + if [ "$OLD_FWPATH" = "" ]; then + OLD_FWPATH=" " + fi + if [ "$TEST_REQS_FW_SET_CUSTOM_PATH" = "yes" ]; then + echo -n "$OLD_FWPATH" >/sys/module/firmware_class/parameters/path + fi + if [ -f $FW ]; then + rm -f "$FW" + fi + if [ -d $FWPATH ]; then + rm -rf "$FWPATH" + fi +} + +kconfig_has() +{ + if [ -f $PROC_CONFIG ]; then + if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then + echo "yes" + else + echo "no" + fi + else + # We currently don't have easy heuristics to infer this + # so best we can do is just try to use the kernel assuming + # you had enabled it. This matches the old behaviour. + if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then + echo "yes" + elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then + if [ -d /sys/class/firmware/ ]; then + echo yes + else + echo no + fi + fi + fi +} -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 10/11] test_firmware: add a library for shared helpers 2018-02-24 2:46 ` [PATCH v2 10/11] test_firmware: add a library for shared helpers Luis R. Rodriguez @ 2018-02-27 23:16 ` Kees Cook 0 siblings, 0 replies; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:16 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > Both fw_fallback.sh and fw_filesystem.sh share a common set of > boiler plate setup and tests. We can share these in a common place. > While at it, move both test to use /bin/bash. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > tools/testing/selftests/firmware/fw_fallback.sh | 69 ++----------- > tools/testing/selftests/firmware/fw_filesystem.sh | 61 ++--------- > tools/testing/selftests/firmware/fw_lib.sh | 120 ++++++++++++++++++++++ Why not initially create this fw_lib.sh instead of moving it later? -Kees > 3 files changed, 137 insertions(+), 113 deletions(-) > create mode 100755 tools/testing/selftests/firmware/fw_lib.sh > > diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh > index 40b6c1d3e832..0db1c09a6bcc 100755 > --- a/tools/testing/selftests/firmware/fw_fallback.sh > +++ b/tools/testing/selftests/firmware/fw_fallback.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # SPDX-License-Identifier: GPL-2.0 > # This validates that the kernel will fall back to using the fallback mechanism > # to load firmware it can't find on disk itself. We must request a firmware > @@ -6,68 +6,17 @@ > # won't find so that we can do the load ourself manually. > set -e > > -PROC_CONFIG="/proc/config.gz" > +TEST_REQS_FW_SYSFS_FALLBACK="yes" > +TEST_REQS_FW_SET_CUSTOM_PATH="no" > TEST_DIR=$(dirname $0) > +source $TEST_DIR/fw_lib.sh > > -modprobe test_firmware > -if [ ! -f $PROC_CONFIG ]; then > - if modprobe configs 2>/dev/null; then > - echo "Loaded configs module" > - if [ ! -f $PROC_CONFIG ]; then > - echo "You must have the following enabled in your kernel:" >&2 > - cat $TEST_DIR/config >&2 > - echo "Resorting to old heuristics" >&2 > - fi > - else > - echo "Failed to load configs module, using old heuristics" >&2 > - fi > -fi > - > -kconfig_has() > -{ > - if [ -f $PROC_CONFIG ]; then > - if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then > - echo "yes" > - else > - echo "no" > - fi > - else > - # We currently don't have easy heuristics to infer this > - # so best we can do is just try to use the kernel assuming > - # you had enabled it. This matches the old behaviour. > - if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then > - echo "yes" > - elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then > - if [ -d /sys/class/firmware/ ]; then > - echo yes > - else > - echo no > - fi > - fi > - fi > -} > - > -DIR=/sys/devices/virtual/misc/test_firmware > - > -HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y) > -HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) > - > -if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > - OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) > -else > - echo "usermode helper disabled so ignoring test" > - exit 0 > -fi > - > -FWPATH=$(mktemp -d) > -FW="$FWPATH/test-firmware.bin" > +check_mods > +check_setup > +verify_reqs > +setup_tmp_file > > -test_finish() > -{ > - echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout > - rm -f "$FW" > - rmdir "$FWPATH" > -} > +trap "test_finish" EXIT > > load_fw() > { > diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh > index f9508e1a4058..7f47877fa7fa 100755 > --- a/tools/testing/selftests/firmware/fw_filesystem.sh > +++ b/tools/testing/selftests/firmware/fw_filesystem.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # SPDX-License-Identifier: GPL-2.0 > # This validates that the kernel will load firmware out of its list of > # firmware locations on disk. Since the user helper does similar work, > @@ -6,52 +6,15 @@ > # know so we can be sure we're not accidentally testing the user helper. > set -e > > -DIR=/sys/devices/virtual/misc/test_firmware > +TEST_REQS_FW_SYSFS_FALLBACK="no" > +TEST_REQS_FW_SET_CUSTOM_PATH="yes" > TEST_DIR=$(dirname $0) > +source $TEST_DIR/fw_lib.sh > > -test_modprobe() > -{ > - if [ ! -d $DIR ]; then > - echo "$0: $DIR not present" > - echo "You must have the following enabled in your kernel:" > - cat $TEST_DIR/config > - exit 1 > - fi > -} > - > -trap "test_modprobe" EXIT > - > -if [ ! -d $DIR ]; then > - modprobe test_firmware > -fi > - > -# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/ > -# These days most distros enable CONFIG_FW_LOADER_USER_HELPER but disable > -# CONFIG_FW_LOADER_USER_HELPER_FALLBACK. We use /sys/class/firmware/ as an > -# indicator for CONFIG_FW_LOADER_USER_HELPER. > -HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi) > - > -if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > - OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) > -fi > - > -OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path) > - > -FWPATH=$(mktemp -d) > -FW="$FWPATH/test-firmware.bin" > - > -test_finish() > -{ > - if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > - echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout > - fi > - if [ "$OLD_FWPATH" = "" ]; then > - OLD_FWPATH=" " > - fi > - echo -n "$OLD_FWPATH" >/sys/module/firmware_class/parameters/path > - rm -f "$FW" > - rmdir "$FWPATH" > -} > +check_mods > +check_setup > +verify_reqs > +setup_tmp_file > > trap "test_finish" EXIT > > @@ -60,14 +23,6 @@ if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > echo 1 >/sys/class/firmware/timeout > fi > > -# Set the kernel search path. > -echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path > - > -# This is an unlikely real-world firmware content. :) > -echo "ABCD0123" >"$FW" > - > -NAME=$(basename "$FW") > - > if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then > echo "$0: empty filename should not succeed" >&2 > exit 1 > diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh > new file mode 100755 > index 000000000000..0702dbf0f06b > --- /dev/null > +++ b/tools/testing/selftests/firmware/fw_lib.sh > @@ -0,0 +1,120 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > + > +# Library of helpers for test scripts. > +set -e > + > +DIR=/sys/devices/virtual/misc/test_firmware > + > +PROC_CONFIG="/proc/config.gz" > +TEST_DIR=$(dirname $0) > + > +print_reqs_exit() > +{ > + echo "You must have the following enabled in your kernel:" >&2 > + cat $TEST_DIR/config >&2 > + exit 1 > +} > + > +test_modprobe() > +{ > + if [ ! -d $DIR ]; then > + print_reqs_exit > + fi > +} > + > +check_mods() > +{ > + trap "test_modprobe" EXIT > + if [ ! -d $DIR ]; then > + modprobe test_firmware > + fi > + if [ ! -f $PROC_CONFIG ]; then > + if modprobe configs 2>/dev/null; then > + echo "Loaded configs module" > + if [ ! -f $PROC_CONFIG ]; then > + echo "You must have the following enabled in your kernel:" >&2 > + cat $TEST_DIR/config >&2 > + echo "Resorting to old heuristics" >&2 > + fi > + else > + echo "Failed to load configs module, using old heuristics" >&2 > + fi > + fi > +} > + > +check_setup() > +{ > + HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y) > + HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) > + > + if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > + OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) > + fi > + > + OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path) > +} > + > +verify_reqs() > +{ > + if [ "$TEST_REQS_FW_SYSFS_FALLBACK" = "yes" ]; then > + if [ ! "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > + echo "usermode helper disabled so ignoring test" > + exit 1 > + fi > + fi > +} > + > +setup_tmp_file() > +{ > + FWPATH=$(mktemp -d) > + FW="$FWPATH/test-firmware.bin" > + echo "ABCD0123" >"$FW" > + NAME=$(basename "$FW") > + if [ "$TEST_REQS_FW_SET_CUSTOM_PATH" = "yes" ]; then > + echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path > + fi > +} > + > +test_finish() > +{ > + if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > + echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout > + fi > + if [ "$OLD_FWPATH" = "" ]; then > + OLD_FWPATH=" " > + fi > + if [ "$TEST_REQS_FW_SET_CUSTOM_PATH" = "yes" ]; then > + echo -n "$OLD_FWPATH" >/sys/module/firmware_class/parameters/path > + fi > + if [ -f $FW ]; then > + rm -f "$FW" > + fi > + if [ -d $FWPATH ]; then > + rm -rf "$FWPATH" > + fi > +} > + > +kconfig_has() > +{ > + if [ -f $PROC_CONFIG ]; then > + if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then > + echo "yes" > + else > + echo "no" > + fi > + else > + # We currently don't have easy heuristics to infer this > + # so best we can do is just try to use the kernel assuming > + # you had enabled it. This matches the old behaviour. > + if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then > + echo "yes" > + elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then > + if [ -d /sys/class/firmware/ ]; then > + echo yes > + else > + echo no > + fi > + fi > + fi > +} > -- > 2.16.2 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez ` (9 preceding siblings ...) 2018-02-24 2:46 ` [PATCH v2 10/11] test_firmware: add a library for shared helpers Luis R. Rodriguez @ 2018-02-24 2:46 ` Luis R. Rodriguez 2018-02-27 23:18 ` Kees Cook 10 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-24 2:46 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez Since we now have knobs to twiddle what used to be set on kernel configurations we can build one base kernel configuration and modify behaviour to mimic such kernel configurations to test them. Provided you build a kernel with: CONFIG_TEST_FIRMWARE=y CONFIG_FW_LOADER=y CONFIG_FW_LOADER_USER_HELPER=y CONFIG_IKCONFIG=y CONFIG_IKCONFIG_PROC=y We should now be able test all possible kernel configurations when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide the built-in functionality of the built-in firmware. If you're on an old kernel and either don't have /proc/config.gz (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER we cannot run these dynamic tests, so just run both scripts just as we used to before making blunt assumptions about your setup and requirements exactly as we did before. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- tools/testing/selftests/firmware/Makefile | 2 +- tools/testing/selftests/firmware/fw_lib.sh | 53 +++++++++++++++++++ tools/testing/selftests/firmware/fw_run_tests.sh | 67 ++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 1 deletion(-) create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh diff --git a/tools/testing/selftests/firmware/Makefile b/tools/testing/selftests/firmware/Makefile index 1894d625af2d..826f38d5dd19 100644 --- a/tools/testing/selftests/firmware/Makefile +++ b/tools/testing/selftests/firmware/Makefile @@ -3,7 +3,7 @@ # No binaries, but make sure arg-less "make" doesn't trigger "run_tests" all: -TEST_PROGS := fw_filesystem.sh fw_fallback.sh +TEST_PROGS := fw_run_tests.sh include ../lib.mk diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh index 0702dbf0f06b..3362a2aac40e 100755 --- a/tools/testing/selftests/firmware/fw_lib.sh +++ b/tools/testing/selftests/firmware/fw_lib.sh @@ -47,6 +47,34 @@ check_setup() { HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y) HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) + PROC_FW_IGNORE_SYSFS_FALLBACK="N" + PROC_FW_FORCE_SYSFS_FALLBACK="N" + + if [ -z $PROC_SYS_DIR ]; then + PROC_SYS_DIR="/proc/sys/kernel" + fi + + FW_PROC="${PROC_SYS_DIR}/firmware_config" + FW_FORCE_SYSFS_FALLBACK="$FW_PROC/force_sysfs_fallback" + FW_IGNORE_SYSFS_FALLBACK="$FW_PROC/ignore_sysfs_fallback" + + if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then + PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK) + fi + + if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then + PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK) + fi + + if [ "$PROC_FW_IGNORE_SYSFS_FALLBACK" = "1" ]; then + HAS_FW_LOADER_USER_HELPER_FALLBACK="no" + HAS_FW_LOADER_USER_HELPER="no" + fi + + if [ "$PROC_FW_FORCE_SYSFS_FALLBACK" = "1" ]; then + HAS_FW_LOADER_USER_HELPER="yes" + HAS_FW_LOADER_USER_HELPER_FALLBACK="yes" + fi if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) @@ -76,6 +104,30 @@ setup_tmp_file() fi } +proc_set_force_sysfs_fallback() +{ + if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then + echo -n $1 > $FW_FORCE_SYSFS_FALLBACK + PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK) + check_setup + fi +} + +proc_set_ignore_sysfs_fallback() +{ + if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then + echo -n $1 > $FW_IGNORE_SYSFS_FALLBACK + PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK) + check_setup + fi +} + +proc_restore_defaults() +{ + proc_set_force_sysfs_fallback 0 + proc_set_ignore_sysfs_fallback 0 +} + test_finish() { if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then @@ -93,6 +145,7 @@ test_finish() if [ -d $FWPATH ]; then rm -rf "$FWPATH" fi + proc_restore_defaults } kconfig_has() diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh b/tools/testing/selftests/firmware/fw_run_tests.sh new file mode 100755 index 000000000000..a12b5809ad8b --- /dev/null +++ b/tools/testing/selftests/firmware/fw_run_tests.sh @@ -0,0 +1,67 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +# This runs all known tests across all known possible configurations we could +# emulate in one run. + +set -e + +TEST_DIR=$(dirname $0) +source $TEST_DIR/fw_lib.sh + +run_tests() +{ + $TEST_DIR/fw_filesystem.sh + $TEST_DIR/fw_fallback.sh +} + +run_test_config_0001() +{ + echo "-----------------------------------------------------" + echo "Running kernel configuration test 1 -- rare" + echo "Emulates:" + echo "CONFIG_FW_LOADER=y" + echo "CONFIG_FW_LOADER_USER_HELPER=n" + echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n" + proc_set_force_sysfs_fallback 0 + proc_set_ignore_sysfs_fallback 1 + run_tests +} + +run_test_config_0002() +{ + echo "-----------------------------------------------------" + echo "Running kernel configuration test 2 -- distro" + echo "Emulates:" + echo "CONFIG_FW_LOADER=y" + echo "CONFIG_FW_LOADER_USER_HELPER=y" + echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n" + proc_set_force_sysfs_fallback 0 + proc_set_ignore_sysfs_fallback 0 + run_tests +} + +run_test_config_0003() +{ + echo "-----------------------------------------------------" + echo "Running kernel configuration test 3 -- android" + echo "Emulates:" + echo "CONFIG_FW_LOADER=y" + echo "CONFIG_FW_LOADER_USER_HELPER=y" + echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" + proc_set_force_sysfs_fallback 1 + proc_set_ignore_sysfs_fallback 0 + run_tests +} + +check_mods +check_setup + +if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then + run_test_config_0001 + run_test_config_0002 + run_test_config_0003 +else + echo "Running basic kernel configuration, working with your config" + run_test +fi -- 2.16.2 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-02-24 2:46 ` [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob Luis R. Rodriguez @ 2018-02-27 23:18 ` Kees Cook 2018-02-28 1:32 ` Luis R. Rodriguez 0 siblings, 1 reply; 34+ messages in thread From: Kees Cook @ 2018-02-27 23:18 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > Since we now have knobs to twiddle what used to be set on kernel > configurations we can build one base kernel configuration and modify > behaviour to mimic such kernel configurations to test them. > > Provided you build a kernel with: > > CONFIG_TEST_FIRMWARE=y > CONFIG_FW_LOADER=y > CONFIG_FW_LOADER_USER_HELPER=y > CONFIG_IKCONFIG=y > CONFIG_IKCONFIG_PROC=y > > We should now be able test all possible kernel configurations > when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide > the built-in functionality of the built-in firmware. > > If you're on an old kernel and either don't have /proc/config.gz > (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER > we cannot run these dynamic tests, so just run both scripts just > as we used to before making blunt assumptions about your setup > and requirements exactly as we did before. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> Cool. Nice to have it all in one test build now. :) Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > tools/testing/selftests/firmware/Makefile | 2 +- > tools/testing/selftests/firmware/fw_lib.sh | 53 +++++++++++++++++++ > tools/testing/selftests/firmware/fw_run_tests.sh | 67 ++++++++++++++++++++++++ > 3 files changed, 121 insertions(+), 1 deletion(-) > create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh > > diff --git a/tools/testing/selftests/firmware/Makefile b/tools/testing/selftests/firmware/Makefile > index 1894d625af2d..826f38d5dd19 100644 > --- a/tools/testing/selftests/firmware/Makefile > +++ b/tools/testing/selftests/firmware/Makefile > @@ -3,7 +3,7 @@ > # No binaries, but make sure arg-less "make" doesn't trigger "run_tests" > all: > > -TEST_PROGS := fw_filesystem.sh fw_fallback.sh > +TEST_PROGS := fw_run_tests.sh > > include ../lib.mk > > diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh > index 0702dbf0f06b..3362a2aac40e 100755 > --- a/tools/testing/selftests/firmware/fw_lib.sh > +++ b/tools/testing/selftests/firmware/fw_lib.sh > @@ -47,6 +47,34 @@ check_setup() > { > HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y) > HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) > + PROC_FW_IGNORE_SYSFS_FALLBACK="N" > + PROC_FW_FORCE_SYSFS_FALLBACK="N" > + > + if [ -z $PROC_SYS_DIR ]; then > + PROC_SYS_DIR="/proc/sys/kernel" > + fi > + > + FW_PROC="${PROC_SYS_DIR}/firmware_config" > + FW_FORCE_SYSFS_FALLBACK="$FW_PROC/force_sysfs_fallback" > + FW_IGNORE_SYSFS_FALLBACK="$FW_PROC/ignore_sysfs_fallback" > + > + if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then > + PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK) > + fi > + > + if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then > + PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK) > + fi > + > + if [ "$PROC_FW_IGNORE_SYSFS_FALLBACK" = "1" ]; then > + HAS_FW_LOADER_USER_HELPER_FALLBACK="no" > + HAS_FW_LOADER_USER_HELPER="no" > + fi > + > + if [ "$PROC_FW_FORCE_SYSFS_FALLBACK" = "1" ]; then > + HAS_FW_LOADER_USER_HELPER="yes" > + HAS_FW_LOADER_USER_HELPER_FALLBACK="yes" > + fi > > if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) > @@ -76,6 +104,30 @@ setup_tmp_file() > fi > } > > +proc_set_force_sysfs_fallback() > +{ > + if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then > + echo -n $1 > $FW_FORCE_SYSFS_FALLBACK > + PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK) > + check_setup > + fi > +} > + > +proc_set_ignore_sysfs_fallback() > +{ > + if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then > + echo -n $1 > $FW_IGNORE_SYSFS_FALLBACK > + PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK) > + check_setup > + fi > +} > + > +proc_restore_defaults() > +{ > + proc_set_force_sysfs_fallback 0 > + proc_set_ignore_sysfs_fallback 0 > +} > + > test_finish() > { > if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > @@ -93,6 +145,7 @@ test_finish() > if [ -d $FWPATH ]; then > rm -rf "$FWPATH" > fi > + proc_restore_defaults > } > > kconfig_has() > diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh b/tools/testing/selftests/firmware/fw_run_tests.sh > new file mode 100755 > index 000000000000..a12b5809ad8b > --- /dev/null > +++ b/tools/testing/selftests/firmware/fw_run_tests.sh > @@ -0,0 +1,67 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0 > + > +# This runs all known tests across all known possible configurations we could > +# emulate in one run. > + > +set -e > + > +TEST_DIR=$(dirname $0) > +source $TEST_DIR/fw_lib.sh > + > +run_tests() > +{ > + $TEST_DIR/fw_filesystem.sh > + $TEST_DIR/fw_fallback.sh > +} > + > +run_test_config_0001() > +{ > + echo "-----------------------------------------------------" > + echo "Running kernel configuration test 1 -- rare" > + echo "Emulates:" > + echo "CONFIG_FW_LOADER=y" > + echo "CONFIG_FW_LOADER_USER_HELPER=n" > + echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n" > + proc_set_force_sysfs_fallback 0 > + proc_set_ignore_sysfs_fallback 1 > + run_tests > +} > + > +run_test_config_0002() > +{ > + echo "-----------------------------------------------------" > + echo "Running kernel configuration test 2 -- distro" > + echo "Emulates:" > + echo "CONFIG_FW_LOADER=y" > + echo "CONFIG_FW_LOADER_USER_HELPER=y" > + echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n" > + proc_set_force_sysfs_fallback 0 > + proc_set_ignore_sysfs_fallback 0 > + run_tests > +} > + > +run_test_config_0003() > +{ > + echo "-----------------------------------------------------" > + echo "Running kernel configuration test 3 -- android" > + echo "Emulates:" > + echo "CONFIG_FW_LOADER=y" > + echo "CONFIG_FW_LOADER_USER_HELPER=y" > + echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" > + proc_set_force_sysfs_fallback 1 > + proc_set_ignore_sysfs_fallback 0 > + run_tests > +} > + > +check_mods > +check_setup > + > +if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then > + run_test_config_0001 > + run_test_config_0002 > + run_test_config_0003 > +else > + echo "Running basic kernel configuration, working with your config" > + run_test > +fi > -- > 2.16.2 > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-02-27 23:18 ` Kees Cook @ 2018-02-28 1:32 ` Luis R. Rodriguez 2018-02-28 9:07 ` Josh Triplett 0 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-28 1:32 UTC (permalink / raw) To: Kees Cook, Josh Triplett Cc: Luis R. Rodriguez, Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Tue, Feb 27, 2018 at 03:18:15PM -0800, Kees Cook wrote: > On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > Since we now have knobs to twiddle what used to be set on kernel > > configurations we can build one base kernel configuration and modify > > behaviour to mimic such kernel configurations to test them. > > > > Provided you build a kernel with: > > > > CONFIG_TEST_FIRMWARE=y > > CONFIG_FW_LOADER=y > > CONFIG_FW_LOADER_USER_HELPER=y > > CONFIG_IKCONFIG=y > > CONFIG_IKCONFIG_PROC=y > > > > We should now be able test all possible kernel configurations > > when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide > > the built-in functionality of the built-in firmware. > > > > If you're on an old kernel and either don't have /proc/config.gz > > (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER > > we cannot run these dynamic tests, so just run both scripts just > > as we used to before making blunt assumptions about your setup > > and requirements exactly as we did before. > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > Cool. Nice to have it all in one test build now. :) Now what about we start discussing one kernel config only for the future? The impact would be the size of the fallback mechanism. That should be a bit clear in terms of size impact after this series. Wonder what Josh thinks as he help with tinyconfig. We could target v4.18 if its sensible. > Acked-by: Kees Cook <keescook@chromium.org> Luis ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-02-28 1:32 ` Luis R. Rodriguez @ 2018-02-28 9:07 ` Josh Triplett 2018-02-28 18:26 ` Luis R. Rodriguez 0 siblings, 1 reply; 34+ messages in thread From: Josh Triplett @ 2018-02-28 9:07 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Kees Cook, Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Wed, Feb 28, 2018 at 01:32:37AM +0000, Luis R. Rodriguez wrote: > On Tue, Feb 27, 2018 at 03:18:15PM -0800, Kees Cook wrote: > > On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > > Since we now have knobs to twiddle what used to be set on kernel > > > configurations we can build one base kernel configuration and modify > > > behaviour to mimic such kernel configurations to test them. > > > > > > Provided you build a kernel with: > > > > > > CONFIG_TEST_FIRMWARE=y > > > CONFIG_FW_LOADER=y > > > CONFIG_FW_LOADER_USER_HELPER=y > > > CONFIG_IKCONFIG=y > > > CONFIG_IKCONFIG_PROC=y > > > > > > We should now be able test all possible kernel configurations > > > when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide > > > the built-in functionality of the built-in firmware. > > > > > > If you're on an old kernel and either don't have /proc/config.gz > > > (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER > > > we cannot run these dynamic tests, so just run both scripts just > > > as we used to before making blunt assumptions about your setup > > > and requirements exactly as we did before. > > > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > > > Cool. Nice to have it all in one test build now. :) > > Now what about we start discussing one kernel config only for the future? The > impact would be the size of the fallback mechanism. That should be a bit clear > in terms of size impact after this series. > > Wonder what Josh thinks as he help with tinyconfig. We could target v4.18 if > its sensible. Having any of these unconditionally compiled in seems likely to be a significant impact, both directly and because of what else it would implicitly prevent compiling out or removing. And the firmware loader, for instance, is something that many kernels or hardware will not need at all. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-02-28 9:07 ` Josh Triplett @ 2018-02-28 18:26 ` Luis R. Rodriguez 2018-03-01 0:00 ` Josh Triplett 0 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-28 18:26 UTC (permalink / raw) To: Josh Triplett Cc: Luis R. Rodriguez, Kees Cook, Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Wed, Feb 28, 2018 at 01:07:23AM -0800, Josh Triplett wrote: > On Wed, Feb 28, 2018 at 01:32:37AM +0000, Luis R. Rodriguez wrote: > > On Tue, Feb 27, 2018 at 03:18:15PM -0800, Kees Cook wrote: > > > On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > > > Since we now have knobs to twiddle what used to be set on kernel > > > > configurations we can build one base kernel configuration and modify > > > > behaviour to mimic such kernel configurations to test them. > > > > > > > > Provided you build a kernel with: > > > > > > > > CONFIG_TEST_FIRMWARE=y > > > > CONFIG_FW_LOADER=y > > > > CONFIG_FW_LOADER_USER_HELPER=y > > > > CONFIG_IKCONFIG=y > > > > CONFIG_IKCONFIG_PROC=y > > > > > > > > We should now be able test all possible kernel configurations > > > > when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide > > > > the built-in functionality of the built-in firmware. > > > > > > > > If you're on an old kernel and either don't have /proc/config.gz > > > > (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER > > > > we cannot run these dynamic tests, so just run both scripts just > > > > as we used to before making blunt assumptions about your setup > > > > and requirements exactly as we did before. > > > > > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > > > > > Cool. Nice to have it all in one test build now. :) > > > > Now what about we start discussing one kernel config only for the future? The > > impact would be the size of the fallback mechanism. That should be a bit clear > > in terms of size impact after this series. > > > > Wonder what Josh thinks as he help with tinyconfig. We could target v4.18 if > > its sensible. > > Having any of these unconditionally compiled in seems likely to be a > significant impact, both directly and because of what else it would > implicitly prevent compiling out or removing. And the firmware loader, > for instance, is something that many kernels or hardware will not need > at all. Oh sorry, I did not mean always enabling the firmware loader, that would add an extra 828 bytes, and 14264 bytes if the fallback mechanism is enabled as well. I meant having only CONFIG_FW_LOADER=y, and removing CONFIG_FW_LOADER_USER_HELPER so that we just always compile it in if we have CONFIG_FW_LOADER=y, so a penalty of 13436 bytes for those who enabled the firmware loader but hadn't before enabled the fallback mechanism. I'll note CONFIG_FW_LOADER_USER_HELPER is actually known to be enabled by most distributions these days. We have an extra CONFIG_FW_LOADER_USER_HELPER_FALLBACK but this is now just a toggle of a boolean, and actually Android is known to enable it mostly, not other Linux distributions. Since Android enables CONFIG_FW_LOADER_USER_HELPER_FALLBACK we know they also enable the fallback mechanism with CONFIG_FW_LOADER_USER_HELPER_FALLBACK. So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an extra 13436 bytes broken down as follows: ------------------------------------------------------------------------------------------- allnoconfig with no firmware loader (with procfs enabled): $ size vmlinux text data bss dec hex filename 1135188 272012 1219736 2626936 281578 vmlinux $ du -b vmlinux 1745556 vmlinux ------------------------------------------------------------------------------------------- CONFIG_FW_LOADER=y $ size vmlinux text data bss dec hex filename 1137244 267984 1219716 2624944 280db0 vmlinux $ du -b vmlinux 1746384 vmlinux ------------------------------------------------------------------------------------------- CONFIG_FW_LOADER=y CONFIG_FW_LOADER_USER_HELPER=y $ size vmlinux text data bss dec hex filename 1140554 272464 1219716 2632734 282c1e vmlinux $ du -b vmlinux 1759820 vmlinux Luis ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-02-28 18:26 ` Luis R. Rodriguez @ 2018-03-01 0:00 ` Josh Triplett 2018-03-01 0:38 ` Luis R. Rodriguez 0 siblings, 1 reply; 34+ messages in thread From: Josh Triplett @ 2018-03-01 0:00 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Kees Cook, Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Wed, Feb 28, 2018 at 06:26:03PM +0000, Luis R. Rodriguez wrote: > On Wed, Feb 28, 2018 at 01:07:23AM -0800, Josh Triplett wrote: > > On Wed, Feb 28, 2018 at 01:32:37AM +0000, Luis R. Rodriguez wrote: > > > On Tue, Feb 27, 2018 at 03:18:15PM -0800, Kees Cook wrote: > > > > On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > > > > Since we now have knobs to twiddle what used to be set on kernel > > > > > configurations we can build one base kernel configuration and modify > > > > > behaviour to mimic such kernel configurations to test them. > > > > > > > > > > Provided you build a kernel with: > > > > > > > > > > CONFIG_TEST_FIRMWARE=y > > > > > CONFIG_FW_LOADER=y > > > > > CONFIG_FW_LOADER_USER_HELPER=y > > > > > CONFIG_IKCONFIG=y > > > > > CONFIG_IKCONFIG_PROC=y > > > > > > > > > > We should now be able test all possible kernel configurations > > > > > when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide > > > > > the built-in functionality of the built-in firmware. > > > > > > > > > > If you're on an old kernel and either don't have /proc/config.gz > > > > > (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER > > > > > we cannot run these dynamic tests, so just run both scripts just > > > > > as we used to before making blunt assumptions about your setup > > > > > and requirements exactly as we did before. > > > > > > > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > > > > > > > Cool. Nice to have it all in one test build now. :) > > > > > > Now what about we start discussing one kernel config only for the future? The > > > impact would be the size of the fallback mechanism. That should be a bit clear > > > in terms of size impact after this series. > > > > > > Wonder what Josh thinks as he help with tinyconfig. We could target v4.18 if > > > its sensible. > > > > Having any of these unconditionally compiled in seems likely to be a > > significant impact, both directly and because of what else it would > > implicitly prevent compiling out or removing. And the firmware loader, > > for instance, is something that many kernels or hardware will not need > > at all. > > Oh sorry, I did not mean always enabling the firmware loader, that would add > an extra 828 bytes, and 14264 bytes if the fallback mechanism is enabled as > well. > > I meant having only CONFIG_FW_LOADER=y, and removing > CONFIG_FW_LOADER_USER_HELPER so that we just always compile it in if we have > CONFIG_FW_LOADER=y, so a penalty of 13436 bytes for those who enabled the > firmware loader but hadn't before enabled the fallback mechanism. > > I'll note CONFIG_FW_LOADER_USER_HELPER is actually known to be enabled by most > distributions these days. We have an extra CONFIG_FW_LOADER_USER_HELPER_FALLBACK > but this is now just a toggle of a boolean, and actually Android is known to > enable it mostly, not other Linux distributions. Since Android enables > CONFIG_FW_LOADER_USER_HELPER_FALLBACK we know they also enable the fallback > mechanism with CONFIG_FW_LOADER_USER_HELPER_FALLBACK. > > So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an > extra 13436 bytes broken down as follows: Ah, I see. If you have CONFIG_FW_LOADER and not CONFIG_FW_LOADER_USER_HELPER, then you only have the in-kernel firmware loading mechanism? Given the *substantial* size difference between the two, it seems useful to have that option. What would it gain to combine the two? ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-03-01 0:00 ` Josh Triplett @ 2018-03-01 0:38 ` Luis R. Rodriguez 2018-03-01 2:25 ` Josh Triplett 0 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-03-01 0:38 UTC (permalink / raw) To: Josh Triplett Cc: Luis R. Rodriguez, Kees Cook, Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Wed, Feb 28, 2018 at 04:00:58PM -0800, Josh Triplett wrote: > On Wed, Feb 28, 2018 at 06:26:03PM +0000, Luis R. Rodriguez wrote: > > So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an > > extra 13436 bytes broken down as follows: > > Ah, I see. > > If you have CONFIG_FW_LOADER and not CONFIG_FW_LOADER_USER_HELPER, then > you only have the in-kernel firmware loading mechanism? Right, we don't have the old fallback mechanism (which BTW used to be the default way back in the hayday). > Given the > *substantial* size difference between the two, it seems useful to have > that option. That's what I wanted to get to, is 13436 bytes is*substantial* enough to merit a kernel configuration option? It seems like that is the case. > What would it gain to combine the two? Well Android enables CONFIG_FW_LOADER_USER_HELPER, and if they do, I was trying to think if there really was any point in having CONFIG_FW_LOADER_USER_HELPER as an option. Who would enable CONFIG_FW_LOADER but not CONFIG_FW_LOADER_USER_HELPER? You see other than CONFIG_FW_LOADER_USER_HELPER we also have CONFIG_FW_LOADER_USER_HELPER_FALLBACK and Android defaults that to y too. It used to be that CONFIG_FW_LOADER_USER_HELPER_FALLBACK was a mess to understand in code, and this series has reduced it to simple bool now. I started wondering if trimming kconfig options may be worth it. Sadly I don't think we can remove CONFIG_FW_LOADER_USER_HELPER_FALLBACK, and we'll have to just deal with it mapping to switching a sysctl option. But I figured it was a good time to also reconsider also if we even had any need for CONFIG_FW_LOADER_USER_HELPER. The less hairball of mess of kconfig options the better to test. Even though this series has reduced being able to consolidating being able to make a kernel now which lets us test all configurations in one build. Who would save some 13436 bytes in the real world? Luis ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-03-01 0:38 ` Luis R. Rodriguez @ 2018-03-01 2:25 ` Josh Triplett 2018-03-01 17:33 ` Luis R. Rodriguez 0 siblings, 1 reply; 34+ messages in thread From: Josh Triplett @ 2018-03-01 2:25 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Kees Cook, Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Thu, Mar 01, 2018 at 12:38:16AM +0000, Luis R. Rodriguez wrote: > On Wed, Feb 28, 2018 at 04:00:58PM -0800, Josh Triplett wrote: > > On Wed, Feb 28, 2018 at 06:26:03PM +0000, Luis R. Rodriguez wrote: > > > So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an > > > extra 13436 bytes broken down as follows: > > > > Ah, I see. > > > > If you have CONFIG_FW_LOADER and not CONFIG_FW_LOADER_USER_HELPER, then > > you only have the in-kernel firmware loading mechanism? > > Right, we don't have the old fallback mechanism (which BTW used to be > the default way back in the hayday). > > > Given the > > *substantial* size difference between the two, it seems useful to have > > that option. > > That's what I wanted to get to, is 13436 bytes is*substantial* enough to > merit a kernel configuration option? It seems like that is the case. By at least an order of magnitude, yes. > > What would it gain to combine the two? > > Well Android enables CONFIG_FW_LOADER_USER_HELPER, and if they do, I was trying > to think if there really was any point in having CONFIG_FW_LOADER_USER_HELPER > as an option. Who would enable CONFIG_FW_LOADER but not > CONFIG_FW_LOADER_USER_HELPER? An embedded system with a fixed set of hardware that needs exclusively a fixed set of firmware files known at system build time. > The less hairball of mess of kconfig options the better to test. Even > though this series has reduced being able to consolidating being > able to make a kernel now which lets us test all configurations in > one build. > > Who would save some 13436 bytes in the real world? *raises hand* ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-03-01 2:25 ` Josh Triplett @ 2018-03-01 17:33 ` Luis R. Rodriguez 0 siblings, 0 replies; 34+ messages in thread From: Luis R. Rodriguez @ 2018-03-01 17:33 UTC (permalink / raw) To: Josh Triplett Cc: Luis R. Rodriguez, Kees Cook, Greg KH, Andrew Morton, Shuah Khan, Martin Fuzzey, Mimi Zohar, David Howells, pali.rohar, Takashi Iwai, arend.vanspriel, Rafał Miłecki, nbroeking, Vikram Mulukutla, stephen.boyd, Mark Brown, Dmitry Torokhov, David Woodhouse, Linus Torvalds, Abhay_Salunke, bjorn.andersson, jewalt, LKML, linux-fsdevel On Wed, Feb 28, 2018 at 06:25:16PM -0800, Josh Triplett wrote: > On Thu, Mar 01, 2018 at 12:38:16AM +0000, Luis R. Rodriguez wrote: > > On Wed, Feb 28, 2018 at 04:00:58PM -0800, Josh Triplett wrote: > > > On Wed, Feb 28, 2018 at 06:26:03PM +0000, Luis R. Rodriguez wrote: > > > > So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an > > > > extra 13436 bytes broken down as follows: > > > > > > Ah, I see. > > > > > > If you have CONFIG_FW_LOADER and not CONFIG_FW_LOADER_USER_HELPER, then > > > you only have the in-kernel firmware loading mechanism? > > > > Right, we don't have the old fallback mechanism (which BTW used to be > > the default way back in the hayday). > > > > > Given the > > > *substantial* size difference between the two, it seems useful to have > > > that option. > > > > That's what I wanted to get to, is 13436 bytes *substantial* enough to > > merit a kernel configuration option? It seems like that is the case. > > By at least an order of magnitude, yes. OK, then now we have a worthy reasonable description to amend into the kconfig option too. And since its now revisited, I guess we can live with it for a good while. > > > What would it gain to combine the two? > > > > Well Android enables CONFIG_FW_LOADER_USER_HELPER, and if they do, I was trying > > to think if there really was any point in having CONFIG_FW_LOADER_USER_HELPER > > as an option. Who would enable CONFIG_FW_LOADER but not > > CONFIG_FW_LOADER_USER_HELPER? > > An embedded system with a fixed set of hardware that needs exclusively a > fixed set of firmware files known at system build time. Fair enough, this should help also in the description. > > The less hairball of mess of kconfig options the better to test. Even > > though this series has reduced being able to consolidating being > > able to make a kernel now which lets us test all configurations in > > one build. > > > > Who would save some 13436 bytes in the real world? > > *raises hand* Thanks for the feedback. Luis ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 00/11] firmware: cleanup for v4.17 @ 2018-02-14 0:41 Luis R. Rodriguez 2018-02-14 0:41 ` [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob Luis R. Rodriguez 0 siblings, 1 reply; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-14 0:41 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez Greg, Now that v4.16-rc1 is out... This is the pending cleanup I have for v4.17. Its based on my last iteration of cleanup patches I had submitted for v4.16. You took in most patches except a few patches which added some selftests bash helpers like kconfig_has() as it was observed perhaps we could share this. We poked Shuah Khan to see what she thought and her recommendation was to keep these things within the firmware helpers alone, given sharing a helper script would currently just add more complexity [0]. The biggest advantage of the missing patches was that we'd be able to test at run time all firmware kernel configs usign only one kernel build. In this series I also decided to switch from debugfs to proc sys for the knobs to help dynamically modify the firmware behaviour, this proved less code and easier to maintain. I also went ahead and finally shifted all fallback code onto its own file, a goal I've been aiming for a while now. 0-day has given me its blessings on these builds. Questions, feedback, rants all equally welcomed. [0] https://lkml.kernel.org/r/938069cf-4478-2e46-f195-386830ce1a97@kernel.org Luis R. Rodriguez (11): test_firmware: enable custom fallback testing on limited kernel configs test_firmware: replace syfs fallback check with kconfig_has helper firmware: enable to split firmware_class into separate target files firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further firmware: use helpers for setting up a temporary cache timeout firmware: move loading timeout under struct firmware_fallback_config firmware: split firmware fallback functionality into its own file firmware: enable run time change of forcing fallback loader firmware: enable to force disable the fallback mechanism at run time test_firmware: add a library for shared helpers test_firmware: test three firmware kernel configs using a proc knob drivers/base/Makefile | 2 + drivers/base/firmware_fallback.c | 720 +++++++++++++++++++ drivers/base/firmware_fallback.h | 44 ++ .../base/{firmware_class.c => firmware_loader.c} | 765 +-------------------- drivers/base/firmware_loader.h | 115 ++++ kernel/sysctl.c | 11 + tools/testing/selftests/firmware/Makefile | 2 +- tools/testing/selftests/firmware/config | 4 + tools/testing/selftests/firmware/fw_fallback.sh | 39 +- tools/testing/selftests/firmware/fw_filesystem.sh | 61 +- tools/testing/selftests/firmware/fw_lib.sh | 173 +++++ tools/testing/selftests/firmware/fw_run_tests.sh | 67 ++ 12 files changed, 1166 insertions(+), 837 deletions(-) create mode 100644 drivers/base/firmware_fallback.c create mode 100644 drivers/base/firmware_fallback.h rename drivers/base/{firmware_class.c => firmware_loader.c} (61%) create mode 100644 drivers/base/firmware_loader.h create mode 100755 tools/testing/selftests/firmware/fw_lib.sh create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh -- 2.16.1 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob 2018-02-14 0:41 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez @ 2018-02-14 0:41 ` Luis R. Rodriguez 0 siblings, 0 replies; 34+ messages in thread From: Luis R. Rodriguez @ 2018-02-14 0:41 UTC (permalink / raw) To: gregkh Cc: akpm, keescook, shuah, mfuzzey, zohar, dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking, markivx, stephen.boyd, broonie, dmitry.torokhov, dwmw2, torvalds, Abhay_Salunke, bjorn.andersson, jewalt, linux-kernel, linux-fsdevel, Luis R. Rodriguez Since we now have knobs to twiddle what used to be set on kernel configurations we can build one base kernel configuration and modify behaviour to mimic such kernel configurations to test them. Provided you build a kernel with: CONFIG_TEST_FIRMWARE=y CONFIG_FW_LOADER=y CONFIG_FW_LOADER_USER_HELPER=y CONFIG_IKCONFIG=y CONFIG_IKCONFIG_PROC=y We should now be able test all possible kernel configurations when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide the built-in functionality of the built-in firmware. If you're on an old kernel and either don't have /proc/config.gz (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER we cannot run these dynamic tests, so just run both scripts just as we used to before making blunt assumptions about your setup and requirements exactly as we did before. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- tools/testing/selftests/firmware/Makefile | 2 +- tools/testing/selftests/firmware/fw_lib.sh | 53 +++++++++++++++++++ tools/testing/selftests/firmware/fw_run_tests.sh | 67 ++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 1 deletion(-) create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh diff --git a/tools/testing/selftests/firmware/Makefile b/tools/testing/selftests/firmware/Makefile index 1894d625af2d..826f38d5dd19 100644 --- a/tools/testing/selftests/firmware/Makefile +++ b/tools/testing/selftests/firmware/Makefile @@ -3,7 +3,7 @@ # No binaries, but make sure arg-less "make" doesn't trigger "run_tests" all: -TEST_PROGS := fw_filesystem.sh fw_fallback.sh +TEST_PROGS := fw_run_tests.sh include ../lib.mk diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh index 0702dbf0f06b..3362a2aac40e 100755 --- a/tools/testing/selftests/firmware/fw_lib.sh +++ b/tools/testing/selftests/firmware/fw_lib.sh @@ -47,6 +47,34 @@ check_setup() { HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y) HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y) + PROC_FW_IGNORE_SYSFS_FALLBACK="N" + PROC_FW_FORCE_SYSFS_FALLBACK="N" + + if [ -z $PROC_SYS_DIR ]; then + PROC_SYS_DIR="/proc/sys/kernel" + fi + + FW_PROC="${PROC_SYS_DIR}/firmware_config" + FW_FORCE_SYSFS_FALLBACK="$FW_PROC/force_sysfs_fallback" + FW_IGNORE_SYSFS_FALLBACK="$FW_PROC/ignore_sysfs_fallback" + + if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then + PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK) + fi + + if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then + PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK) + fi + + if [ "$PROC_FW_IGNORE_SYSFS_FALLBACK" = "1" ]; then + HAS_FW_LOADER_USER_HELPER_FALLBACK="no" + HAS_FW_LOADER_USER_HELPER="no" + fi + + if [ "$PROC_FW_FORCE_SYSFS_FALLBACK" = "1" ]; then + HAS_FW_LOADER_USER_HELPER="yes" + HAS_FW_LOADER_USER_HELPER_FALLBACK="yes" + fi if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then OLD_TIMEOUT=$(cat /sys/class/firmware/timeout) @@ -76,6 +104,30 @@ setup_tmp_file() fi } +proc_set_force_sysfs_fallback() +{ + if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then + echo -n $1 > $FW_FORCE_SYSFS_FALLBACK + PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK) + check_setup + fi +} + +proc_set_ignore_sysfs_fallback() +{ + if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then + echo -n $1 > $FW_IGNORE_SYSFS_FALLBACK + PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK) + check_setup + fi +} + +proc_restore_defaults() +{ + proc_set_force_sysfs_fallback 0 + proc_set_ignore_sysfs_fallback 0 +} + test_finish() { if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then @@ -93,6 +145,7 @@ test_finish() if [ -d $FWPATH ]; then rm -rf "$FWPATH" fi + proc_restore_defaults } kconfig_has() diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh b/tools/testing/selftests/firmware/fw_run_tests.sh new file mode 100755 index 000000000000..a12b5809ad8b --- /dev/null +++ b/tools/testing/selftests/firmware/fw_run_tests.sh @@ -0,0 +1,67 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +# This runs all known tests across all known possible configurations we could +# emulate in one run. + +set -e + +TEST_DIR=$(dirname $0) +source $TEST_DIR/fw_lib.sh + +run_tests() +{ + $TEST_DIR/fw_filesystem.sh + $TEST_DIR/fw_fallback.sh +} + +run_test_config_0001() +{ + echo "-----------------------------------------------------" + echo "Running kernel configuration test 1 -- rare" + echo "Emulates:" + echo "CONFIG_FW_LOADER=y" + echo "CONFIG_FW_LOADER_USER_HELPER=n" + echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n" + proc_set_force_sysfs_fallback 0 + proc_set_ignore_sysfs_fallback 1 + run_tests +} + +run_test_config_0002() +{ + echo "-----------------------------------------------------" + echo "Running kernel configuration test 2 -- distro" + echo "Emulates:" + echo "CONFIG_FW_LOADER=y" + echo "CONFIG_FW_LOADER_USER_HELPER=y" + echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n" + proc_set_force_sysfs_fallback 0 + proc_set_ignore_sysfs_fallback 0 + run_tests +} + +run_test_config_0003() +{ + echo "-----------------------------------------------------" + echo "Running kernel configuration test 3 -- android" + echo "Emulates:" + echo "CONFIG_FW_LOADER=y" + echo "CONFIG_FW_LOADER_USER_HELPER=y" + echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" + proc_set_force_sysfs_fallback 1 + proc_set_ignore_sysfs_fallback 0 + run_tests +} + +check_mods +check_setup + +if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then + run_test_config_0001 + run_test_config_0002 + run_test_config_0003 +else + echo "Running basic kernel configuration, working with your config" + run_test +fi -- 2.16.1 ^ permalink raw reply related [flat|nested] 34+ messages in thread
end of thread, other threads:[~2018-03-08 3:44 UTC | newest] Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-24 2:46 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez 2018-02-24 2:46 ` [PATCH v2 01/11] test_firmware: enable custom fallback testing on limited kernel configs Luis R. Rodriguez 2018-02-27 23:07 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 02/11] test_firmware: replace syfs fallback check with kconfig_has helper Luis R. Rodriguez 2018-02-27 23:09 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 03/11] firmware: enable to split firmware_class into separate target files Luis R. Rodriguez 2018-02-24 2:46 ` [PATCH v2 04/11] firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further Luis R. Rodriguez 2018-02-27 23:20 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 05/11] firmware: use helpers for setting up a temporary cache timeout Luis R. Rodriguez 2018-02-27 23:20 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 06/11] firmware: move loading timeout under struct firmware_fallback_config Luis R. Rodriguez 2018-02-27 23:21 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 07/11] firmware: split firmware fallback functionality into its own file Luis R. Rodriguez 2018-02-27 23:14 ` Kees Cook 2018-02-28 1:28 ` Luis R. Rodriguez 2018-02-28 5:33 ` Kees Cook 2018-02-28 7:11 ` Greg KH 2018-03-08 3:44 ` Luis R. Rodriguez 2018-02-24 2:46 ` [PATCH v2 08/11] firmware: enable run time change of forcing fallback loader Luis R. Rodriguez 2018-02-27 23:22 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 09/11] firmware: enable to force disable the fallback mechanism at run time Luis R. Rodriguez 2018-02-27 23:23 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 10/11] test_firmware: add a library for shared helpers Luis R. Rodriguez 2018-02-27 23:16 ` Kees Cook 2018-02-24 2:46 ` [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob Luis R. Rodriguez 2018-02-27 23:18 ` Kees Cook 2018-02-28 1:32 ` Luis R. Rodriguez 2018-02-28 9:07 ` Josh Triplett 2018-02-28 18:26 ` Luis R. Rodriguez 2018-03-01 0:00 ` Josh Triplett 2018-03-01 0:38 ` Luis R. Rodriguez 2018-03-01 2:25 ` Josh Triplett 2018-03-01 17:33 ` Luis R. Rodriguez -- strict thread matches above, loose matches on Subject: below -- 2018-02-14 0:41 [PATCH v2 00/11] firmware: cleanup for v4.17 Luis R. Rodriguez 2018-02-14 0:41 ` [PATCH v2 11/11] test_firmware: test three firmware kernel configs using a proc knob Luis R. Rodriguez
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.