[PATCH 4/5] umh: fix processed error when UMH_WAIT_PROC is used
From: Luis Chamberlain When UMH_WAIT_PROC is used we call kernel_wait4(). This is the *only* place in the kernel where we actually inspect the error code. Prior to this patch we returned the value from the wait call, and that technically requires us to use wrappers such as WEXITSTATUS(). We either fix all callers to start using WEXITSTATUS() and friends *or* we do address this within the umh code and let the callers get the actual error code. The way we use kernel_wait4() on the umh is with the options set to 0, and when this is done the wait call only waits for terminated children. Because of this, there is no point to complicate checks for the umh with W*() calls. That would make the checks complex, redundant, and simply not needed. By making the umh do the checks for us we keep users kernel_wait4() at bay, and promote avoiding introduction of further W*() macros and the complexities this can bring. There were only a few callers which properly checked for the error status using open-coded solutions. We remove them as they are no longer needed, and also remove open coded implicit uses of W*() uses which should never trigger given that the options passed to wait is 0. The only helpers we really need are for termination, so we just include those, and we prefix our W*() helpers with K. Since all this does is *correct* an error code, if one was found, this change only fixes reporting the *correct* error, and there are two places where this matters, and which this patch fixes: * request_module() used to fail with an error code of 256 when a module was not found. Now it properly returns 1. * fs/nfsd/nfs4recover.c: we never were disabling the upcall as the error code of -ENOENT or -EACCES was *never* properly checked for. Reported-by: Tiezhu Yang Signed-off-by: Luis Chamberlain --- drivers/block/drbd/drbd_nl.c | 20 fs/nfsd/nfs4recover.c| 2 +- include/linux/sched/task.h | 13 + kernel/umh.c | 4 ++-- net/bridge/br_stp_if.c | 10 ++ security/keys/request_key.c | 2 +- 6 files changed, 27 insertions(+), 24 deletions(-) diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c index da4a3ebe04ef..aee272e620b9 100644 --- a/drivers/block/drbd/drbd_nl.c +++ b/drivers/block/drbd/drbd_nl.c @@ -382,13 +382,11 @@ int drbd_khelper(struct drbd_device *device, char *cmd) notify_helper(NOTIFY_CALL, device, connection, cmd, 0); ret = call_usermodehelper(drbd_usermode_helper, argv, envp, UMH_WAIT_PROC); if (ret) - drbd_warn(device, "helper command: %s %s %s exit code %u (0x%x)\n", - drbd_usermode_helper, cmd, mb, - (ret >> 8) & 0xff, ret); + drbd_warn(device, "helper command: %s %s %s failed with exit code %u (0x%x)\n", + drbd_usermode_helper, cmd, mb, ret, ret); else - drbd_info(device, "helper command: %s %s %s exit code %u (0x%x)\n", - drbd_usermode_helper, cmd, mb, - (ret >> 8) & 0xff, ret); + drbd_info(device, "helper command: %s %s %s completed successfully\n", + drbd_usermode_helper, cmd, mb); sib.sib_reason = SIB_HELPER_POST; sib.helper_exit_code = ret; drbd_bcast_event(device, &sib); @@ -424,13 +422,11 @@ enum drbd_peer_state conn_khelper(struct drbd_connection *connection, char *cmd) ret = call_usermodehelper(drbd_usermode_helper, argv, envp, UMH_WAIT_PROC); if (ret) - drbd_warn(connection, "helper command: %s %s %s exit code %u (0x%x)\n", - drbd_usermode_helper, cmd, resource_name, - (ret >> 8) & 0xff, ret); + drbd_warn(connection, "helper command: %s %s %s failed with exit code %u (0x%x)\n", + drbd_usermode_helper, cmd, resource_name, ret, ret); else - drbd_info(connection, "helper command: %s %s %s exit code %u (0x%x)\n", - drbd_usermode_helper, cmd, resource_name, - (ret >> 8) & 0xff, ret); + drbd_info(connection, "helper command: %s %s %s completed successfully\n", + drbd_usermode_helper, cmd, resource_name); /* TODO: conn_bcast_event() ?? */ notify_helper(NOTIFY_RESPONSE, NULL, connection, cmd, ret); diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c index 9e40dfecf1b1..33e6a7fd7961 100644 --- a/fs/nfsd/nfs4recover.c +++ b/fs/nfsd/nfs4recover.c @@ -1820,7 +1820,7 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1) ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC); /* -* Disable the upcall mechanism if we're getting an ENOENT or EACCES +* Disable the upcall mechanism if we're
[PATCH 2/5] kmod: Remove redundant "be an" in the comment
From: Tiezhu Yang There exists redundant "be an" in the comment, remove it. Acked-by: Luis Chamberlain Signed-off-by: Tiezhu Yang Signed-off-by: Luis Chamberlain --- kernel/kmod.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/kernel/kmod.c b/kernel/kmod.c index 37c3c4b97b8e..3cd075ce2a1e 100644 --- a/kernel/kmod.c +++ b/kernel/kmod.c @@ -36,9 +36,8 @@ * * If you need less than 50 threads would mean we're dealing with systems * smaller than 3200 pages. This assumes you are capable of having ~13M memory, - * and this would only be an be an upper limit, after which the OOM killer - * would take effect. Systems like these are very unlikely if modules are - * enabled. + * and this would only be an upper limit, after which the OOM killer would take + * effect. Systems like these are very unlikely if modules are enabled. */ #define MAX_KMOD_CONCURRENT 50 static atomic_t kmod_concurrent_max = ATOMIC_INIT(MAX_KMOD_CONCURRENT); -- 2.26.2
[PATCH 3/5] test_kmod: Avoid potential double free in trigger_config_run_type()
From: Tiezhu Yang
Reset the member "test_fs" of the test configuration after a call
of the function "kfree_const" to a null pointer so that a double
memory release will not be performed.
Fixes: d9c6a72d6fa2 ("kmod: add test driver to stress test the module loader")
Acked-by: Luis Chamberlain
Signed-off-by: Tiezhu Yang
Signed-off-by: Luis Chamberlain
---
lib/test_kmod.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index e651c37d56db..eab52770070d 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -745,7 +745,7 @@ static int trigger_config_run_type(struct kmod_test_device
*test_dev,
break;
case TEST_KMOD_FS_TYPE:
kfree_const(config->test_fs);
- config->test_driver = NULL;
+ config->test_fs = NULL;
copied = config_copy_test_fs(config, test_str,
strlen(test_str));
break;
--
2.26.2
[PATCH 0/5] kmod/umh: a few fixes
From: Luis Chamberlain Tiezhu Yang had sent out a patch set with a slew of kmod selftest fixes, and one patch which modified kmod to return 254 when a module was not found. This opened up pandora's box about why that was being used for and low and behold its because when UMH_WAIT_PROC is used we call a kernel_wait4() call but have never unwrapped the error code. The commit log for that fix details the rationale for the approach taken. I'd appreciate some review on that, in particular nfs folks as it seems a case was never really hit before. This goes boot tested, selftested with kmod, and 0-day gives its build blessings. Luis Chamberlain (2): umh: fix processed error when UMH_WAIT_PROC is used selftests: simplify kmod failure value Tiezhu Yang (3): selftests: kmod: Use variable NAME in kmod_test_0001() kmod: Remove redundant "be an" in the comment test_kmod: Avoid potential double free in trigger_config_run_type() drivers/block/drbd/drbd_nl.c | 20 +-- fs/nfsd/nfs4recover.c| 2 +- include/linux/sched/task.h | 13 kernel/kmod.c| 5 ++- kernel/umh.c | 4 +-- lib/test_kmod.c | 2 +- net/bridge/br_stp_if.c | 10 ++ security/keys/request_key.c | 2 +- tools/testing/selftests/kmod/kmod.sh | 50 +++- 9 files changed, 71 insertions(+), 37 deletions(-) -- 2.26.2
[PATCH 5/5] selftests: simplify kmod failure value
From: Luis Chamberlain
The "odd" 256 value was just an issue with the umh never
wrapping it around with WEXITSTATUS() for us. Now that it
does that, we can use a sane value / name for the selftest,
and this is no longer a oddity.
We add a way to detect this for older kernels, and support
the old return value for kernel code where it was given.
This never affected userspace.
Reported-by: Tiezhu Yang
Signed-off-by: Luis Chamberlain
---
tools/testing/selftests/kmod/kmod.sh | 46 +++-
1 file changed, 39 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/kmod/kmod.sh
b/tools/testing/selftests/kmod/kmod.sh
index da60c3bd4f23..df7b21d8561c 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -64,6 +64,8 @@ ALL_TESTS="$ALL_TESTS 0009:150:1"
ALL_TESTS="$ALL_TESTS 0010:1:1"
ALL_TESTS="$ALL_TESTS 0011:1:1"
+MODULE_NOT_FOUND="FAILURE"
+
# Kselftest framework requirement - SKIP code is 4.
ksft_skip=4
@@ -155,14 +157,19 @@ test_finish()
echo "Test completed"
}
+# OLD_FAILURE is just because the old kernel umh never wrapped
+# the error with WEXITSTATUS(). Now that it does it, we get the
+# appropriate actual value from userspace observed in-kernel.
+
+# We keep the old mapping to ensure this script keeps working
+# with older kernels.
errno_name_to_val()
{
case "$1" in
- # kmod calls modprobe and upon of a module not found
- # modprobe returns just 1... However in the kernel we
- # *sometimes* see 256...
- MODULE_NOT_FOUND)
+ OLD_FAILURE)
echo 256;;
+ FAILURE)
+ echo 1;;
SUCCESS)
echo 0;;
-EPERM)
@@ -181,7 +188,9 @@ errno_name_to_val()
errno_val_to_name()
case "$1" in
256)
- echo MODULE_NOT_FOUND;;
+ echo OLD_FAILURE;;
+ 1)
+ echo FAILURE;;
0)
echo SUCCESS;;
-1)
@@ -335,6 +344,28 @@ kmod_defaults_fs()
config_set_test_case_fs
}
+check_umh()
+{
+ NAME=''
+
+ kmod_defaults_driver
+ config_num_threads 1
+ printf '\0' >"$DIR"/config_test_driver
+ config_trigger ${FUNCNAME[0]}
+ RC=$(config_get_test_result)
+ if [[ "$RC" == "256" ]]; then
+ MODULE_NOT_FOUND="OLD_FAILURE"
+ echo "check_umh: you have and old umh which didn't wrap errors"
+ echo " with WEXITSTATUS(). This is OK!"
+ elif [[ "$RC" != "1" ]]; then
+ echo "check_umh: Unexpected return value with no modprobe
argument: $RC"
+ exit
+ else
+ echo "check_umh: You have a new umh which wraps erros with"
+ echo " WEXITSTATUS(). This is OK!"
+ fi
+}
+
kmod_test_0001_driver()
{
NAME='\000'
@@ -343,7 +374,7 @@ kmod_test_0001_driver()
config_num_threads 1
printf $NAME >"$DIR"/config_test_driver
config_trigger ${FUNCNAME[0]}
- config_expect_result ${FUNCNAME[0]} MODULE_NOT_FOUND
+ config_expect_result ${FUNCNAME[0]} $MODULE_NOT_FOUND
}
kmod_test_0001_fs()
@@ -371,7 +402,7 @@ kmod_test_0002_driver()
config_set_driver $NAME
config_num_threads 1
config_trigger ${FUNCNAME[0]}
- config_expect_result ${FUNCNAME[0]} MODULE_NOT_FOUND
+ config_expect_result ${FUNCNAME[0]} $MODULE_NOT_FOUND
}
kmod_test_0002_fs()
@@ -648,6 +679,7 @@ load_req_mod
MODPROBE=$(
[PATCH 1/5] selftests: kmod: Use variable NAME in kmod_test_0001()
From: Tiezhu Yang
Use the variable NAME instead of "\000" directly in kmod_test_0001().
Acked-by: Luis Chamberlain
Signed-off-by: Tiezhu Yang
Signed-off-by: Luis Chamberlain
---
tools/testing/selftests/kmod/kmod.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kmod/kmod.sh
b/tools/testing/selftests/kmod/kmod.sh
index 3702dbcc90a7..da60c3bd4f23 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -341,7 +341,7 @@ kmod_test_0001_driver()
kmod_defaults_driver
config_num_threads 1
- printf '\000' >"$DIR"/config_test_driver
+ printf $NAME >"$DIR"/config_test_driver
config_trigger ${FUNCNAME[0]}
config_expect_result ${FUNCNAME[0]} MODULE_NOT_FOUND
}
@@ -352,7 +352,7 @@ kmod_test_0001_fs()
kmod_defaults_fs
config_num_threads 1
- printf '\000' >"$DIR"/config_test_fs
+ printf $NAME >"$DIR"/config_test_fs
config_trigger ${FUNCNAME[0]}
config_expect_result ${FUNCNAME[0]} -EINVAL
}
--
2.26.2
[PATCH 2/3] Revert "selftests: firmware: remove use of non-standard diff -Z option"
From: Luis Chamberlain
This reverts commit f70b472e937bb659a7b7a14e64f07308e230888c.
This breaks testing on Debian, and this patch was NACKed anyway.
The proper way to address this is a quirk for busybox as that is
where the issue is present.
Signed-off-by: Luis Chamberlain
---
tools/testing/selftests/firmware/fw_filesystem.sh | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh
b/tools/testing/selftests/firmware/fw_filesystem.sh
index 466cf2f91ba0..a4320c4b44dc 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -155,8 +155,11 @@ read_firmwares()
{
for i in $(seq 0 3); do
config_set_read_fw_idx $i
- # Verify the contents match
- if ! diff -q "$FW" $DIR/read_firmware 2>/dev/null ; then
+ # Verify the contents are what we expect.
+ # -Z required for now -- check for yourself, md5sum
+ # on $FW and DIR/read_firmware will yield the same. Even
+ # cmp agrees, so something is off.
+ if ! diff -q -Z "$FW" $DIR/read_firmware 2>/dev/null ; then
echo "request #$i: firmware was not loaded" >&2
exit 1
fi
@@ -168,7 +171,7 @@ read_firmwares_expect_nofile()
for i in $(seq 0 3); do
config_set_read_fw_idx $i
# Ensures contents differ
- if diff -q "$FW" $DIR/read_firmware 2>/dev/null ; then
+ if diff -q -Z "$FW" $DIR/read_firmware 2>/dev/null ; then
echo "request $i: file was not expected to match" >&2
exit 1
fi
--
2.18.0
[PATCH 1/3] Revert "selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config"
From: Luis Chamberlain This reverts commit 7492902e8d22b568463897fa967c0886764cf034. The commit tried to address an issue discovered by Dan where he got a message saying: 'usermode helper disabled so ignoring test'. Dans's commit is forcing CONFIG_FW_LOADER_USER_HELPER_FALLBACK but just having CONFIG_FW_LOADER_USER_HELPER suffices to emulate the_FALLBACK functionality. Dan's commit is trying to fix an issue which is hidden from a previous commit. That issue will be addressed properly next. Signed-off-by: Luis Chamberlain --- tools/testing/selftests/firmware/config | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config index 913a25a4a32b..bf634dda0720 100644 --- a/tools/testing/selftests/firmware/config +++ b/tools/testing/selftests/firmware/config @@ -1,6 +1,5 @@ CONFIG_TEST_FIRMWARE=y CONFIG_FW_LOADER=y CONFIG_FW_LOADER_USER_HELPER=y -CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y CONFIG_IKCONFIG=y CONFIG_IKCONFIG_PROC=y -- 2.18.0
[PATCH 3/3] selftests: firmware: fix verify_reqs() return value
From: Luis Chamberlain
commit a6a9be9270c87 ("selftests: firmware: return Kselftest Skip code
for skipped tests") by Shuah modified failures to return the special
error code of $ksft_skip (4). We have a corner case issue where we
*do* want to verify_reqs().
Cc: # >= 4.18
Fixes: a6a9be9270c87 ("selftests: firmware: return Kselftest Skip code for for
skipped tests")
Signed-off-by: Luis Chamberlain
---
tools/testing/selftests/firmware/fw_lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/firmware/fw_lib.sh
b/tools/testing/selftests/firmware/fw_lib.sh
index 6c5f1b2ffb74..1cbb12e284a6 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -91,7 +91,7 @@ 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 $ksft_skip
+ exit 0
fi
fi
}
--
2.18.0
[PATCH 0/3] firmware_loader: few selftest fixes
From: Luis Chamberlain Greg, I've found that Dan's patches really broke firmware testing. I've identified a proper fix for the issue he found, its the last patch. This series reverts his two patches which break testing, and fixes the issue he was running into. I leave it to him as exercise to add a busybox bash quirk for the bash issue he found with diff. His patches are merged on v5.0-rc1 as such these should go to Linus tree as well. They are regressions on v5.0-rc1. Please let me know if there are any questions. Luis Luis Chamberlain (3): Revert "selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config" Revert "selftests: firmware: remove use of non-standard diff -Z option" selftests: firmware: fix verify_reqs() return value tools/testing/selftests/firmware/config | 1 - tools/testing/selftests/firmware/fw_filesystem.sh | 9 ++--- tools/testing/selftests/firmware/fw_lib.sh| 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) -- 2.18.0
[PATCH] MAINTAINERS: update name change for myself
From: Luis Chamberlain My name has changed, works better than Global Entry I tell ya. Signed-off-by: Luis Chamberlain --- Andrew, figured this could go best through your tree. Please let me know if you have a preference for it routed elsewhere. PGP key updated as well, kept the same key. Happy Turkey Massacre Eve day. MAINTAINERS | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 84c3f51b6a71..8d52af40fa13 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2490,7 +2490,7 @@ F:drivers/net/wireless/ath/* ATHEROS ATH5K WIRELESS DRIVER M: Jiri Slaby M: Nick Kossifidis -M: "Luis R. Rodriguez" +M: Luis Chamberlain L: linux-wirel...@vger.kernel.org W: http://wireless.kernel.org/en/users/Drivers/ath5k S: Maintained @@ -5784,7 +5784,7 @@ F:include/uapi/linux/firewire*.h F: tools/firewire/ FIRMWARE LOADER (request_firmware) -M: Luis R. Rodriguez +M: Luis Chamberlain L: linux-kernel@vger.kernel.org S: Maintained F: Documentation/firmware_class/ @@ -8099,7 +8099,7 @@ F:tools/testing/selftests/ F: Documentation/dev-tools/kselftest* KERNEL USERMODE HELPER -M: "Luis R. Rodriguez" +M: Luis Chamberlain L: linux-kernel@vger.kernel.org S: Maintained F: kernel/umh.c @@ -8275,7 +8275,7 @@ F:mm/kmemleak.c F: mm/kmemleak-test.c KMOD KERNEL MODULE LOADER - USERMODE HELPER -M: "Luis R. Rodriguez" +M: Luis Chamberlain L: linux-kernel@vger.kernel.org S: Maintained F: kernel/kmod.c @@ -12033,7 +12033,7 @@ F: kernel/printk/ F: include/linux/printk.h PRISM54 WIRELESS DRIVER -M: "Luis R. Rodriguez" +M: Luis Chamberlain L: linux-wirel...@vger.kernel.org W: http://wireless.kernel.org/en/users/Drivers/p54 S: Obsolete @@ -12049,7 +12049,7 @@ F: include/linux/proc_fs.h F: tools/testing/selftests/proc/ PROC SYSCTL -M: "Luis R. Rodriguez" +M: Luis Chamberlain M: Kees Cook L: linux-kernel@vger.kernel.org L: linux-fsde...@vger.kernel.org -- 2.18.0
Re: [PATCH 11/11] proc/sched: remove unused sched_time_avg_ms
On Thu, Jun 28, 2018 at 05:45:14PM +0200, Vincent Guittot wrote: > /proc/sys/kernel/sched_time_avg_ms entry is not used anywhere. > Remove it > > Cc: Ingo Molnar > Cc: Peter Zijlstra > Cc: Kees Cook > Cc: "Luis R. Rodriguez" > Signed-off-by: Vincent Guittot Reviewed-by: Luis R. Rodriguez Luis
Re: [PATCH 0/2] Avoid firmware warning in imx-sdma
On Fri, Jun 22, 2018 at 04:49:49PM +0200, Sebastian Reichel wrote: > Subject: Avoid firmware warning in imx-sdma > > Hi, > > I grabbed the first patch from patchwork from an 2017 patch series. As far as > I > could see, their usecase vanished due to switching to sync FW API (that > already > supports NOWARN). This series fixes a kernel warning in imx-sdma driver, which > works fine with ROM firmware (and already prints an info message about ROM > firmware being used due to missing firmware file). This has been tested on > arch/arm/boot/dts/imx53-ppd.dts. Please read these threads about the state of affairs with respect to data driven Vs functional API evolution on the firmware API [0] and my latests recommendation for what to do for the async firmware API [1]. Then, recently I've come to realize that perhaps the best thing actually is to *break* the cycle for the async API and make it more functional driven. For instance the call to not use the uevent should be made a separate call as only two drivers use it: o CONFIG_LEDS_LP55XX_COMMON o CONFIG_DELL_RBU Using a struct would make it data driven. A flags approach would make it more flexible and I although I think this is reasonable, if we wanted to match the sync API, we'd have one call per variation. It is therefore subjective whether or not to keep a flags based mechanism for the async API or not. If we at least use flags, we'd just break away from the sync approach. I'll let Kees and Greg pick and choose how they would prefer to see this broken down now as I am off on vacation today and won't be back until the middle of next month. [0] https://lkml.kernel.org/r/20180421173650.gw14...@wotan.suse.de [1] https://lkml.kernel.org/r/20180422202609.gx14...@wotan.suse.de Luis
Re: bpf-next boot error: KASAN: use-after-free Write in call_usermodehelper_exec_work
On Thu, Jun 07, 2018 at 09:07:01AM -0700, Alexei Starovoitov wrote: > On Thu, Jun 07, 2018 at 02:19:16PM +0200, Dmitry Vyukov wrote: > > On Mon, Jun 4, 2018 at 10:21 PM, syzbot > > wrote: > > > Hello, > > > > > > syzbot found the following crash on: > > > > > > HEAD commit:69b450789136 Merge branch 'misc-BPF-improvements' > > > git tree: bpf-next > > > console output: https://syzkaller.appspot.com/x/log.txt?x=1080d1d780 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=e4078980b886800c > > > dashboard link: > > > https://syzkaller.appspot.com/bug?extid=2c73319c406f1987d156 > > > compiler: gcc (GCC) 8.0.1 20180413 (experimental) > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > Reported-by: syzbot+2c73319c406f1987d...@syzkaller.appspotmail.com > > > > > > This crash now happens on every other boot of mainline tree. This > > prevents syzbot testing of new code, and just boots machine with > > corrupted memory. Were there any recent changes in umh? +Alexei, you > > seem to touch it last. Could your change cause this? > > looking into it. I think I see the issue. Trying to reproduce. And this is why a test driver would be useful ;) Luis
Re: [PATCH v6 2/5] efi: Add embedded peripheral firmware support
On Wed, Jun 06, 2018 at 08:39:15PM +0200, Hans de Goede wrote: > On 05-06-18 23:07, Luis R. Rodriguez wrote: > > > +To make request_firmware() fallback to trying EFI embedded firmwares > > > after this, > > > +the driver must set a boolean "efi-embedded-firmware" device-property on > > > the > > > +device before passing it to request_firmware(). > > > > No, as I have requested before I don't want this, it is silly to have such > > functionality always be considered as a fallback if we only have 2 drivers > > which need this. > Please add a call which only if used would then > > *evaluate* > > such fallback prospect. > > Ok, so I've a few questions about this: > > 1) You want me to add a: > > int firmware_request_with_system_firmware_fallback(struct device *device, > const char *name); > > function? The idea is correct, the name however is obviously terrible. This is functionality that is very specialized and only *two* device drivers need it that we are aware of which would be upstream. Experience has shown fallback mechanisms *can* be a pain, and if we add this we will be supporting this for *life*, as such I'd very much prefer to: a) *Clearly* reduce the scope of functionality clearly *beyond* what you have done. b) Have access to one simple call which folks can use to *clearly* and quickly grep for those oddball drivers using this new interface. We can do this by using a separate function for it. Before you claim something seems unreasonable from the above logic, please read the latest state of affairs with respect to data driven Vs functional API evolution discussion over the firmware API [0] as well as my latests recommendation for what to do for the async firmware API [1]. The skinny of it is that long ago I actually proposed having only *two* firmware API calls, an async and a sync call and having all functionality fleshed out through a structure of parameters. The issue with that strategy was it was *too* data driven, and as per Greg's request we'll instead be exposing new symbols per functionality for the firmware API with his justification that this is just what is traditionally done on Linux. Hence we have firmware_request_nowarn() now for just a slight variation for a sync call. Despite Greg's recommendation -- for the respective async functionality I suggested this is not going to scale well -- it is also just dumb to follow the same approach there for a few reasons. 1) We have only *one* async call and had decided to *not* provide a structure for that call since its inception 2) Over time have evolved this single async call each time we have a new feature, causing a slew of collateral evolutions. So, while we like it or not, it turns out the async call to the firmware API is already completely data driven. Extending it with just another argument would just be silly now. So refer to my recommendations to Andres for how to evolve the async API if you need it, however from a quick review you don't need async calls, so you won't have to address any of that. [0] https://lkml.kernel.org/r/20180421173650.gw14...@wotan.suse.de [1] https://lkml.kernel.org/r/20180422202609.gx14...@wotan.suse.de > Note I've deliberately named it with_system_firmware_fallback > and not with_efi_fallback to have the name be platform agnostic in > case we want something similar on other platforms in the future. firmware_request_platform() ? > And then have this pass a new FW_OPT_SYS_FW_FALLBACK flag to > _request_firmware(), right ? Yeap. > 2) Should this flag then be checked inside _request_firmware() before it > calls fw_get_efi_embedded_fw() (which may be an empty stub), You are the architect behind this call, so its up to you. To answer this you have to review the other flags and see if other users of the other flags may want your functionality. For instance the Android folks for instance rely on the FW_OPT_NOFALLBACK - the sysfs fallback mechanism to account for odd partition layouts. Could they ever want to use your fallback mechanism? Granted your mechanism is for x86, but they could eventually add support for it on ARM. Checking if the firmware is on the EFI platform firmware list is much faster than the fallback mechanism, that would be one gain for them, as such it may make sense to check for firmware_request_platform() before using the sysfs fallback mechanism. However if Android folks want to always override the platform firmware with the sysfs fallback interface we'd need another flag added and call to then change the order later if we checked for for the platform firmware first. If you however are 100% sure they won't need it, than checking firmware_request_platform() first would make sense now if you are certain these same devices won't need the s
[PATCH] Documentation: update firmware loader fallback reference
The firmware loader has a fallback mechanism, and it now has some proper kdoc, but we forgot to update the Documentation to use the new kdoc. Fix that. Signed-off-by: Luis R. Rodriguez --- Documentation/driver-api/firmware/fallback-mechanisms.rst | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst index d35fed65eae9..8b041d0ab426 100644 --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst @@ -92,7 +92,7 @@ the loading file. The firmware device used to help load firmware using sysfs is only created if direct firmware loading fails and if the fallback mechanism is enabled for your -firmware request, this is set up with fw_load_from_user_helper(). It is +firmware request, this is set up with :c:func:`firmware_fallback_sysfs`. It is important to re-iterate that no device is created if a direct filesystem lookup succeeded. @@ -108,6 +108,11 @@ firmware_data_read() and firmware_loading_show() are just provided for the test_firmware driver for testing, they are not called in normal use or expected to be used regularly by userspace. +firmware_fallback_sysfs +--- +.. kernel-doc:: drivers/base/firmware_loader/fallback.c + :functions: firmware_fallback_sysfs + Firmware kobject uevent fallback mechanism == -- 2.16.3
Re: [PATCH v6 2/5] efi: Add embedded peripheral firmware support
On Fri, Jun 01, 2018 at 02:53:27PM +0200, Hans de Goede wrote: > Just like with PCI options ROMs, which we save in the setup_efi_pci* > functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself > sometimes may contain data which is useful/necessary for peripheral drivers > to have access to. > > Specifically the EFI code may contain an embedded copy of firmware which > needs to be (re)loaded into the peripheral. Normally such firmware would be > part of linux-firmware, but in some cases this is not feasible, for 2 > reasons: > > 1) The firmware is customized for a specific use-case of the chipset / use > with a specific hardware model, so we cannot have a single firmware file > for the chipset. E.g. touchscreen controller firmwares are compiled > specifically for the hardware model they are used with, as they are > calibrated for a specific model digitizer. > > 2) Despite repeated attempts we have failed to get permission to > redistribute the firmware. This is especially a problem with customized > firmwares, these get created by the chip vendor for a specific ODM and the > copyright may partially belong with the ODM, so the chip vendor cannot > give a blanket permission to distribute these. > > This commit adds support for finding peripheral firmware embedded in the > EFI code and making this available to peripheral drivers through the > standard firmware loading mechanism. > > Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end > of start_kernel(), just before calling rest_init(), this is on purpose > because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for > early_memremap(), so the check must be done after mm_init(). This relies > on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services() > is called, which means that this will only work on x86 for now. > > Reported-by: Dave Olsthoorn > Suggested-by: Peter Jones > Acked-by: Ard Biesheuvel > Signed-off-by: Hans de Goede > --- > Changes in v6: > -Rework code to remove casts from if (prefix == mem) comparison > -Use SHA256 hashes instead of crc32 sums > -Add new READING_FIRMWARE_EFI_EMBEDDED read_file_id and use it > -Call security_kernel_read_file(NULL, READING_FIRMWARE_EFI_EMBEDDED) > to check if this is allowed before looking at EFI embedded fw > -Document why we are not using the PI Firmware Volume protocol > > Changes in v5: > -Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS > > Changes in v4: > -Drop note in docs about EFI_FIRMWARE_VOLUME_PROTOCOL, it is not part of > UEFI proper, so the EFI maintainers don't want us referring people to it > -Use new EFI_BOOT_SERVICES flag > -Put the new fw_get_efi_embedded_fw() function in its own fallback_efi.c > file which only gets built when EFI_EMBEDDED_FIRMWARE is selected > -Define an empty stub for fw_get_efi_embedded_fw() in fallback.h hwen > EFI_EMBEDDED_FIRMWARE is not selected, to avoid the need for #ifdefs > in firmware_loader/main.c > -Properly call security_kernel_post_read_file() on the firmware returned > by efi_get_embedded_fw() to make sure that we are allowed to use it > > Changes in v3: > -Fix the docs using "efi-embedded-fw" as property name instead of > "efi-embedded-firmware" > > Changes in v2: > -Rebased on driver-core/driver-core-next > -Add documentation describing the EFI embedded firmware mechanism to: > Documentation/driver-api/firmware/request_firmware.rst > -Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded > fw support if this is set. This is an invisible option which should be > selected by drivers which need this > -Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices > from the efi-embedded-fw code, instead drivers using this are expected to > export a dmi_system_id array, with each entries' driver_data pointing to a > efi_embedded_fw_desc struct and register this with the efi-embedded-fw code > -Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware, > this avoids us messing with the EFI memmap and avoids the need to make > changes to efi_mem_desc_lookup() > -Make the firmware-loader code only fallback to efi_get_embedded_fw() if the > passed in device has the "efi-embedded-firmware" device-property bool set > -Skip usermodehelper fallback when "efi-embedded-firmware" device-property > is set > --- > .../driver-api/firmware/request_firmware.rst | 76 + > drivers/base/firmware_loader/Makefile | 1 + > drivers/base/firmware_loader/fallback.h | 12 ++ > drivers/base/firmware_loader/fallback_efi.c | 56 +++ > drivers/base/firmware_loader/main.c | 2 + > drivers/firmware/efi/Kconfig | 3 + > drivers/firmware/efi/Makefile | 1 + > drivers/firmware/efi/embedded-firmware.c | 148 ++ > include/linux/efi.h | 6 + > include/linux/efi_embedded_fw.h | 25 +++ > include/li
Re: [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback)
On Tue, May 29, 2018 at 02:01:57PM -0400, Mimi Zohar wrote:
> Luis, is the security_kernel_post_read_file LSM hook in
> firmware_loading_store() still needed after this patch? Should it be
> calling security_kernel_load_data() instead?
That's up to Kees to decide as he added that hook, and knows
what LSMs may be doing with it. From my perspective it is confusing
to have that hook there so I think it could be removed now.
Kees?
Luis
>
> ---
>
> With an IMA policy requiring signed firmware, this patch prevents
> the sysfs fallback method of loading firmware.
>
> Signed-off-by: Mimi Zohar
> Cc: Luis R. Rodriguez
> Cc: David Howells
> Cc: Matthew Garrett
> ---
> security/integrity/ima/ima_main.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index a565d46084c2..4a87f78098c8 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -475,8 +475,10 @@ int ima_post_read_file(struct file *file, void *buf,
> loff_t size,
>
> if (!file && read_id == READING_FIRMWARE) {
> if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> - (ima_appraise & IMA_APPRAISE_ENFORCE))
> + (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> + pr_err("Prevent firmware loading_store.\n");
> return -EACCES; /* INTEGRITY_UNKNOWN */
> + }
> return 0;
> }
>
> @@ -520,6 +522,12 @@ int ima_load_data(enum kernel_load_data_id id)
> pr_err("impossible to appraise a kernel image without a
> file descriptor; try using kexec_file_load syscall.\n");
> return -EACCES; /* INTEGRITY_UNKNOWN */
> }
> + break;
> + case LOADING_FIRMWARE:
> + if (ima_appraise & IMA_APPRAISE_FIRMWARE) {
> + pr_err("Prevent firmware sysfs fallback loading.\n");
> + return -EACCES; /* INTEGRITY_UNKNOWN */
> + }
> default:
> break;
> }
> --
> 2.7.5
>
>
--
Do not panic
Re: [PATCH v3 0/2] mm: PAGE_KERNEL_* fallbacks
On Wed, May 30, 2018 at 10:06:08PM +0200, Greg KH wrote: > On Wed, May 30, 2018 at 09:55:00PM +0200, Luis R. Rodriguez wrote: > > On Wed, May 23, 2018 at 11:35:51PM +0200, Luis R. Rodriguez wrote: > > > On Wed, May 16, 2018 at 06:44:03PM +0200, Luis R. Rodriguez wrote: > > > > On Thu, May 10, 2018 at 11:55:05AM -0700, Luis R. Rodriguez wrote: > > > > > This is the 3rd iteration for moving PAGE_KERNEL_* fallback > > > > > definitions into asm-generic headers. Greg asked for a Changelog > > > > > for patch iteration changes, its below. > > > > > > > > > > All these patches have been tested by 0-day. > > > > > > > > > > Questions, and specially flames are greatly appreciated. > > > > > > > > *Poke* > > > > > > Greg, since this does touch the firmware loader as well, *poke*. > > > > *Re-re-poke* > > Hah, they are not for me to take, sorry, that's up to the mm maintainer. I'm not sure it is up to mm actually, since this is all include/asm-generic/pgtable.h it seems this falls onto Arnd. Its probably *best* mm folks decide though. Arnd, are you OK if Andrew picks this up if he finds no issues with the patches? I'll bouncing copies to Andrew now. Luis
Re: [PATCH v3 0/2] mm: PAGE_KERNEL_* fallbacks
On Wed, May 23, 2018 at 11:35:51PM +0200, Luis R. Rodriguez wrote: > On Wed, May 16, 2018 at 06:44:03PM +0200, Luis R. Rodriguez wrote: > > On Thu, May 10, 2018 at 11:55:05AM -0700, Luis R. Rodriguez wrote: > > > This is the 3rd iteration for moving PAGE_KERNEL_* fallback > > > definitions into asm-generic headers. Greg asked for a Changelog > > > for patch iteration changes, its below. > > > > > > All these patches have been tested by 0-day. > > > > > > Questions, and specially flames are greatly appreciated. > > > > *Poke* > > Greg, since this does touch the firmware loader as well, *poke*. *Re-re-poke* Luis
Re: PostgreSQL licensed code on Linux
On Wed, May 30, 2018 at 02:22:14AM +0300, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 2:12 AM, Luis R. Rodriguez wrote:
> > It would seem I did follow up with a v3 patch and Rusty noted that although
> > I may be right, its hard to care [0]. But of relevance here is again if one
> > of the MODULE_LICENSE() dual tags should be used or the GPL tag. I'll
> > continue to side recommending with the MODULE_LICENSE("GPL") tag even on
> > files with permissive licenses, and even if it we haven't clarified this in
> > documentation as I think scaling these tags further is just silly.
> >
> > [0] http://lkml.kernel.org/r/87bom0hf0f@rustcorp.com.au
>
> https://www.spinics.net/lists/linux-bcache/msg06048.html
>
> https://www.spinics.net/lists/linux-bcache/msg06058.html
For those that are not developers:
The proposed changes referenced in the above URLs take old portions PostgreSQL
C code which were previously on a larger C file and move them to a new module
which has the PostgreSQL header. Modules need to have a MODULE_LICENSE() tag,
and if one is not used the kernel assumes the module is proprietary. The above
code lacks a MODULE_LICENSE() tag as such currently the driver is proprietary.
Clearly that needs to be fixed before upstreaming.
Luis
Re: PostgreSQL licensed code on Linux
It would seem I did follow up with a v3 patch and Rusty noted that although
I may be right, its hard to care [0]. But of relevance here is again if one
of the MODULE_LICENSE() dual tags should be used or the GPL tag. I'll
continue to side recommending with the MODULE_LICENSE("GPL") tag even on
files with permissive licenses, and even if it we haven't clarified this in
documentation as I think scaling these tags further is just silly.
[0] http://lkml.kernel.org/r/87bom0hf0f@rustcorp.com.au
Luis
Re: PostgreSQL licensed code on Linux
On Tue, May 29, 2018 at 05:00:25PM -0400, Kent Overstreet wrote:
> On Tue, May 29, 2018 at 04:51:44PM -0400, Theodore Y. Ts'o wrote:
> > On Tue, May 29, 2018 at 03:26:43PM -0400, Kent Overstreet wrote:
> > > > That seems to indicate that we've had already PostgreSQL licensed code
> > > > on
> > > > Linux since Kent's addition of bcache to Linux in 2013. The portion of
> > > > code
> > > > is rather small though, to me it seems to cover only crc_table[],
> > > > bch_crc64_update(), and bch_crc64().
> > >
> > > > As silly as it may be we should split out the PostgreSQL licensed code
> > > > from
> > > > drivers/md/bcache/util.c into its own file and while at it clarify the
> > > > license.
> >
> > While we're at it maybe we should move the crc-64 code to lib and/or
> > crypto, alongside our support for crc-8, crc-16, and crc-32
> > algorithms? That way if there are other potential users for crc-64,
> > they will be less likely to re-invent the wheel
>
> Yeah, this came up because Coly wanted to do that, but needed to know what to
> put in MODULE_LICENSE().
At run time its GPL, so MODULE_LICENSE("GPL") would make sense. I had
sent a patch to help clarify this in 2012, I'll resend now [0].
[0] https://lkml.org/lkml/2012/4/8/75
Luis
Re: PostgreSQL licensed code on Linux
On Tue, May 29, 2018 at 03:26:43PM -0400, Kent Overstreet wrote: > On Tue, May 29, 2018 at 12:14:01PM -0700, Luis R. Rodriguez wrote: > > The question over future possible PostgreSQL licensed code on Linux came up > > to me recently. While doing some quick of digging around I found code > > already under such license it seems: > > > > The file drivers/md/bcache/util.c has: > > > > cafe563591446 (Kent Overstreet 2013-03-23 16:11:31 -0700 318) /* > > cafe563591446 (Kent Overstreet 2013-03-23 16:11:31 -0700 319) * Portions > > Copyright (c) 1996-2001, PostgreSQL Global Development Group (Any > > cafe563591446 (Kent Overstreet 2013-03-23 16:11:31 -0700 320) * use > > permitted, subject to terms of PostgreSQL license; see.) > > > > That seems to indicate that we've had already PostgreSQL licensed code on > > Linux since Kent's addition of bcache to Linux in 2013. The portion of code > > is rather small though, to me it seems to cover only crc_table[], > > bch_crc64_update(), and bch_crc64(). Four things: > > Yep, it's just that code. Great, thanks for the confirmation. > > a) This is the only code on Linux which seems to use PostgreSQL > > b) The language for license seem to be cut off, 'see.' seems incomplete, > > whereas typically it would point to a file with the full language text. > > c) We can only infer what portions of the file are under this license > > d) Even though some licenses claim to be GPL-Compatible, if possible we > > should dual license such with the GPL if possible (*) > > > > If some folks are considering adding yet more code to Linux which is > > currently under a PostgreSQL license I figured reviewing the existing > > PostgreSQL code's use may be a good start to set precedent for future work. > > Since we already have at least one file with a PostgreSQL-sort-of boiler > > plate it at least sets the precedent we have already sort of dealt with > > PostgreSQL. > > > > My recommendations: > > > > As silly as it may be we should split out the PostgreSQL licensed code from > > drivers/md/bcache/util.c into its own file and while at it clarify the > > license. > > > > If possible, if we can dual license this code with GPL it would be good as > > it would do two things: > > > > 1) Removes any ambiguity in case of questions over GPL Compatibility in the > > future about the PostgreSQL license > > > > 2) Other folks considering using PostgreSQL licensed code on Linux have a > > template they can use > > Sounds good to me, I'll defer to your judgement since you have more experience > with these things than me :) Let me know if there's anything you need from > me. I > never modified that code besides renaming the functions, but dual licensing > would be fine by me. IANAL, but my recommendations below. Trying to get all interested parties on Linux to agree PostgreSQL is indeed GPL-Compatible is certainly possible but may require a bit of legal billable hours on quite a bit of parts in the community. It takes a long time... Dual licensing would be preferred to avoid adding yet-another-license and possibile ambiguities over compatibility, however, that would require the original copyright holder's permission. You can poke if you'd like, however there are two other alternatives. a) License new code to GPL and add provenance notice for PostgreSQL - Useful if we know upstream PostgreSQL does not care for our own changes b) Dual license GPL/PostgreSQL with provenance notice for the original PostgreSQL code. - Useful if we know PostgreSQL may be interested in reaping benefit of our own changes on Linux as well. a) and b) are possible if you made changes to the code (even space and style changes count). If you opt for a) our code on Linux and evolutions of it remains GPL, but would annotate provenance from the PostgreSQL license. It'd include language such as: --- * This file incorporates work covered by the following copyright and * permission notice: --- So for instance this strategy was done on the carl9170 device driver rewrite where Johannes took ISC licensed otus device driver from staging and rewrote the driver based on it, an example file with the notice: drivers/net/wireless/ath/carl9170/phy.c This followed the guidence previously provided by SFLC on dealing with this [0]. But since there may be other code coming up we have to consider what the goals are. - Is th
PostgreSQL licensed code on Linux
The question over future possible PostgreSQL licensed code on Linux came up to me recently. While doing some quick of digging around I found code already under such license it seems: The file drivers/md/bcache/util.c has: cafe563591446 (Kent Overstreet 2013-03-23 16:11:31 -0700 318) /* cafe563591446 (Kent Overstreet 2013-03-23 16:11:31 -0700 319) * Portions Copyright (c) 1996-2001, PostgreSQL Global Development Group (Any cafe563591446 (Kent Overstreet 2013-03-23 16:11:31 -0700 320) * use permitted, subject to terms of PostgreSQL license; see.) That seems to indicate that we've had already PostgreSQL licensed code on Linux since Kent's addition of bcache to Linux in 2013. The portion of code is rather small though, to me it seems to cover only crc_table[], bch_crc64_update(), and bch_crc64(). Four things: a) This is the only code on Linux which seems to use PostgreSQL b) The language for license seem to be cut off, 'see.' seems incomplete, whereas typically it would point to a file with the full language text. c) We can only infer what portions of the file are under this license d) Even though some licenses claim to be GPL-Compatible, if possible we should dual license such with the GPL if possible (*) If some folks are considering adding yet more code to Linux which is currently under a PostgreSQL license I figured reviewing the existing PostgreSQL code's use may be a good start to set precedent for future work. Since we already have at least one file with a PostgreSQL-sort-of boiler plate it at least sets the precedent we have already sort of dealt with PostgreSQL. My recommendations: As silly as it may be we should split out the PostgreSQL licensed code from drivers/md/bcache/util.c into its own file and while at it clarify the license. If possible, if we can dual license this code with GPL it would be good as it would do two things: 1) Removes any ambiguity in case of questions over GPL Compatibility in the future about the PostgreSQL license 2) Other folks considering using PostgreSQL licensed code on Linux have a template they can use Other thoughts? * Although some websites / organizations may state a license is GPL compatible we have to be careful in the kernel as some companies or organizations may disagree with some of these views (example is FSF's website and position -- an example was one of the old BSD licenses which some folks questioned its GPL compatibility with); despite the ambiguity possible with dual licensing language [0], if one chooses a clear language it can be extremely useful and cautious on the kernel community's part. [0] https://www.softwarefreedom.org/resources/2007/gpl-non-gpl-collaboration.html Luis
Re: [PATCH v3 0/2] mm: PAGE_KERNEL_* fallbacks
On Wed, May 16, 2018 at 06:44:03PM +0200, Luis R. Rodriguez wrote: > On Thu, May 10, 2018 at 11:55:05AM -0700, Luis R. Rodriguez wrote: > > This is the 3rd iteration for moving PAGE_KERNEL_* fallback > > definitions into asm-generic headers. Greg asked for a Changelog > > for patch iteration changes, its below. > > > > All these patches have been tested by 0-day. > > > > Questions, and specially flames are greatly appreciated. > > *Poke* Greg, since this does touch the firmware loader as well, *poke*. Luis
Re: [PATCH v3 0/2] mm: PAGE_KERNEL_* fallbacks
On Thu, May 10, 2018 at 11:55:05AM -0700, Luis R. Rodriguez wrote: > This is the 3rd iteration for moving PAGE_KERNEL_* fallback > definitions into asm-generic headers. Greg asked for a Changelog > for patch iteration changes, its below. > > All these patches have been tested by 0-day. > > Questions, and specially flames are greatly appreciated. *Poke* Who's tree should this go through? Luis > > v3: > > Removed documentation effort to keep tabs on which architectures > currently don't defint the respective PAGE_* flags. Keeping tabs > on this is just not worth it. > > Ran a spell checker on all patches :) > > v2: > > I added a patch for PAGE_KERNEL_EXEC as suggested by Matthew Wilcox. > > v1: > > I sent out a patch just for dealing witht he fallback mechanism for > PAGE_KERNEL_RO. > > Luis R. Rodriguez (2): > mm: provide a fallback for PAGE_KERNEL_RO for architectures > mm: provide a fallback for PAGE_KERNEL_EXEC for architectures > > drivers/base/firmware_loader/fallback.c | 5 - > include/asm-generic/pgtable.h | 18 ++ > mm/nommu.c | 4 > mm/vmalloc.c| 4 > 4 files changed, 18 insertions(+), 13 deletions(-) > > -- > 2.17.0 > > -- Do not panic
Re: Are media drivers abusing of GFP_DMA? - was: Re: [LSF/MM TOPIC NOTES] x86 ZONE_DMA love
On Mon, May 14, 2018 at 07:35:03AM -0300, Mauro Carvalho Chehab wrote: > Hi Fabien, > > Em Mon, 14 May 2018 08:00:37 + > Fabien DESSENNE escreveu: > > > On 07/05/18 17:19, Mauro Carvalho Chehab wrote: > > > Em Mon, 07 May 2018 16:26:08 +0300 > > > Laurent Pinchart escreveu: > > > > > >> Hi Mauro, > > >> > > >> On Saturday, 5 May 2018 19:08:15 EEST Mauro Carvalho Chehab wrote: > > >>> There was a recent discussion about the use/abuse of GFP_DMA flag when > > >>> allocating memories at LSF/MM 2018 (see Luis notes enclosed). > > >>> > > >>> The idea seems to be to remove it, using CMA instead. Before doing that, > > >>> better to check if what we have on media is are valid use cases for it, > > >>> or > > >>> if it is there just due to some misunderstanding (or because it was > > >>> copied from some other code). > > >>> > > >>> Hans de Goede sent us today a patch stopping abuse at gspca, and I'm > > >>> also posting today two other patches meant to stop abuse of it on USB > > >>> drivers. Still, there are 4 platform drivers using it: > > >>> > > >>> $ git grep -l -E "GFP_DMA\\b" drivers/media/ > > >>> drivers/media/platform/omap3isp/ispstat.c > > >>> drivers/media/platform/sti/bdisp/bdisp-hw.c > > >>> drivers/media/platform/sti/hva/hva-mem.c > > > > Hi Mauro, > > > > The two STI drivers (bdisp-hw.c and hva-mem.c) are only expected to run > > on ARM platforms, not on x86. > > Since this thread deals with x86 & DMA trouble, I am not sure that we > > actually have a problem for the sti drivers. > > > > There are some other sti drivers that make use of this GFP_DMA flag > > (drivers/gpu/drm/sti/sti_*.c) and it does not seem to be a problem. > > > > Nevertheless I can see that the media sti drivers depend on COMPILE_TEST > > (which is not the case for the DRM ones). > > Would it be an acceptable solution to remove the COMPILE_TEST dependency? > > This has nothing to do with either x86 Actually it does. > or COMPILE_TEST. The thing is > that there's a plan for removing GFP_DMA from the Kernel[1], That would not be possible given architectures use GFP_DMA for other things and there are plenty of legacy x86 drivers which still need to be around. So the focus from mm folks shifted to letting x86 folks map GFP_DMA onto the CMA pool. Long term, this is nothing that driver developers need to care for, but just knowing internally behind the scenes there is some cleaning up being done in terms of architecture. > as it was > originally meant to be used only by old PCs, where the DMA controllers > used only on the bottom 16 MB memory address (24 bits). This is actually the part that is x86 specific. Each other architecture may use it for some other definition and it seems some architectures use GFP_DMA all over the place. So the topic really should be about x86. > IMHO, it is > very unlikely that any ARM SoC have such limitation. Right, how the flag is used on other architectures varies, so in fact the focus for cleaning up for now should be an x86 effort. Whether or not other architectures do something silly with GFP_DMA is beyond the scope of what was discussed. Luis
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Mon, May 14, 2018 at 10:02:31PM -0400, Mimi Zohar wrote:
> On Mon, 2018-05-14 at 19:28 +0000, Luis R. Rodriguez wrote:
> > > - CONFIG_IMA_APPRAISE is not fine enough grained.
> > >
> > > The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option. Similar
> > > Kconfig options will require kernel modules, kexec'ed image, and the
> > > IMA policy to be signed.
> >
> > Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
> > doing firmware verification in userspace or in the kernel.
>
> The kernel is verifying signatures.
>
> > > There are a number of reasons that the kernel should be verifying
> > > firmware signatures (eg. requiring a specific version of the firmware,
> > > that was locally signed).
> >
> > Oh I agree, Linux enterprise distributions also have a strong reason to
> > have this, so that for instance we only trust and run vendor-approved
> > signed firmware. Otherwise the driver should reject the firmware. Every
> > now and then enterprise distros may run into cases were certain customers
> > may run oddball firmwares, and its unclear if we expect proper functionality
> > with that firmware. Having some form of firmware signing would help with
> > this pipeline, but this is currently dealt with at the packaging, and
> > noting other than logs ensures the driver is using an intended firmware.
> > But these needs *IMHO* have not been enough to push to generalize a kernel
> > firmware signing facility.
>
> In order for IMA-appraisal to verify firmware signatures, the
> signatures need to be distributed with the firmware. Perhaps this
> will be enough of an incentive for distros to start including firmware
> signatures in the packages.
Best to poke the maintainers about that... We have been sending mixed messages
about firmware signing over years now. Josh, heads up the new one is we can
do firmware signing through IMA future CONFIG_IMA_APPRAISE_FIRMWARE. I'll
bounce you a few emails related to this.
> > If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality
> > somehow
> > I'm happy to hear it.
>
> The functionality has been there since commit 5a9196d ("ima: add
> support for measuring and appraising firmware"). The
> security_kernel_fw_from_file() hook was later replaced with the
> generic security_kernel_read_file() hook.
Groovy, its unclear from the code on that commit how this is done, so I
suppose I need to study this a bit more. Josh, do you grok it?
Luis
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Mon, May 14, 2018 at 08:58:12AM -0400, Mimi Zohar wrote: > On Fri, 2018-05-11 at 21:52 +0000, Luis R. Rodriguez wrote: > > diff --git a/drivers/base/firmware_loader/main.c > > b/drivers/base/firmware_loader/main.c > > index eb34089e4299..d7cdf04a8681 100644 > > --- a/drivers/base/firmware_loader/main.c > > +++ b/drivers/base/firmware_loader/main.c > > @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device *device, > > struct fw_priv *fw_priv) > > break; > > } > > > > +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB > > + if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) || > > + (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0)) > > + id = READING_FIRMWARE_REGULATORY_DB; > > +#endif > > fw_priv->size = 0; > > rc = kernel_read_file_from_path(path, &fw_priv->data, &size, > > msize, id); > > > > This is eye-soring, and in turn would mean adding yet more #ifdefs for any > > code on the kernel which open codes other firmware signing efforts with > > its own kconfig... > > Agreed, adding ifdef's is ugly. As previously discussed, this code > will be removed. > > To coordinate the signature verification, at build time either regdb > or IMA-appraisal firmware will be enabled. But not both, right? > At runtime, in the case > that regdb is enabled and a custom policy requires IMA-appraisal > firmware signature verification, then both signature verification > methods will verify the signatures. If either fails, then the > signature verification will fail. OK so you're saying that if CONFIG_IMA_APPRAISE_FIRMWARE is disabled you can still end up with CONFIG_CFG80211_REQUIRE_SIGNED_REGDB as enabled *and* a custom policy which requires IMA-appraisal for the certain firmware signature verifications? > > Then as for my concern on extending and adding new READING_* ID tags > > for other future open-coded firmware calls, since: > > > > a) You seem to be working on a CONFIG_IMA_APPRAISE_FIRMWARE which would > > enable similar functionality as firmware signing but in userspace. > > There are a number of different builtin policies. The "secure_boot" > builtin policy enables file signature verification for firmware, > kernel modules, kexec'ed image, and the IMA policy, but builtin > policies are enabled on the boot command line. > > There are two problems: > - there's no way of configuring a builtin policy to verify firmware > signatures. I'm not too familiar with IMA however it sounds like you can extend the IMA built-in policy on the boot command line. If so I'll note MODULE_FIRMWARE() macro is typically used to annotate firmware description -- not all drivers use this though for a few reasons, for instance once is that some names are constructed dynamically at run time. Consider how some drivers rev firmware with versions, and as they do this they want to keep certain features only for certain firmware versions. Despite this lack of a direct 1-1 mapping for all firmwares needed, I *believe* one current use case for this macro is to extract required firmwares needed on early boot so they can be stashed into the initramfs if you have these modules enabled on the initramfs. Dracut folks can confirm but -- dracut *seems* broken then given the semantic gap I mentioned above. dracut-init.sh:for _fw in $(modinfo -k $kernel -F firmware $1 2>/dev/null); do If I read this correctly this just complains if the firmware file is missing if the module is installed on initramfs and the firmware file is not present. If so we have a current semantic gap and modules with dynamic names are not handled. And its unclear which modules would be affected. This is a not a big issue for dracut though since not all drivers strive to be included on initramfs, unless their storage I suppose -- not sure how common these storage drivers are for initramfs with dynamic firmware names which do *not* use MODULE_FIRMWARE(). While the idea of MODULE_FIRMWARE() may need to be given some love or adjusted to incorporate globs / regexps to fix this existing gap, or we come up with something more reliable, if fixed, it in theory could end up being re-used to enable you to extract and build policies for firmware signing at build time... > - CONFIG_IMA_APPRAISE is not fine enough grained. > > The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option. Similar > Kconfig options will require kernel modules, kexec'ed image, and the > IMA policy to be signed. Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be doing fir
Re: [PATCH 6/9] firmware: print firmware name on fallback path
On Sat, May 12, 2018 at 11:03:52AM +0300, Kalle Valo wrote:
> (sorry for the delay, this got buried in my inbox)
>
> "Luis R. Rodriguez" writes:
>
> > On Mon, Apr 23, 2018 at 04:12:02PM -0400, Andres Rodriguez wrote:
> >> Previously, one could assume the firmware name from the preceding
> >> message: "Direct firmware load for {name} failed with error %d".
> >>
> >> However, with the new firmware_request_nowarn() entrypoint, the message
> >> outlined above will not always be printed.
> >
> > I though the whole point was to not print an error message. What if
> > we want later to disable this error message? This would prove a bit
> > pointless.
> >
> > Let's discuss the exact semantics desired here. Why would only the
> > fallback be desirable here?
> >
> > Andres, Kalle?
>
> So from ath10k point of view we do not want to have any messages printed
> when calling firmware_request_nowarn(). The warnings get users really
> confused when ath10k is checking if an optional firmware file is
> available or not.
I figured, that is the intended functionality now.
Luis
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Fri, May 11, 2018 at 01:00:26AM -0400, Mimi Zohar wrote: > On Thu, 2018-05-10 at 23:26 +0000, Luis R. Rodriguez wrote: > > On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote: > > > On Wed, 2018-05-09 at 23:48 +, Luis R. Rodriguez wrote: > > > > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote: > > > > > > > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable. The LSM > > > > > > > would differentiate between other firmware and the regulatory.db > > > > > > > based > > > > > > > on the firmware's pathname. > > > > > > > > > > > > If that is the only way then it would be silly to do the mini LSM > > > > > > as all > > > > > > calls would have to have the check. A special LSM hook for just the > > > > > > regulatory db also doesn't make much sense. > > > > > > > > > > All calls to request_firmware() are already going through this LSM > > > > > hook. I should have said, it would be based on both READING_FIRMWARE > > > > > and the firmware's pathname. > > > > > > > > Yes, but it would still be a strcmp() computation added for all > > > > READING_FIRMWARE. In that sense, the current arrangement is only open > > > > coding the > > > > signature verification for the regulatory.db file. One way to avoid > > > > this would > > > > be to add an LSM specific to the regulatory db > > > > > > Casey already commented on this suggestion. > > > > Sorry but I must have missed this, can you send me the email or URL where > > he did that? > > I never got a copy of that email I think. > > My mistake. I've posted similar patches for kexec_load and for the > firmware sysfs fallback, both call security_kernel_read_file(). > Casey's comment was in regards to kexec_load[1], not for the sysfs > fallback mode. Here's the link to the most recent version of the > kexec_load patches.[2] > > [1] > http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006690.html > [2] > http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006854.html It seems I share Eric's concern on these threads are over general architecture, below some notes which I think may help for the long term on that regards. In the firmware_loader case we have *one* subsystem which as open coded firmware signing -- the wireless subsystem open codes firmware verification by doing two request_firmware() calls, one for the regulatory.bin and one for regulatory.bin.p7s, and then it does its own check. In this patch set you suggested adding a new READING_FIRMWARE_REGULATORY_DB. But your first patch in the series also adds READING_FIRMWARE_FALLBACK for the fallback case where we enable use of the old syfs loading facility. My concerns are two fold for this case: a) This would mean adding a new READING_* ID tag per any kernel mechanism which open codes its own signature verification scheme. b) The way it was implemented was to do (just showing READING_FIRMWARE_REGULATORY_DB here): diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index eb34089e4299..d7cdf04a8681 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv) break; } +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB + if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) || + (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0)) + id = READING_FIRMWARE_REGULATORY_DB; +#endif fw_priv->size = 0; rc = kernel_read_file_from_path(path, &fw_priv->data, &size, msize, id); This is eye-soring, and in turn would mean adding yet more #ifdefs for any code on the kernel which open codes other firmware signing efforts with its own kconfig... I gather from reading the threads above that Eric's concerns are the re-use of an API for security to read files for something which is really not a file, but a binary blob of some sort and Casey's rebuttal is adding more hooks for small things is a bad idea. In light of all this I'll say that the concerns Eric has are unfortunately too late, that ship has sailed eons ago. The old non-fd API for module loading init_module() calls security_kernel_read_file(NULL, READING_MODULE). Your patch in this series adds security_kernel_read_file(NULL, READING_FIRMW
Re: [PATCH] coredump: rename umh_pipe_setup() to coredump_pipe_setup()
On Fri, May 11, 2018 at 03:48:51AM +0100, Al Viro wrote: > On Thu, May 10, 2018 at 11:32:47PM +0000, Luis R. Rodriguez wrote: > > > I think net-next makes sense if Al Viro is OK with that. This way it could > > go > > in regardless of the state of your series, but it also lines up with your > > work. > > Fine by me... OK thanks. Dave, I'll bounce a copy of the original patch to you, if anything else is needed please let me know. Luis
Re: [PATCH] coredump: rename umh_pipe_setup() to coredump_pipe_setup()
On Thu, May 10, 2018 at 04:19:09PM -0700, Alexei Starovoitov wrote: > On Mon, May 07, 2018 at 04:30:02PM -0700, Luis R. Rodriguez wrote: > > This makes it clearer this code is part of the coredump code, and > > is not an exported generic helper from kernel/umh.c. > > > > Signed-off-by: Luis R. Rodriguez > > --- > > fs/coredump.c | 9 + > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/fs/coredump.c b/fs/coredump.c > > index 1e2c87acac9b..566504781683 100644 > > --- a/fs/coredump.c > > +++ b/fs/coredump.c > > @@ -508,7 +508,7 @@ static void wait_for_dump_helpers(struct file *file) > > } > > > > /* > > - * umh_pipe_setup > > + * coredump_pipe_setup > > * helper function to customize the process used > > * to collect the core in userspace. Specifically > > * it sets up a pipe and installs it as fd 0 (stdin) > > @@ -518,7 +518,7 @@ static void wait_for_dump_helpers(struct file *file) > > * is a special value that we use to trap recursive > > * core dumps > > */ > > -static int umh_pipe_setup(struct subprocess_info *info, struct cred *new) > > +static int coredump_pipe_setup(struct subprocess_info *info, struct cred > > *new) > > I think this renaming makes sense. > How do we want to proceed? > I can take it as part of my series and get the whole thing through net-next > or folks want to apply this separately? I think net-next makes sense if Al Viro is OK with that. This way it could go in regardless of the state of your series, but it also lines up with your work. Luis
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote: > On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote: > > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote: > > > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable. The LSM > > > > > would differentiate between other firmware and the regulatory.db based > > > > > on the firmware's pathname. > > > > > > > > If that is the only way then it would be silly to do the mini LSM as all > > > > calls would have to have the check. A special LSM hook for just the > > > > regulatory db also doesn't make much sense. > > > > > > All calls to request_firmware() are already going through this LSM > > > hook. I should have said, it would be based on both READING_FIRMWARE > > > and the firmware's pathname. > > > > Yes, but it would still be a strcmp() computation added for all > > READING_FIRMWARE. In that sense, the current arrangement is only open > > coding the > > signature verification for the regulatory.db file. One way to avoid this > > would > > be to add an LSM specific to the regulatory db > > Casey already commented on this suggestion. Sorry but I must have missed this, can you send me the email or URL where he did that? I never got a copy of that email I think. Luis
[RFC v2 2/4] xfs: add verifier check for symlink with append/immutable flags
The Linux VFS does not allow a way to set append/immuttable attributes to symlinks, this is just not possible. If this is detected we can correct this with xfs_repair, so inform the user. Signed-off-by: Luis R. Rodriguez --- fs/xfs/libxfs/xfs_symlink_remote.c | 5 + 1 file changed, 5 insertions(+) diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c index 5ef5f354587e..42dd81ede3d6 100644 --- a/fs/xfs/libxfs/xfs_symlink_remote.c +++ b/fs/xfs/libxfs/xfs_symlink_remote.c @@ -242,5 +242,10 @@ xfs_symlink_shortform_verify( /* We /did/ null-terminate the buffer, right? */ if (*endp != 0) return __this_address; + + /* Immutable and append flags are not allowed on symlinks */ + if (ip->i_d.di_flags & (XFS_DIFLAG_APPEND | XFS_DIFLAG_IMMUTABLE)) + return __this_address; + return NULL; } -- 2.17.0
[RFC v2 0/4] vfs: detect symlink corruption with attributes
Filesystems which detect symlinks with append/immutable should inform users their filesystem is corrupted and their respective filesystem checker tool should fix this. In lieu of this though users may be stuck with pesky files or directories which they cannot remove. We cannot expect all filesystems to be updated to address this, so since the VFS does not let filesystems set these attributes -- let the VFS enable users to remove symlink with these attributes, but also provide a fallback warning, in case the users's own filesystem does not catch it. Sending again as RFC as this just goes compile tested so far, and it is still unclear if this is the direction we want to go with this. v2: As per Darrick, even though the VFS should probably allow not so popular filesystems to delete corrupt symlinks with append/immutable -- popular filesystems should check for this themselves and inform the users of corruption. These filesystems should have their respective filesystem checker tools updated to correct this as well. v1: Sent out a single patch just to ignore the append/immutable attributes set on symlinks. Luis R. Rodriguez (4): vfs: skip extra attributes check on removal for symlinks xfs: add verifier check for symlink with append/immutable flags ext4: add verifier check for symlink with append/immutable flags btrfs: verify symlinks with append/immutable flags fs/btrfs/inode.c | 9 + fs/ext4/inode.c| 7 +++ fs/namei.c | 24 ++-- fs/xfs/libxfs/xfs_symlink_remote.c | 5 + 4 files changed, 43 insertions(+), 2 deletions(-) -- 2.17.0
[RFC v2 3/4] ext4: add verifier check for symlink with append/immutable flags
The Linux VFS does not allow a way to set append/immuttable
attributes to symlinks, this is just not possible. If this is
detected inform the user as the filesystem must be corrupted.
Signed-off-by: Luis R. Rodriguez
---
fs/ext4/inode.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 37a2f7a2b66a..6acf0dd6b6e6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4947,6 +4947,13 @@ struct inode *ext4_iget(struct super_block *sb, unsigned
long ino)
inode->i_op = &ext4_dir_inode_operations;
inode->i_fop = &ext4_dir_operations;
} else if (S_ISLNK(inode->i_mode)) {
+ /* VFS does not allow setting these so must be corruption */
+ if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
+ EXT4_ERROR_INODE(inode,
+ "immutable or append flags not allowed on symlinks");
+ ret = -EFSCORRUPTED;
+ goto bad_inode;
+ }
if (ext4_encrypted_inode(inode)) {
inode->i_op = &ext4_encrypted_symlink_inode_operations;
ext4_set_aops(inode);
--
2.17.0
[RFC v2 4/4] btrfs: verify symlinks with append/immutable flags
The Linux VFS does not allow a way to set append/immuttable
attributes to symlinks, this is just not possible. If this is
detected inform the user as the filesystem must be corrupted.
Signed-off-by: Luis R. Rodriguez
---
fs/btrfs/inode.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c4bdb597b323..d9c786be408c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3933,6 +3933,15 @@ static int btrfs_read_locked_inode(struct inode *inode)
inode->i_op = &btrfs_dir_inode_operations;
break;
case S_IFLNK:
+ /* VFS does not allow setting these so must be corruption */
+ if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
+ ret = -EUCLEAN;
+ btrfs_crit(fs_info,
+ "corrupt symlink with append/immutable
ino=%llu root=%llu\n",
+ btrfs_ino(BTRFS_I(inode)),
+ root->root_key.objectid);
+ goto make_bad;
+ }
inode->i_op = &btrfs_symlink_inode_operations;
inode_nohighmem(inode);
inode->i_mapping->a_ops = &btrfs_symlink_aops;
--
2.17.0
[RFC v2 1/4] vfs: skip extra attributes check on removal for symlinks
Linux filesystems cannot set extra file attributes (stx_attributes as per
statx(2)) on a symbolic link. To set extra file attributes you issue
ioctl(2) with FS_IOC_SETFLAGS, *all* ioctl(2) calls on a symbolic link
yield EBADF.
This is because ioctl(2) tries to obtain struct fd from the symbolic link
file descriptor passed using fdget(), fdget() in turn always returns no
file set when a file descriptor is open with O_PATH. As per symlink(2)
O_PATH and O_NOFOLLOW must *always* be used when you want to get the file
descriptor of a symbolic link, and this holds true for Linux, as such extra
file attributes cannot possibly be set on symbolic links on Linux.
Filesystems repair utilities should be updated to detect this as
corruption and correct this, however, the VFS *does* respect these
extra attributes on symlinks for removal.
Since we cannot set these attributes we should special-case the
immutable/append on delete for symlinks, this would be consistent with
what we *do* allow on Linux for all filesystems. Since this is a clear
sign to the VFS the filesystem must be corrupted filesystems can
implement a verifier to catch this earlier. A generic warning issued for
filesystems which don't implement these verifiers, and the VFS also lets
users delete these pesky symlinks as otherwise users cannot get rid of
them.
The userspace utility chattr(1) cannot set these attributes on symlinks
*and* other special files as well:
# chattr -a symlink
chattr: Operation not supported while reading flags on b
The reason for this is different though. Refer to commit 023d111e92195
("chattr.1.in: Document the compression attribute flags E, X, and ...")
merged on e2fsprogs v1.28 since August 2002. This commit prevented
issuing the ioctl() for symlink *and* special files in consideration for
a buggy DRM driver where issuing lsattr on their special files crashed
the system. For details refer to Debian bug 152029 [0].
You can craft your own tool to query the extra file attributes with
the new shiny statx(2) tool, statx(2) will list the attributes if
they were set for instance on a corrupt filesystem. However statx(2)
is only used for *querying* -- not for setting the attributes.
If you implement issuing your own ioctl() for FS_IOC_FSGETXATTR or
FS_IOC_FSSETXATTR on special files (block, char, fifo) it will fail
returning -1 and errno is set to ENOTTY (Inappropriate ioctl for
device). The reason for this is different than for symlinks.
For special files this fails on vfs_ioctl() when the filesystem
f_op callbacks are not set for these special files:
long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
int error = -ENOTTY;
if (!filp->f_op->unlocked_ioctl)
goto out;
error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
if (error == -ENOIOCTLCMD)
error = -ENOTTY;
out:
return error;
}
The same applies to PF_LOCAL named sockets. Since this varies by
filesystem for special files, only make a special rule to respect
the immutable and append attribute on symlinks.
[0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=152029
Signed-off-by: Luis R. Rodriguez
---
fs/namei.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index e861b409c241..23ebc14805dc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2760,6 +2760,26 @@ int __check_sticky(struct inode *dir, struct inode
*inode)
}
EXPORT_SYMBOL(__check_sticky);
+/* Process extra file attributes only when they make sense */
+static bool may_delete_stx_attributes(struct inode *inode)
+{
+ /*
+* The VFS does not allow setting append/immutable on symlinks.
+*
+* Filesystems can implement their own verifier which would avoid this
+* generic splat, this generic splat is desirable if the respective
+* filesystem repair utility won't implement a fix for this, otherwise
+* users end up with a nagging dangling file which is impossible to
+* fix in userspace.
+*/
+ if (S_ISLNK(inode->i_mode)) {
+ WARN_ONCE((IS_APPEND(inode) || IS_IMMUTABLE(inode)),
+"Immutable or append flag set on symlink. VFS does not allow
this, must be a filesystem corruption. Allowing deletion though");
+ } else if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
+ return false;
+ return true;
+}
+
/*
* Check whether we can remove a link victim from directory dir, check
* whether the type of victim is right.
@@ -2798,8 +2818,8 @@ static int may_delete(struct inode *dir, struct dentry
*victim, bool isdir)
if (IS_APPEND(dir))
return -EPERM;
- if (check_sticky(dir, inode) || IS_APPEND(inode) ||
- IS_IMMUTABLE(inode) || IS_SWAPFILE(inode) || HAS_UNMAPPED_ID(inode))
+ if (check_sticky(dir, inode) || !may_delete_stx_attri
Re: [RFC] vfs: skip extra attributes check on removal for symlinks
On Thu, May 10, 2018 at 09:48:07PM +0100, Al Viro wrote: > On Thu, Apr 26, 2018 at 04:46:39PM -0700, Luis R. Rodriguez wrote: > > > Since we cannot set these attributes we should special-case the > > immutable/append on delete for symlinks, this would be consistent with > > what we *do* allow on Linux for all filesystems. > > Er... So why not simply sanity-check it in places that set it on > inodes? The patch is not about sanity-checks on setters though as *that* is in place already. Its about the case where the filesystem gets corrupted and the VFS *still* does process these attributes for symlinks and still prevents deletion because of these attributes. So we already do not allow for settings these attributes. > If anything, I would suggest > * converting all places that set those in ->i_flags to > inode_set_flags() > * making inode_set_flags() check and return an error on > that... But if I misunderstood your suggestion please let me know. I'll send out a v2 RFC next which illustrates what filesystems can do for now. Luis
[PATCH v7 01/14] firmware: wrap FW_OPT_* into an enum
From: Andres Rodriguez
This should let us associate enum kdoc to these values.
While at it, kdocify the fw_opt.
Signed-off-by: Andres Rodriguez
Reviewed-by: Kees Cook
Acked-by: Luis R. Rodriguez
[mcgrof: coding style fixes, merge kdoc with enum move]
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_loader/fallback.c | 12
drivers/base/firmware_loader/fallback.h | 6 ++--
drivers/base/firmware_loader/firmware.h | 37 +++--
drivers/base/firmware_loader/main.c | 6 ++--
4 files changed, 42 insertions(+), 19 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..b57a7b3b4122 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -512,7 +512,7 @@ static const struct attribute_group *fw_dev_attr_groups[] =
{
static struct fw_sysfs *
fw_create_instance(struct firmware *firmware, const char *fw_name,
- struct device *device, unsigned int opt_flags)
+ struct device *device, enum fw_opt opt_flags)
{
struct fw_sysfs *fw_sysfs;
struct device *f_dev;
@@ -545,7 +545,7 @@ fw_create_instance(struct firmware *firmware, const char
*fw_name,
* In charge of constructing a sysfs fallback interface for firmware loading.
**/
static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
- unsigned int opt_flags, long timeout)
+ enum fw_opt opt_flags, long timeout)
{
int retval = 0;
struct device *f_dev = &fw_sysfs->dev;
@@ -599,7 +599,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
static int fw_load_from_user_helper(struct firmware *firmware,
const char *name, struct device *device,
- unsigned int opt_flags)
+ enum fw_opt opt_flags)
{
struct fw_sysfs *fw_sysfs;
long timeout;
@@ -640,7 +640,7 @@ static int fw_load_from_user_helper(struct firmware
*firmware,
return ret;
}
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
{
if (fw_fallback_config.force_sysfs_fallback)
return true;
@@ -649,7 +649,7 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
return true;
}
-static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
{
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl
knob\n");
@@ -664,7 +664,7 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
int ret)
{
if (!fw_run_sysfs_fallback(opt_flags))
diff --git a/drivers/base/firmware_loader/fallback.h
b/drivers/base/firmware_loader/fallback.h
index f8255670a663..a3b73a09db6c 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -5,6 +5,8 @@
#include
#include
+#include "firmware.h"
+
/**
* struct firmware_fallback_config - firmware fallback configuration settings
*
@@ -31,7 +33,7 @@ struct firmware_fallback_config {
#ifdef CONFIG_FW_LOADER_USER_HELPER
int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
int ret);
void kill_pending_fw_fallback_reqs(bool only_kill_custom);
@@ -43,7 +45,7 @@ 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,
+ enum fw_opt opt_flags,
int ret)
{
/* Keep carrying over the same error */
diff --git a/drivers/base/firmware_loader/firmware.h
b/drivers/base/firmware_loader/firmware.h
index 64acbb1a392c..4f433b447367 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -2,6 +2,7 @@
#ifndef __FIRMWARE_LOADER_H
#define __FIRMWARE_LOADER_H
+#include
#include
#include
#include
@@ -10,13 +11,33 @@
#include
-/* 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
[PATCH v7 03/14] firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()
From: Andres Rodriguez
This is done since this call is now exposed through kernel-doc,
and since this also paves the way for different future types of
fallback mechanims.
Signed-off-by: Andres Rodriguez
Reviewed-by: Kees Cook
Acked-by: Luis R. Rodriguez
[mcgrof: small coding style changes]
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_loader/fallback.c | 8
drivers/base/firmware_loader/fallback.h | 16
drivers/base/firmware_loader/firmware.h | 2 +-
drivers/base/firmware_loader/main.c | 2 +-
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 529f7013616f..3db9e0f225ac 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,10 +662,10 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
}
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret)
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret)
{
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
diff --git a/drivers/base/firmware_loader/fallback.h
b/drivers/base/firmware_loader/fallback.h
index a3b73a09db6c..21063503e4ea 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -31,10 +31,10 @@ struct firmware_fallback_config {
};
#ifdef CONFIG_FW_LOADER_USER_HELPER
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret);
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret);
void kill_pending_fw_fallback_reqs(bool only_kill_custom);
void fw_fallback_set_cache_timeout(void);
@@ -43,10 +43,10 @@ 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,
- enum fw_opt opt_flags,
- int ret)
+static inline int firmware_fallback_sysfs(struct firmware *fw, const char
*name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret)
{
/* Keep carrying over the same error */
return ret;
diff --git a/drivers/base/firmware_loader/firmware.h
b/drivers/base/firmware_loader/firmware.h
index 4f433b447367..4c1395f8e7ed 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -20,7 +20,7 @@
* @FW_OPT_NOWAIT: Used to describe the firmware request is asynchronous.
* @FW_OPT_USERHELPER: Enable the fallback mechanism, in case the direct
* filesystem lookup fails at finding the firmware. For details refer to
- * fw_sysfs_fallback().
+ * firmware_fallback_sysfs().
* @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
* @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
* cache the firmware upon suspend, so that upon resume races against the
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index 4d11efadb3a4..abdc4e4d55f1 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -581,7 +581,7 @@ _request_firmware(const struct firmware **firmware_p, const
char *name,
dev_warn(device,
"Direct firmware load for %s failed with error
%d\n",
name, ret);
- ret = fw_sysfs_fallback(fw, name, device, opt_flags, ret);
+ ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
} else
ret = assign_fw(fw, device, opt_flags);
--
2.17.0
[PATCH v7 02/14] firmware: use () to terminate kernel-doc function names
From: Andres Rodriguez The kernel-doc spec dictates a function name ends in (). Signed-off-by: Andres Rodriguez Reviewed-by: Kees Cook Acked-by: Randy Dunlap Acked-by: Luis R. Rodriguez [mcgrof: adjust since the wide API rename is not yet merged] Signed-off-by: Luis R. Rodriguez --- drivers/base/firmware_loader/fallback.c | 8 drivers/base/firmware_loader/main.c | 22 +++--- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index b57a7b3b4122..529f7013616f 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -125,7 +125,7 @@ static ssize_t timeout_show(struct class *class, struct class_attribute *attr, } /** - * firmware_timeout_store - set number of seconds to wait for firmware + * 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 @@ -239,7 +239,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv) } /** - * firmware_loading_store - set value in the 'loading' control file + * 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 @@ -431,7 +431,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int min_size) } /** - * firmware_data_write - write method for firmware + * firmware_data_write() - write method for firmware * @filp: open sysfs file * @kobj: kobject for the device * @bin_attr: bin_attr structure @@ -537,7 +537,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name, } /** - * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism + * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism * @fw_sysfs: firmware sysfs information for the firmware to load * @opt_flags: flags of options, FW_OPT_* * @timeout: timeout to wait for the load diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index 9919f0e6a7cc..4d11efadb3a4 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -597,7 +597,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name, } /** - * request_firmware: - send firmware request and wait for it + * request_firmware() - send firmware request and wait for it * @firmware_p: pointer to firmware image * @name: name of firmware file * @device: device for which firmware is being loaded @@ -632,7 +632,7 @@ request_firmware(const struct firmware **firmware_p, const char *name, EXPORT_SYMBOL(request_firmware); /** - * request_firmware_direct: - load firmware directly without usermode helper + * request_firmware_direct() - load firmware directly without usermode helper * @firmware_p: pointer to firmware image * @name: name of firmware file * @device: device for which firmware is being loaded @@ -657,7 +657,7 @@ int request_firmware_direct(const struct firmware **firmware_p, EXPORT_SYMBOL_GPL(request_firmware_direct); /** - * firmware_request_cache: - cache firmware for suspend so resume can use it + * firmware_request_cache() - cache firmware for suspend so resume can use it * @name: name of firmware file * @device: device for which firmware should be cached for * @@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const char *name) EXPORT_SYMBOL_GPL(firmware_request_cache); /** - * request_firmware_into_buf - load firmware into a previously allocated buffer + * request_firmware_into_buf() - load firmware into a previously allocated buffer * @firmware_p: pointer to firmware image * @name: name of firmware file * @device: device for which firmware is being loaded and DMA region allocated @@ -713,7 +713,7 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name, EXPORT_SYMBOL(request_firmware_into_buf); /** - * release_firmware: - release the resource associated with a firmware image + * release_firmware() - release the resource associated with a firmware image * @fw: firmware resource to release **/ void release_firmware(const struct firmware *fw) @@ -755,7 +755,7 @@ static void request_firmware_work_func(struct work_struct *work) } /** - * request_firmware_nowait - asynchronous version of request_firmware + * request_firmware_nowait() - asynchronous version of request_firmware * @module: module requesting the firmware * @uevent: sends uevent to copy the firmware image if this flag * is non-zero else the firmware copy must be done manually. @@ -824,7 +824,7 @@ EXPORT_SYMBOL(request_firmware_nowait); static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain); /** - * cache_firmware - cache one firmware image in kernel memory space + * cache_fi
[PATCH v7 05/14] firmware_loader: enhance Kconfig documentation over FW_LOADER
If you try to read FW_LOADER today it speaks of old riddles and unless you have been following development closely you will lose track of what is what. Even the documentation for PREVENT_FIRMWARE_BUILD is a bit fuzzy and how it fits into this big picture. Give the FW_LOADER kconfig documentation some love with more up to date developments and recommendations. While at it, wrap the FW_LOADER code into its own menu to compartmentalize and make it clearer which components really are part of the FW_LOADER. This should also make it easier to later move these kconfig entries into the firmware_loader/ directory later. This also now recommends using firmwared [0] for folks left needing a uevent handler in userspace for the sysfs firmware fallback mechanis given udev's uevent firmware mechanism was ripped out a while ago. [0] https://github.com/teg/firmwared Reviewed-by: Kees Cook Signed-off-by: Luis R. Rodriguez --- drivers/base/Kconfig | 165 ++- 1 file changed, 131 insertions(+), 34 deletions(-) diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 29b0eb452b3a..db2bbe483927 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -70,39 +70,64 @@ config STANDALONE If unsure, say Y. config PREVENT_FIRMWARE_BUILD - bool "Prevent firmware from being built" + bool "Disable drivers features which enable custom firmware building" default y help - Say yes to avoid building firmware. Firmware is usually shipped - with the driver and only when updating the firmware should a - rebuild be made. - If unsure, say Y here. + Say yes to disable driver features which enable building a custom + driver firmware at kernel build time. These drivers do not use the + kernel firmware API to load firmware (CONFIG_FW_LOADER), instead they + use their own custom loading mechanism. The required firmware is + usually shipped with the driver, building the driver firmware + should only be needed if you have an updated firmware source. + + Firmware should not be being built as part of kernel, these days + you should always prevent this and say Y here. There are only two + old drivers which enable building of its firmware at kernel build + time: + + o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE + o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE + +menu "Firmware loader" config FW_LOADER - tristate "Userspace firmware loading support" if EXPERT + tristate "Firmware loading facility" if EXPERT default y ---help--- - This option is provided for the case where none of the in-tree modules - require userspace firmware loading support, but a module built - out-of-tree does. + This enables the firmware loading facility in the kernel. The kernel + will first look for built-in firmware, if it has any. Next, it will + look for the requested firmware in a series of filesystem paths: + + o firmware_class path module parameter or kernel boot param + o /lib/firmware/updates/UTS_RELEASE + o /lib/firmware/updates + o /lib/firmware/UTS_RELEASE + o /lib/firmware + + Enabling this feature only increases your kernel image by about + 828 bytes, enable this option unless you are certain you don't + need firmware. + + You typically want this built-in (=y) but you can also enable this + as a module, in which case the firmware_class module will be built. + You also want to be sure to enable this built-in if you are going to + enable built-in firmware (CONFIG_EXTRA_FIRMWARE). + +if FW_LOADER config EXTRA_FIRMWARE - string "External firmware blobs to build into the kernel binary" - depends on FW_LOADER + string "Build named firmware blobs into the kernel binary" help - Various drivers in the kernel source tree may require firmware, - which is generally available in your distribution's linux-firmware - package. + Device drivers which require firmware can typically deal with + having the kernel load firmware from the various supported + /lib/firmware/ paths. This option enables you to build into the + kernel firmware files. Built-in firmware searches are preceded + over firmware lookups using your filesystem over the supported + /lib/firmware paths documented on CONFIG_FW_LOADER. - The linux-firmware package should install firmware into - /lib/firmware/ on your system, so they can be loaded by userspace - helpers on request. - - This option allows firmware to be built into the kernel for the case -
[PATCH v7 04/14] firmware_loader: document firmware_sysfs_fallback()
This also sets the expecations for future fallback interfaces, even if they are not exported. Reviewed-by: Kees Cook Signed-off-by: Luis R. Rodriguez --- drivers/base/firmware_loader/fallback.c | 20 1 file changed, 20 insertions(+) diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index 3db9e0f225ac..9169e7b9800c 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -662,6 +662,26 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags) return fw_force_sysfs_fallback(opt_flags); } +/** + * firmware_fallback_sysfs() - use the fallback mechanism to find firmware + * @fw: pointer to firmware image + * @name: name of firmware file to look for + * @device: device for which firmware is being loaded + * @opt_flags: options to control firmware loading behaviour + * @ret: return value from direct lookup which triggered the fallback mechanism + * + * This function is called if direct lookup for the firmware failed, it enables + * a fallback mechanism through userspace by exposing a sysfs loading + * interface. Userspace is in charge of loading the firmware through the syfs + * loading interface. This syfs fallback mechanism may be disabled completely + * on a system by setting the proc sysctl value ignore_sysfs_fallback to true. + * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK + * flag, if so it would also disable the fallback mechanism. A system may want + * to enfoce the sysfs fallback mechanism at all times, it can do this by + * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true. + * Enabling force_sysfs_fallback is functionally equivalent to build a kernel + * with CONFIG_FW_LOADER_USER_HELPER_FALLBACK. + **/ int firmware_fallback_sysfs(struct firmware *fw, const char *name, struct device *device, enum fw_opt opt_flags, -- 2.17.0
[PATCH v7 07/14] firmware_loader: move kconfig FW_LOADER entries to its own file
This will make it easier to track and easier to understand
what components and features are part of the FW_LOADER. There
are some components related to firmware which have *nothing* to
do with the FW_LOADER, souch as PREVENT_FIRMWARE_BUILD.
Reviewed-by: Kees Cook
Signed-off-by: Luis R. Rodriguez
---
drivers/base/Kconfig | 155 +--
drivers/base/firmware_loader/Kconfig | 154 ++
2 files changed, 155 insertions(+), 154 deletions(-)
create mode 100644 drivers/base/firmware_loader/Kconfig
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 0c38df32c7fe..3e63a900b330 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -88,160 +88,7 @@ config PREVENT_FIRMWARE_BUILD
o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
-menu "Firmware loader"
-
-config FW_LOADER
- tristate "Firmware loading facility" if EXPERT
- default y
- help
- This enables the firmware loading facility in the kernel. The kernel
- will first look for built-in firmware, if it has any. Next, it will
- look for the requested firmware in a series of filesystem paths:
-
- o firmware_class path module parameter or kernel boot param
- o /lib/firmware/updates/UTS_RELEASE
- o /lib/firmware/updates
- o /lib/firmware/UTS_RELEASE
- o /lib/firmware
-
- Enabling this feature only increases your kernel image by about
- 828 bytes, enable this option unless you are certain you don't
- need firmware.
-
- You typically want this built-in (=y) but you can also enable this
- as a module, in which case the firmware_class module will be built.
- You also want to be sure to enable this built-in if you are going to
- enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
-
-if FW_LOADER
-
-config EXTRA_FIRMWARE
- string "Build named firmware blobs into the kernel binary"
- help
- Device drivers which require firmware can typically deal with
- having the kernel load firmware from the various supported
- /lib/firmware/ paths. This option enables you to build into the
- kernel firmware files. Built-in firmware searches are preceded
- over firmware lookups using your filesystem over the supported
- /lib/firmware paths documented on CONFIG_FW_LOADER.
-
- This may be useful for testing or if the firmware is required early on
- in boot and cannot rely on the firmware being placed in an initrd or
- initramfs.
-
- This option is a string and takes the (space-separated) names of the
- firmware files -- the same names that appear in MODULE_FIRMWARE()
- and request_firmware() in the source. These files should exist under
- the directory specified by the EXTRA_FIRMWARE_DIR option, which is
- /lib/firmware by default.
-
- For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
- the usb8388.bin file into /lib/firmware, and build the kernel. Then
- any request_firmware("usb8388.bin") will be satisfied internally
- inside the kernel without ever looking at your filesystem at runtime.
-
- WARNING: If you include additional firmware files into your binary
- kernel image that are not available under the terms of the GPL,
- then it may be a violation of the GPL to distribute the resulting
- image since it combines both GPL and non-GPL work. You should
- consult a lawyer of your own before distributing such an image.
-
-config EXTRA_FIRMWARE_DIR
- string "Firmware blobs root directory"
- depends on EXTRA_FIRMWARE != ""
- default "/lib/firmware"
- help
- This option controls the directory in which the kernel build system
- looks for the firmware files listed in the EXTRA_FIRMWARE option.
-
-config FW_LOADER_USER_HELPER
- bool "Enable the firmware sysfs fallback mechanism"
- help
- This option enables a sysfs loading facility to enable firmware
- loading to the kernel through userspace as a fallback mechanism
- if and only if the kernel's direct filesystem lookup for the
- firmware failed using the different /lib/firmware/ paths, or the
- path specified in the firmware_class path module parameter, or the
- firmware_class path kernel boot parameter if the firmware_class is
- built-in. For details on how to work with the sysfs fallback mechanism
- refer to Documentation/driver-api/firmware/fallback-mechanisms.rst.
-
- The direct filesystem lookup for firmware is always used first now.
-
- If the kernel's direct filesyste
[PATCH v7 08/14] firmware_loader: make firmware_fallback_sysfs() print more useful
If we resort to using the sysfs fallback mechanism we don't print the filename. This can be deceiving given we could have a series of callers intertwined and it'd be unclear exactly for what firmware this was meant for. Additionally, although we don't currently use FW_OPT_NO_WARN when dealing with the fallback mechanism, we will soon, so just respect its use consistently. And even if you *don't* want to print always on failure, you may want to print when debugging so enable dynamic debug print when FW_OPT_NO_WARN is used. Reviewed-by: Kees Cook Signed-off-by: Luis R. Rodriguez --- drivers/base/firmware_loader/fallback.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index 9169e7b9800c..b676a99c469c 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -690,6 +690,11 @@ int firmware_fallback_sysfs(struct firmware *fw, const char *name, if (!fw_run_sysfs_fallback(opt_flags)) return ret; - dev_warn(device, "Falling back to user helper\n"); + if (!(opt_flags & FW_OPT_NO_WARN)) + dev_warn(device, "Falling back to syfs fallback for: %s\n", +name); + else + dev_dbg(device, "Falling back to sysfs fallback for: %s\n", + name); return fw_load_from_user_helper(fw, name, device, opt_flags); } -- 2.17.0
[PATCH v7 06/14] firmware_loader: replace ---help--- with help
As per checkpatch using help is preferred over ---help---. Signed-off-by: Luis R. Rodriguez --- drivers/base/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index db2bbe483927..0c38df32c7fe 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -93,7 +93,7 @@ menu "Firmware loader" config FW_LOADER tristate "Firmware loading facility" if EXPERT default y - ---help--- + help This enables the firmware loading facility in the kernel. The kernel will first look for built-in firmware, if it has any. Next, it will look for the requested firmware in a series of filesystem paths: -- 2.17.0
[PATCH v7 10/14] ath10k: use firmware_request_nowarn() to load firmware
From: Andres Rodriguez This reduces the unnecessary spew when trying to load optional firmware: "Direct firmware load for ... failed with error -2" Signed-off-by: Andres Rodriguez Reviewed-by: Kees Cook Acked-by: Kalle Valo Signed-off-by: Luis R. Rodriguez --- drivers/net/wireless/ath/ath10k/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index 4cf54a7ef09a..ad4f6e3c0737 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -694,7 +694,7 @@ static const struct firmware *ath10k_fetch_fw_file(struct ath10k *ar, dir = "."; snprintf(filename, sizeof(filename), "%s/%s", dir, file); - ret = request_firmware(&fw, filename, ar->dev); + ret = firmware_request_nowarn(&fw, filename, ar->dev); ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot fw request '%s': %d\n", filename, ret); -- 2.17.0
[PATCH v7 14/14] Documentation: clarify firmware_class provenance and why we can't rename the module
Clarify the provenance of the firmware loader firmware_class module name and why we cannot rename the module in the future. Reviewed-by: Mauro Carvalho Chehab Reviewed-by: Kees Cook Signed-off-by: Luis R. Rodriguez --- .../driver-api/firmware/fallback-mechanisms.rst | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst index a39323ef7d29..d35fed65eae9 100644 --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst @@ -72,9 +72,12 @@ the firmware requested, and establishes it in the device hierarchy by associating the device used to make the request as the device's parent. The sysfs directory's file attributes are defined and controlled through the new device's class (firmware_class) and group (fw_dev_attr_groups). -This is actually where the original firmware_class.c file name comes from, -as originally the only firmware loading mechanism available was the -mechanism we now use as a fallback mechanism. +This is actually where the original firmware_class module name came from, +given that originally the only firmware loading mechanism available was the +mechanism we now use as a fallback mechanism, which registers a struct class +firmware_class. Because the attributes exposed are part of the module name, the +module name firmware_class cannot be renamed in the future, to ensure backward +compatibility with old userspace. To load firmware using the sysfs interface we expose a loading indicator, and a file upload firmware into: -- 2.17.0
[PATCH v7 11/14] ath10k: re-enable the firmware fallback mechanism for testmode
From: Andres Rodriguez The ath10k testmode uses request_firmware_direct() in order to avoid producing firmware load warnings. Disabling the fallback mechanism was a side effect of disabling warnings. We now have a new API that allows us to avoid warnings while keeping the fallback mechanism enabled. So use that instead. Signed-off-by: Andres Rodriguez Reviewed-by: Kees Cook Acked-by: Kalle Valo Signed-off-by: Luis R. Rodriguez --- drivers/net/wireless/ath/ath10k/testmode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath10k/testmode.c b/drivers/net/wireless/ath/ath10k/testmode.c index 568810b41657..c24ee616833c 100644 --- a/drivers/net/wireless/ath/ath10k/testmode.c +++ b/drivers/net/wireless/ath/ath10k/testmode.c @@ -157,7 +157,7 @@ static int ath10k_tm_fetch_utf_firmware_api_1(struct ath10k *ar, ar->hw_params.fw.dir, ATH10K_FW_UTF_FILE); /* load utf firmware image */ - ret = request_firmware_direct(&fw_file->firmware, filename, ar->dev); + ret = firmware_request_nowarn(&fw_file->firmware, filename, ar->dev); ath10k_dbg(ar, ATH10K_DBG_TESTMODE, "testmode fw request '%s': %d\n", filename, ret); -- 2.17.0
[PATCH v7 13/14] Documentation: remove stale firmware API reference
It refers to a pending patch, but this was merged eons ago. Reviewed-by: Mauro Carvalho Chehab Reviewed-by: Kees Cook Signed-off-by: Luis R. Rodriguez --- Documentation/dell_rbu.txt | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Documentation/dell_rbu.txt b/Documentation/dell_rbu.txt index 0fdb6aa2704c..5d1ce7bcd04d 100644 --- a/Documentation/dell_rbu.txt +++ b/Documentation/dell_rbu.txt @@ -121,10 +121,7 @@ read back the image downloaded. .. note:: - This driver requires a patch for firmware_class.c which has the modified - request_firmware_nowait function. - - Also after updating the BIOS image a user mode application needs to execute + After updating the BIOS image a user mode application needs to execute code which sends the BIOS update request to the BIOS. So on the next reboot the BIOS knows about the new image downloaded and it updates itself. Also don't unload the rbu driver if the image has to be updated. -- 2.17.0
[PATCH v7 00/14] firmware_loader changes for v4.18
Greg, This is what I have queued up for the firmware_loader for v4.18. Below is a changelog of the changes, mostly addressing Andre's changes and a few other set of cleanups and documentation updates on my part and a few others pointed out by others. Mimi will still be working on her changes later to add IMA firmware signature support, that should not touch the firmware loader code at all, and should help provide a generic alternative to the wireless regulatory signing work -- for all firmware. Hans's EFI changes are still being worked on. These changes are available in git form on my linux-next [0] and linux [1] trees on kernel.org on the branch name firmware_loader_for-v4.18. Hans, feel free to work off of that for your next iteration. Questions, and specially rants are greatly appreciated. [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180510-firmware_loader_for-v4.18 [1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180510-firmware_loader_for-v4.18 v7: - Annoted Reviewed-by tags. - Installed codespell and am now using checkpatch.sh --codespell to check each patch for spell check and blunders. Edited all files for corrections missed by other reviewers. - checkpatch complained about using ---help--- when I moved the Kconfig file for the firmware loader so I replaced that now with what it suggests. - fixed the other series of typos pickd up by reviewers v6: - Added the firmware_request_nowarn() patch again, now that we've decided on not to warn even for the fallback case. v5: - I took up Andres' patches and cleaned them up a bit further with minor details which would have otherwise have taken a few more iterations to do. - Dropped the firmware_request_nowarn() patch since there was some inconsistencies over actually not warning on the fallback case - Added some further firmware loader Kconfig clarifications which have come up through recent discussions. - I dropped my firmware API rename changes for v4.18, so I adjusted Andres' patches accordingly. v4: - Andres removed the async nowarn API call firmware_request_nowait2() from his series, given we got into a discussion over how to properly add a new async API to the firmware loader and there was no real consensus. Given I was tired of the bikeshedding I offered for folks recommend a solution and for us to just go with it. None solution was provided by any. To not leave Andres hanging during patch review, I made a set of recommendations but suggested that the asycn API be punted until *after* we get the simple sync nowarn API added. The recommendation still stands and my hope is that Andres will follow up with it later: https://lkml.kernel.org/r/20180422202609.gx14...@wotan.suse.de - Andres dropped the drm/amdgpu/brcmfmac driver changes based on feedback and becuase the async nowarn call was dropped in favor of addressing this later. v3: - Andres fixed typos picked up by Randy Dunlap. v2: - Andres added request_firmware_nowait2() - Based on feedback Andres extended his patches to include use of the new API on the drivers amdgpu, ath10k and brcmfmac. v1: Andres only posted one patch to add request_firmware_optional() without any users Andres Rodriguez (6): firmware: wrap FW_OPT_* into an enum firmware: use () to terminate kernel-doc function names firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs() firmware: add firmware_request_nowarn() - load firmware without warnings ath10k: use firmware_request_nowarn() to load firmware ath10k: re-enable the firmware fallback mechanism for testmode Luis R. Rodriguez (8): firmware_loader: document firmware_sysfs_fallback() firmware_loader: enhance Kconfig documentation over FW_LOADER firmware_loader: replace ---help--- with help firmware_loader: move kconfig FW_LOADER entries to its own file firmware_loader: make firmware_fallback_sysfs() print more useful Documentation: fix few typos and clarifications for the firmware loader Documentation: remove stale firmware API reference Documentation: clarify firmware_class provenance and why we can't rename the module Documentation/dell_rbu.txt| 5 +- .../firmware/fallback-mechanisms.rst | 14 +- .../driver-api/firmware/firmware_cache.rst| 4 +- .../driver-api/firmware/request_firmware.rst | 5 + drivers/base/Kconfig | 90 ++ drivers/base/firmware_loader/Kconfig | 154 ++ drivers/base/firmware_loader/fallback.c | 53 -- drivers/base/firmware_loader/fallback.h | 18 +- drivers/base/firmware_loader/firmware.h | 37 - drivers/base/firmware_loader/main.c | 57 +-- drivers/net/wireless/ath/ath10k/core.c| 2 +- drivers/net/wireless/ath/ath10k/testmode.c| 2 +- include/linux/firmware.h | 10 ++ 13 files changed, 320 insertion
[PATCH v7 09/14] firmware: add firmware_request_nowarn() - load firmware without warnings
From: Andres Rodriguez
Currently the firmware loader only exposes one silent path for querying
optional firmware, and that is firmware_request_direct(). This function
also disables the sysfs fallback mechanism, which might not always be the
desired behaviour [0].
This patch introduces a variations of request_firmware() that enable the
caller to disable the undesired warning messages but enables the sysfs
fallback mechanism. This is equivalent to adding FW_OPT_NO_WARN to the
old behaviour.
[0]: https://git.kernel.org/linus/c0cc00f250e1
Signed-off-by: Andres Rodriguez
Reviewed-by: Kees Cook
Acked-by: Luis R. Rodriguez
[mcgrof: used the old API calls as the full rename is not done yet, and
add the caller for when FW_LOADER is disabled, enhance documentation ]
Signed-off-by: Luis R. Rodriguez
---
.../driver-api/firmware/request_firmware.rst | 5
drivers/base/firmware_loader/main.c | 27 +++
include/linux/firmware.h | 10 +++
3 files changed, 42 insertions(+)
diff --git a/Documentation/driver-api/firmware/request_firmware.rst
b/Documentation/driver-api/firmware/request_firmware.rst
index d5ec95a7195b..f62bdcbfed5b 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -20,6 +20,11 @@ request_firmware
.. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: request_firmware
+firmware_request_nowarn
+---
+.. kernel-doc:: drivers/base/firmware_loader/main.c
+ :functions: firmware_request_nowarn
+
request_firmware_direct
---
.. kernel-doc:: drivers/base/firmware_loader/main.c
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index abdc4e4d55f1..0943e7065e0e 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -631,6 +631,33 @@ request_firmware(const struct firmware **firmware_p, const
char *name,
}
EXPORT_SYMBOL(request_firmware);
+/**
+ * firmware_request_nowarn() - request for an optional fw module
+ * @firmware: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ * The sysfs fallback mechanism is enabled if direct filesystem lookup fails,
+ * however, however failures to find the firmware file with it are still
+ * suppressed. It is therefore up to the driver to check for the return value
+ * of this call and to decide when to inform the users of errors.
+ **/
+int firmware_request_nowarn(const struct firmware **firmware, const char *name,
+ struct device *device)
+{
+ int ret;
+
+ /* Need to pin this module until return */
+ __module_get(THIS_MODULE);
+ ret = _request_firmware(firmware, name, device, NULL, 0,
+ FW_OPT_UEVENT | FW_OPT_NO_WARN);
+ module_put(THIS_MODULE);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(firmware_request_nowarn);
+
/**
* request_firmware_direct() - load firmware directly without usermode helper
* @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 41050417cafb..2dd566c91d44 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -42,6 +42,8 @@ struct builtin_fw {
#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) &&
defined(MODULE))
int request_firmware(const struct firmware **fw, const char *name,
struct device *device);
+int firmware_request_nowarn(const struct firmware **fw, const char *name,
+ struct device *device);
int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
@@ -59,6 +61,14 @@ static inline int request_firmware(const struct firmware
**fw,
{
return -EINVAL;
}
+
+static inline int firmware_request_nowarn(const struct firmware **fw,
+ const char *name,
+ struct device *device)
+{
+ return -EINVAL;
+}
+
static inline int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
--
2.17.0
[PATCH v7 12/14] Documentation: fix few typos and clarifications for the firmware loader
Fix a few typos, and clarify a few sentences. Reviewed-by: Kees Cook Signed-off-by: Luis R. Rodriguez --- Documentation/driver-api/firmware/fallback-mechanisms.rst | 5 +++-- Documentation/driver-api/firmware/firmware_cache.rst | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst index f353783ae0be..a39323ef7d29 100644 --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst @@ -83,7 +83,7 @@ and a file upload firmware into: * /sys/$DEVPATH/data To upload firmware you will echo 1 onto the loading file to indicate -you are loading firmware. You then cat the firmware into the data file, +you are loading firmware. You then write the firmware into the data file, and you notify the kernel the firmware is ready by echo'ing 0 onto the loading file. @@ -136,7 +136,8 @@ by kobject uevents. This is specially exacerbated due to the fact that most distributions today disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK. Refer to do_firmware_uevent() for details of the kobject event variables -setup. Variables passwdd with a kobject add event: +setup. The variables currently passed to userspace with a "kobject add" +event are: * FIRMWARE=firmware name * TIMEOUT=timeout value diff --git a/Documentation/driver-api/firmware/firmware_cache.rst b/Documentation/driver-api/firmware/firmware_cache.rst index 2210e5bfb332..c2e69d9c6bf1 100644 --- a/Documentation/driver-api/firmware/firmware_cache.rst +++ b/Documentation/driver-api/firmware/firmware_cache.rst @@ -29,8 +29,8 @@ Some implementation details about the firmware cache setup: * If an asynchronous call is used the firmware cache is only set up for a device if if the second argument (uevent) to request_firmware_nowait() is true. When uevent is true it requests that a kobject uevent be sent to - userspace for the firmware request. For details refer to the Fackback - mechanism documented below. + userspace for the firmware request through the sysfs fallback mechanism + if the firmware file is not found. * If the firmware cache is determined to be needed as per the above two criteria the firmware cache is setup by adding a devres entry for the -- 2.17.0
[PATCH v3 2/2] mm: provide a fallback for PAGE_KERNEL_EXEC for architectures
Some architectures just don't have PAGE_KERNEL_EXEC. The mm/nommu.c
and mm/vmalloc.c code have been using PAGE_KERNEL as a fallback for
years. Move this fallback to asm-generic.
Suggested-by: Matthew Wilcox
Signed-off-by: Luis R. Rodriguez
---
include/asm-generic/pgtable.h | 4
mm/nommu.c| 4
mm/vmalloc.c | 4
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index 4e310e543fc8..81371468ed5a 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -1097,6 +1097,10 @@ static inline void init_espfix_bsp(void) { }
# define PAGE_KERNEL_RO PAGE_KERNEL
#endif
+#ifndef PAGE_KERNEL_EXEC
+# define PAGE_KERNEL_EXEC PAGE_KERNEL
+#endif
+
#endif /* !__ASSEMBLY__ */
#ifndef io_remap_pfn_range
diff --git a/mm/nommu.c b/mm/nommu.c
index 13723736d38f..08ad4dcd281d 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -364,10 +364,6 @@ void *vzalloc_node(unsigned long size, int node)
}
EXPORT_SYMBOL(vzalloc_node);
-#ifndef PAGE_KERNEL_EXEC
-# define PAGE_KERNEL_EXEC PAGE_KERNEL
-#endif
-
/**
* vmalloc_exec - allocate virtually contiguous, executable memory
* @size: allocation size
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index ebff729cc956..89543d13e32a 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1920,10 +1920,6 @@ void *vzalloc_node(unsigned long size, int node)
}
EXPORT_SYMBOL(vzalloc_node);
-#ifndef PAGE_KERNEL_EXEC
-# define PAGE_KERNEL_EXEC PAGE_KERNEL
-#endif
-
/**
* vmalloc_exec - allocate virtually contiguous, executable memory
* @size: allocation size
--
2.17.0
[PATCH v3 1/2] mm: provide a fallback for PAGE_KERNEL_RO for architectures
Some architectures do not define certain PAGE_KERNEL_* flags, this is
either because:
a) The way to implement some of these flags is *not yet ported*, or
b) The architecture *has no way* to describe them
Over time we have accumulated a few PAGE_KERNEL_* fallback work arounds
for architectures in the kernel which do not define them using *relatively
safe* equivalents. Move these scattered fallback hacks into asm-generic.
We start off with PAGE_KERNEL_RO using PAGE_KERNEL as a fallback. This
has been in place on the firmware loader for years. Move the fallback
into the respective asm-generic header.
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_loader/fallback.c | 5 -
include/asm-generic/pgtable.h | 14 ++
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..36f016b753e0 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -219,11 +219,6 @@ static ssize_t firmware_loading_show(struct device *dev,
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)
{
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index f59639afaa39..4e310e543fc8 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -1083,6 +1083,20 @@ int phys_mem_access_prot_allowed(struct file *file,
unsigned long pfn,
static inline void init_espfix_bsp(void) { }
#endif
+/*
+ * Architecture PAGE_KERNEL_* fallbacks
+ *
+ * Some architectures don't define certain PAGE_KERNEL_* flags. This is either
+ * because they really don't support them, or the port needs to be updated to
+ * reflect the required functionality. Below are a set of relatively safe
+ * fallbacks, as best effort, which we can count on in lieu of the
architectures
+ * not defining them on their own yet.
+ */
+
+#ifndef PAGE_KERNEL_RO
+# define PAGE_KERNEL_RO PAGE_KERNEL
+#endif
+
#endif /* !__ASSEMBLY__ */
#ifndef io_remap_pfn_range
--
2.17.0
[PATCH v3 0/2] mm: PAGE_KERNEL_* fallbacks
This is the 3rd iteration for moving PAGE_KERNEL_* fallback definitions into asm-generic headers. Greg asked for a Changelog for patch iteration changes, its below. All these patches have been tested by 0-day. Questions, and specially flames are greatly appreciated. v3: Removed documentation effort to keep tabs on which architectures currently don't defint the respective PAGE_* flags. Keeping tabs on this is just not worth it. Ran a spell checker on all patches :) v2: I added a patch for PAGE_KERNEL_EXEC as suggested by Matthew Wilcox. v1: I sent out a patch just for dealing witht he fallback mechanism for PAGE_KERNEL_RO. Luis R. Rodriguez (2): mm: provide a fallback for PAGE_KERNEL_RO for architectures mm: provide a fallback for PAGE_KERNEL_EXEC for architectures drivers/base/firmware_loader/fallback.c | 5 - include/asm-generic/pgtable.h | 18 ++ mm/nommu.c | 4 mm/vmalloc.c| 4 4 files changed, 18 insertions(+), 13 deletions(-) -- 2.17.0
Re: [PATCH v2 0/2] mm: PAGE_KERNEL_* fallbacks
On Thu, May 10, 2018 at 08:07:33AM +0200, Greg KH wrote: > On Wed, May 09, 2018 at 06:44:45PM -0700, Luis R. Rodriguez wrote: > > While dusting out the firmware loader closet I spotted a PAGE_KERNEL_* > > fallback hack. This hurts my eyes, and it should also be blinding > > others. Turns out we have other PAGE_KERNEL_* fallback hacks in > > other places. > > > > This moves them to asm-generic, and keeps track of architectures which > > need some love or review. At least 0-day was happy with the changes. > > > > Matthew Wilcox did put together a PAGE_KERNEL_RO patch for ia64, that > > needs review and testing, and if it goes well it should be merged. > > > > Luis R. Rodriguez (2): > > mm: provide a fallback for PAGE_KERNEL_RO for architectures > > mm: provide a fallback for PAGE_KERNEL_EXEC for architectures > > > > drivers/base/firmware_loader/fallback.c | 5 > > include/asm-generic/pgtable.h | 36 + > > mm/nommu.c | 4 --- > > mm/vmalloc.c| 4 --- > > 4 files changed, 36 insertions(+), 13 deletions(-) > > No list of changes that happened from v1? :( Didn't know you'd want it for such simple patch set, but I'll provide one for v3 and also list the changes in v2. Luis
Re: [PATCH v2 2/2] mm: provide a fallback for PAGE_KERNEL_EXEC for architectures
On Thu, May 10, 2018 at 03:33:55PM +, Luis R. Rodriguez wrote: > On Thu, May 10, 2018 at 09:45:56AM +0200, Geert Uytterhoeven wrote: > > Hi Luis, > > > > On Thu, May 10, 2018 at 3:44 AM, Luis R. Rodriguez > > wrote: > > > Some architectures just don't have PAGE_KERNEL_EXEC. The mm/nommu.c > > > and mm/vmalloc.c code have been using PAGE_KERNEL as a fallback for years. > > > Move this fallback to asm-generic. > > > > > > Architectures which do not define PAGE_KERNEL_EXEC yet: > > > > > > o alpha > > > o mips > > > o openrisc > > > o sparc64 > > > > The above list seems to be far from complete? > > I'll look again. If you know of others lemme know. You know, better just ignore documenting these. I'll respin without that. Luis
Re: [PATCH v2 2/2] mm: provide a fallback for PAGE_KERNEL_EXEC for architectures
On Thu, May 10, 2018 at 09:45:56AM +0200, Geert Uytterhoeven wrote: > Hi Luis, > > On Thu, May 10, 2018 at 3:44 AM, Luis R. Rodriguez wrote: > > Some architectures just don't have PAGE_KERNEL_EXEC. The mm/nommu.c > > and mm/vmalloc.c code have been using PAGE_KERNEL as a fallback for years. > > Move this fallback to asm-generic. > > > > Architectures which do not define PAGE_KERNEL_EXEC yet: > > > > o alpha > > o mips > > o openrisc > > o sparc64 > > The above list seems to be far from complete? I'll look again. If you know of others lemme know. Luis
[PATCH v2 2/2] mm: provide a fallback for PAGE_KERNEL_EXEC for architectures
Some architectures just don't have PAGE_KERNEL_EXEC. The mm/nommu.c
and mm/vmalloc.c code have been using PAGE_KERNEL as a fallback for years.
Move this fallback to asm-generic.
Architectures which do not define PAGE_KERNEL_EXEC yet:
o alpha
o mips
o openrisc
o sparc64
Suggested-by: Matthew Wilcox
Signed-off-by: Luis R. Rodriguez
---
include/asm-generic/pgtable.h | 12
mm/nommu.c| 4
mm/vmalloc.c | 4
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index 890fc54f4713..39e9bd66c786 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -1107,6 +1107,18 @@ static inline void init_espfix_bsp(void) { }
# define PAGE_KERNEL_RO PAGE_KERNEL
#endif
+/*
+ * Current architectures known to not define PAGE_KERNEL_EXEC:
+ *
+ * o alpha
+ * o mips
+ * o openrisc
+ * o sparc64
+ */
+#ifndef PAGE_KERNEL_EXEC
+# define PAGE_KERNEL_EXEC PAGE_KERNEL
+#endif
+
#endif /* !__ASSEMBLY__ */
#ifndef io_remap_pfn_range
diff --git a/mm/nommu.c b/mm/nommu.c
index 13723736d38f..08ad4dcd281d 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -364,10 +364,6 @@ void *vzalloc_node(unsigned long size, int node)
}
EXPORT_SYMBOL(vzalloc_node);
-#ifndef PAGE_KERNEL_EXEC
-# define PAGE_KERNEL_EXEC PAGE_KERNEL
-#endif
-
/**
* vmalloc_exec - allocate virtually contiguous, executable memory
* @size: allocation size
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index ebff729cc956..89543d13e32a 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1920,10 +1920,6 @@ void *vzalloc_node(unsigned long size, int node)
}
EXPORT_SYMBOL(vzalloc_node);
-#ifndef PAGE_KERNEL_EXEC
-# define PAGE_KERNEL_EXEC PAGE_KERNEL
-#endif
-
/**
* vmalloc_exec - allocate virtually contiguous, executable memory
* @size: allocation size
--
2.17.0
[PATCH v2 0/2] mm: PAGE_KERNEL_* fallbacks
While dusting out the firmware loader closet I spotted a PAGE_KERNEL_* fallback hack. This hurts my eyes, and it should also be blinding others. Turns out we have other PAGE_KERNEL_* fallback hacks in other places. This moves them to asm-generic, and keeps track of architectures which need some love or review. At least 0-day was happy with the changes. Matthew Wilcox did put together a PAGE_KERNEL_RO patch for ia64, that needs review and testing, and if it goes well it should be merged. Luis R. Rodriguez (2): mm: provide a fallback for PAGE_KERNEL_RO for architectures mm: provide a fallback for PAGE_KERNEL_EXEC for architectures drivers/base/firmware_loader/fallback.c | 5 include/asm-generic/pgtable.h | 36 + mm/nommu.c | 4 --- mm/vmalloc.c| 4 --- 4 files changed, 36 insertions(+), 13 deletions(-) -- 2.17.0
[PATCH v2 1/2] mm: provide a fallback for PAGE_KERNEL_RO for architectures
Some architectures do not define certain PAGE_KERNEL_* flags, this is
either because:
a) The way to implement some these flags is *not yet ported*, or
b) The architecture *has no way* to describe them
Over time we have accumulated a few PAGE_KERNEL_* fallback work arounds for
architectures in the kernel which do not define them using *relatively safe*
equivalents. Move these scattered fallback hacks into asm-generic and
document which architectures needs further evaluation for which
PAGE_KERNEL_* flag.
We start off with PAGE_KERNEL_RO using PAGE_KERNEL as a fallback. This
has been in place on the firmware loader for years. Move the fallback
into the respective asm-generic header.
Architectures which don't define this yet:
o alpha
o ia64
o m68k
o mips
o sparc64
o sparc
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_loader/fallback.c | 5 -
include/asm-generic/pgtable.h | 24
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..36f016b753e0 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -219,11 +219,6 @@ static ssize_t firmware_loading_show(struct device *dev,
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)
{
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index f59639afaa39..890fc54f4713 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -1083,6 +1083,30 @@ int phys_mem_access_prot_allowed(struct file *file,
unsigned long pfn,
static inline void init_espfix_bsp(void) { }
#endif
+/*
+ * Architecture PAGE_KERNEL_* fallbacks
+ *
+ * Some architectures don't define certain PAGE_KERNEL_* flags. This is either
+ * because they really don't support them, or the port needs to be updated to
+ * reflect the required functionality. Below are a set of relatively safe
+ * fallbacks, as best effort, which we can count on in lieu of the
architectures
+ * not defining them on their own yet.
+ */
+
+/*
+ * Current architectures known to not define PAGE_KERNEL_RO:
+ *
+ * o alpha
+ * o ia64
+ * o m68k
+ * o mips
+ * o sparc64
+ * o sparc
+ */
+#ifndef PAGE_KERNEL_RO
+# define PAGE_KERNEL_RO PAGE_KERNEL
+#endif
+
#endif /* !__ASSEMBLY__ */
#ifndef io_remap_pfn_range
--
2.17.0
Re: [PATCH] mm: provide a fallback for PAGE_KERNEL_RO for architectures
On Wed, May 02, 2018 at 03:11:13PM +, Luis R. Rodriguez wrote: > On Wed, May 02, 2018 at 12:08:57PM +0200, Geert Uytterhoeven wrote: > > Hi Luis, > > > > On Sat, Apr 28, 2018 at 2:15 AM, Luis R. Rodriguez > > wrote: > > > Some architectures do not define PAGE_KERNEL_RO, best we can do > > > for them is to provide a fallback onto PAGE_KERNEL. Remove the > > > hack from the firmware loader and move it onto the asm-generic > > > header, and document while at it the affected architectures > > > which do not have a PAGE_KERNEL_RO: > > > > > > o alpha > > > o ia64 > > > o m68k > > > o mips > > > o sparc64 > > > o sparc > > > > > > Blessed-by: 0-day > > > Signed-off-by: Luis R. Rodriguez > > > > I believe the "best we can do" is to add the missing definitions for the > > architectures where the hardware does support it? > > True, but we cannot wait for every architecture to implement a feature to then > such generics upstream, Come to think of it your point was the wording. I changed it to not be as misleading. Luis
Re: [PATCH] mm: provide a fallback for PAGE_KERNEL_RO for architectures
On Tue, May 08, 2018 at 06:39:35PM -0700, Matthew Wilcox wrote: > On Wed, May 09, 2018 at 01:04:38AM +0000, Luis R. Rodriguez wrote: > > On Fri, Apr 27, 2018 at 08:18:10PM -0700, Matthew Wilcox wrote: > > > ia64: Add PAGE_KERNEL_RO and PAGE_KERNEL_EXEC > > > > > > The rest of the kernel was falling back to simple PAGE_KERNEL pages; using > > > PAGE_KERNEL_RO and PAGE_KERNEL_EXEC provide better protection against > > > unintended writes. > > > > > > Signed-off-by: Matthew Wilcox > > > > Nice, should I queue this into my series as well? > > A little reluctant to queue it without anyone having tested it. Heck, > I didn't even check it compiled ;-) > > We used to just break architectures and let them fix it up for this kind > of thing. History is wonderful. > That's not really acceptable nowadays, but I don't know how > we get arch maintainers to fix up their ports now. OK then in that case I'll proceed with my patches for now and just document they don't have it. Once and folks test the patch we can consider it. Luis
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote: > On Wed, 2018-05-09 at 21:22 +0000, Luis R. Rodriguez wrote: > > > > OK, its still not clear to what it will do. If it does not touch the > > firmware > > loader code, and it just sets and configures IMA to do file signature > > checking > > on its own, then yes I think both mechanisms would be doing the similar > > work. > > > > Wouldn't IMA do file signature checks then for all files? Or it would just > > enable this for whatever files userspace wishes to cover? > > Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware > signatures on all directly loaded firmware and fail any method of > loading firmware that the signature couldn't be verified. Ah, so a generic firmware signing mechanism via IMA. Sounds very sensible to me. Specially in light of the fact that its what we recommend folks to consider if they need to address firmware signing for devices which do not have the ability to do hardware firmware signing verification on their own. > > One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and > > trust > > the wireless-regdgb maintainer's key for this file, could IMA be configured > > to > > do that? > > IMA has its own trusted keyring. So either the maintainer's key would > need to be added to the IMA keyring, I see so we'd need this documented somehow. > or IMA-appraisal would need to use the regdb keyring. Can you describe this a bit more, for those not too familiar with IMA, in terms of what would be involved in the kernel? Or is this all userspace configuration stuff? > > Because that would be one difference here. The whole point of adding > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a > > userspace > > component which checks the signature of regulatory.db before reading it and > > passing data to the kernel from it. > > > > Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and* > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set > > you are mentioning, to still enable both to co-exist? > > At build, either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or > CONFIG_IMA_APPRAISE_FIRMWARE, where IMA is appraising all firwmare, > would be enabled, not both. OK. > > > Yes, writing regdb as a micro/mini LSM sounds reasonable. The LSM > > > would differentiate between other firmware and the regulatory.db based > > > on the firmware's pathname. > > > > If that is the only way then it would be silly to do the mini LSM as all > > calls would have to have the check. A special LSM hook for just the > > regulatory db also doesn't make much sense. > > All calls to request_firmware() are already going through this LSM > hook. I should have said, it would be based on both READING_FIRMWARE > and the firmware's pathname. Yes, but it would still be a strcmp() computation added for all READING_FIRMWARE. In that sense, the current arrangement is only open coding the signature verification for the regulatory.db file. One way to avoid this would be to add an LSM specific to the regulatory db and have the security_check_regulatory_db() do what it needs per LSM, but that would mean setting a precedent for open possibly open coded future firmware verification call. Its not too crazy to consider if an end goal may be avoid further open coded firmware signature verification hacks. > > > Making regdb an LSM would have the same issues as currently - deciding > > > if regdb, IMA-appraisal, or both verify the regdb's signature. > > > > Its unclear to me why they can't co-exist yet and not have to touch > > the firmware_loader code at all. > > With the changes discussed above, they will co-exist. Other than the > Kconfig changes, I don't think it will touch the firmware_loader code. Great. Luis
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Wed, May 09, 2018 at 03:57:18PM -0400, Mimi Zohar wrote: > On Wed, 2018-05-09 at 19:15 +0000, Luis R. Rodriguez wrote: > > > > > > If both are enabled, do we require both signatures or is one enough. > > > > > > > > Good question. Considering it as a stacked LSM (although not implemented > > > > as one), I'd say its up to who enabled the Kconfig entries. If IMA and > > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone > > > > enabled > > > > IMA though, then surely I agree that enabling > > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its > > > > up to the > > > > system integrator to decide. > > > > > > Just because IMA-appraisal is enabled in the kernel doesn't mean that > > > firmware signatures will be verified. That is a run time policy > > > decision. > > > > Sure, I accept this if IMA does not do signature verification. However > > signature verification seems like a stackable LSM decision, no? > > IMA-appraisal can be configured to enforce file signatures. Refer to > discussion below as to how. > > > > > If we however want to make it clear that such things as > > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is > > > > enabled we > > > > could just make the kconfig depend on !IMA or something? Or perhaps a > > > > new > > > > kconfig for IMA which if selected it means that drivers can opt to open > > > > code > > > > *further* kernel signature verification, even though IMA already is > > > > sufficient. > > > > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it? > > > > > > The existing CONFIG_IMA_APPRAISE is not enough. If there was a build > > > time IMA config that translated into an IMA policy requiring firmware > > > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could > > > be sorted out at build time. > > > > I see makes sense. > > Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll > post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described > above. OK, its still not clear to what it will do. If it does not touch the firmware loader code, and it just sets and configures IMA to do file signature checking on its own, then yes I think both mechanisms would be doing the similar work. Wouldn't IMA do file signature checks then for all files? Or it would just enable this for whatever files userspace wishes to cover? One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust the wireless-regdgb maintainer's key for this file, could IMA be configured to do that? Because that would be one difference here. The whole point of adding CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a userspace component which checks the signature of regulatory.db before reading it and passing data to the kernel from it. Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and* CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set you are mentioning, to still enable both to co-exist? > > > > > Assigning a different id for regdb signed firmware allows LSMs and IMA > > > > > to handle regdb files differently. > > > > > > > > That's not the main concern here, its the precedent we are setting here > > > > for > > > > any new kernel interface which open codes firmware signing on its own. > > > > What > > > > you are doing means other kernel users who open codes their own firmware > > > > signing may need to add yet-another reading ID. That doesn't either look > > > > well on code, and seems kind of silly from a coding perspective given > > > > the above, in which I clarify IMA still is doing its own appraisal on > > > > it. > > > > > > Suppose, > > > > > > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or > > > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build. > > > > > > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not > > > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that > > > appraises the firmware signature could be defined. In this case, both > > > signature verification methods would be enforced. > > > > > > then READING_FIRMWARE_REGULATORY_DB would not be needed. > > > > True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB > > could just be a mini subsystem stackable LSM. > > Yes, writing regdb as a micro/mini LSM sounds reasonable. The LSM > would differentiate between other firmware and the regulatory.db based > on the firmware's pathname. If that is the only way then it would be silly to do the mini LSM as all calls would have to have the check. A special LSM hook for just the regulatory db also doesn't make much sense. > Making regdb an LSM would have the same issues as currently - deciding > if regdb, IMA-appraisal, or both verify the regdb's signature. Its unclear to me why they can't co-exist yet and not have to touch the firmware_loader code at all. Luis
Re: [PATCH v6 05/13] firmware_loader: enhance Kconfig documentation over FW_LOADER
On Tue, May 08, 2018 at 03:42:33PM -0700, Kees Cook wrote: > On Tue, May 8, 2018 at 11:12 AM, Luis R. Rodriguez wrote: > > + This used to be the default firmware loading facility, and udev > > used > > + to listen for uvents to load firmware for the kernel. The firmware > > + loading facility functionality in udev has been removed, as such > > it > > + can no longer be relied upon as a fallback mechanism. Linux no > > longer > > + relies on or uses a fallback mechanism in userspace. If you need > > to > > + rely on one refer to the permissively licensed firmwared: > > Typo: firmware Thanks fixed all typos except this one, this one is meant to be firmwared as that is the name of the project, the url is below. > > > + > > + https://github.com/teg/firmwared Luis
Re: [PATCH v6 12/13] Documentation: remove stale firmware API reference
On Wed, May 09, 2018 at 12:12:09PM -0300, Mauro Carvalho Chehab wrote: > Em Tue, 8 May 2018 11:12:46 -0700 > "Luis R. Rodriguez" escreveu: > > > It refers to a pending patch, but this was merged eons ago. > > Didn't know that such patch was already merged. Great! > > Regards, > Mauro > > > > > Signed-off-by: Luis R. Rodriguez > > --- > > Documentation/dell_rbu.txt | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/Documentation/dell_rbu.txt b/Documentation/dell_rbu.txt > > index 0fdb6aa2704c..077fdc29a0d0 100644 > > --- a/Documentation/dell_rbu.txt > > +++ b/Documentation/dell_rbu.txt > > @@ -121,9 +121,6 @@ read back the image downloaded. > > > > .. note:: > > > > - This driver requires a patch for firmware_class.c which has the modified > > - request_firmware_nowait function. > > - > > > Also after updating the BIOS image a user mode application needs to > > execute > > code which sends the BIOS update request to the BIOS. So on the next > > reboot > > the BIOS knows about the new image downloaded and it updates itself. > > You should likely remove the "Also" here. Good catch. Will adjust. > > With that, > > Reviewed-by: Mauro Carvalho Chehab Great, thanks. Luis
Re: [PATCH v7 1/4] ipc: IPCMNI limit check for msgmni and shmmni
On Mon, May 07, 2018 at 07:57:12PM -0400, Waiman Long wrote:
> On 05/07/2018 06:39 PM, Luis R. Rodriguez wrote:
> > On Mon, May 07, 2018 at 04:59:09PM -0400, Waiman Long wrote:
> >> A user can write arbitrary integer values to msgmni and shmmni sysctl
> >> parameters without getting error, but the actual limit is really
> >> IPCMNI (32k). This can mislead users as they think they can get a
> >> value that is not real.
> >>
> >> The right limits are now set for msgmni and shmmni so that the users
> >> will become aware if they set a value outside of the acceptable range.
> >>
> >> Signed-off-by: Waiman Long
> >> ---
> >> ipc/ipc_sysctl.c | 7 +--
> >> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
> >> index 8ad93c2..f87cb29 100644
> >> --- a/ipc/ipc_sysctl.c
> >> +++ b/ipc/ipc_sysctl.c
> >> @@ -99,6 +99,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table,
> >> int write,
> >> static int zero;
> >> static int one = 1;
> >> static int int_max = INT_MAX;
> >> +static int ipc_mni = IPCMNI;
> >>
> >> static struct ctl_table ipc_kern_table[] = {
> >>{
> >> @@ -120,7 +121,9 @@ static int proc_ipc_auto_msgmni(struct ctl_table
> >> *table, int write,
> >>.data = &init_ipc_ns.shm_ctlmni,
> >>.maxlen = sizeof(init_ipc_ns.shm_ctlmni),
> >>.mode = 0644,
> >> - .proc_handler = proc_ipc_dointvec,
> >> + .proc_handler = proc_ipc_dointvec_minmax,
> >> + .extra1 = &zero,
> >> + .extra2 = &ipc_mni,
> >>},
> >>{
> >>.procname = "shm_rmid_forced",
> >> @@ -147,7 +150,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table
> >> *table, int write,
> >>.mode = 0644,
> >>.proc_handler = proc_ipc_dointvec_minmax,
> >>.extra1 = &zero,
> >> - .extra2 = &int_max,
> >> + .extra2 = &ipc_mni,
> >>},
> >>{
> >>.procname = "auto_msgmni",
> >> --
> >> 1.8.3.1
> > It seems negative values are not allowed, if true then having
> > a caller to use proc_douintvec_minmax() would help with ensuring
> > no invalid negative input values are used as well.
> >
> > Luis
>
> Negative value doesn't mean sense here. So it is true that we can use
> proc_douintvec_minmax() instead. However, the data types themselves are
> defined as "int". So I think it is better to keep using
> proc_dointvec_minmax() to be consistent with the data type.
Huh, no... If you *know* the valid values *are* only positive, the right
thing to do is to then *change* the data type. Tons of odd bugs can creep
up because of these stupid things.
Luis
Re: [PATCH 13/24] selftests: kmod: return Kselftest Skip code for skipped tests
On Fri, May 04, 2018 at 07:13:17PM -0600, Shuah Khan (Samsung OSG) wrote: > When kmod test is skipped because of unmet dependencies and/or unsupported > configuration, it returns 0 which is treated as a pass by the Kselftest > framework. This leads to false positive result even when the test could > not be run. It returns fail in some cases when test is skipped. Either way, > it is incorrect and incosnistent reporting. > > Change it to return kselftest skip code when a test gets skipped to > clearly report that the test could not be run. > > Kselftest framework SKIP code is 4 and the framework prints appropriate > messages to indicate that the test is skipped. > > Signed-off-by: Shuah Khan (Samsung OSG) Reviewed-by: Luis R. Rodriguez Luis
Re: [PATCH 08/24] selftests: firmware: return Kselftest Skip code for skipped tests
On Fri, May 04, 2018 at 07:13:12PM -0600, Shuah Khan (Samsung OSG) wrote:
> When firmware test(s) get skipped because of unmet dependencies and/or
> unsupported configuration, it returns 0 which is treated as a pass
> by the Kselftest framework. This leads to false positive result even
> when the test could not be run.
>
> Change it to return kselftest skip code when a test gets skipped to
> clearly report that the test could not be run.
>
> Kselftest framework SKIP code is 4 and the framework prints appropriate
> messages to indicate that the test is skipped.
>
> Signed-off-by: Shuah Khan (Samsung OSG)
Reviewed-by: Luis R. Rodriguez
Luis
> ---
> tools/testing/selftests/firmware/fw_fallback.sh | 4 ++--
> tools/testing/selftests/firmware/fw_filesystem.sh | 4 +++-
> tools/testing/selftests/firmware/fw_lib.sh| 7 +--
> 3 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/firmware/fw_fallback.sh
> b/tools/testing/selftests/firmware/fw_fallback.sh
> index 8e2e34a2ca69..70d18be46af5 100755
> --- a/tools/testing/selftests/firmware/fw_fallback.sh
> +++ b/tools/testing/selftests/firmware/fw_fallback.sh
> @@ -74,7 +74,7 @@ load_fw_custom()
> {
> if [ ! -e "$DIR"/trigger_custom_fallback ]; then
> echo "$0: custom fallback trigger not present, ignoring test"
> >&2
> - return 1
> + exit $ksft_skip
> fi
>
> local name="$1"
> @@ -107,7 +107,7 @@ load_fw_custom_cancel()
> {
> if [ ! -e "$DIR"/trigger_custom_fallback ]; then
> echo "$0: canceling custom fallback trigger not present,
> ignoring test" >&2
> - return 1
> + exit $ksft_skip
> fi
>
> local name="$1"
> diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh
> b/tools/testing/selftests/firmware/fw_filesystem.sh
> index 6452d2129cd9..a4320c4b44dc 100755
> --- a/tools/testing/selftests/firmware/fw_filesystem.sh
> +++ b/tools/testing/selftests/firmware/fw_filesystem.sh
> @@ -30,6 +30,7 @@ fi
>
> if [ ! -e "$DIR"/trigger_async_request ]; then
> echo "$0: empty filename: async trigger not present, ignoring test" >&2
> + exit $ksft_skip
> else
> if printf '\000' >"$DIR"/trigger_async_request 2> /dev/null; then
> echo "$0: empty filename should not succeed (async)" >&2
> @@ -69,6 +70,7 @@ fi
> # Try the asynchronous version too
> if [ ! -e "$DIR"/trigger_async_request ]; then
> echo "$0: firmware loading: async trigger not present, ignoring test"
> >&2
> + exit $ksft_skip
> else
> if ! echo -n "$NAME" >"$DIR"/trigger_async_request ; then
> echo "$0: could not trigger async request" >&2
> @@ -89,7 +91,7 @@ test_config_present()
> {
> if [ ! -f $DIR/reset ]; then
> echo "Configuration triggers not present, ignoring test"
> - exit 0
> + exit $ksft_skip
> fi
> }
>
> diff --git a/tools/testing/selftests/firmware/fw_lib.sh
> b/tools/testing/selftests/firmware/fw_lib.sh
> index 962d7f4ac627..6c5f1b2ffb74 100755
> --- a/tools/testing/selftests/firmware/fw_lib.sh
> +++ b/tools/testing/selftests/firmware/fw_lib.sh
> @@ -9,11 +9,14 @@ DIR=/sys/devices/virtual/misc/test_firmware
> PROC_CONFIG="/proc/config.gz"
> TEST_DIR=$(dirname $0)
>
> +# Kselftest framework requirement - SKIP code is 4.
> +ksft_skip=4
> +
> print_reqs_exit()
> {
> echo "You must have the following enabled in your kernel:" >&2
> cat $TEST_DIR/config >&2
> - exit 1
> + exit $ksft_skip
> }
>
> test_modprobe()
> @@ -88,7 +91,7 @@ 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 0
> + exit $ksft_skip
> fi
> fi
> }
> --
> 2.14.1
>
>
--
Do not panic
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Wed, May 09, 2018 at 07:30:28AM -0400, Mimi Zohar wrote: > On Tue, 2018-05-08 at 17:34 +0000, Luis R. Rodriguez wrote: > > On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote: > > > On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote: > > > > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote: > > > > > Allow LSMs and IMA to differentiate between signed regulatory.db and > > > > > other firmware. > > > > > > > > > > Signed-off-by: Mimi Zohar > > > > > Cc: Luis R. Rodriguez > > > > > Cc: David Howells > > > > > Cc: Kees Cook > > > > > Cc: Seth Forshee > > > > > Cc: Johannes Berg > > > > > --- > > > > > drivers/base/firmware_loader/main.c | 5 + > > > > > include/linux/fs.h | 1 + > > > > > 2 files changed, 6 insertions(+) > > > > > > > > > > diff --git a/drivers/base/firmware_loader/main.c > > > > > b/drivers/base/firmware_loader/main.c > > > > > index eb34089e4299..d7cdf04a8681 100644 > > > > > --- a/drivers/base/firmware_loader/main.c > > > > > +++ b/drivers/base/firmware_loader/main.c > > > > > @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device > > > > > *device, struct fw_priv *fw_priv) > > > > > break; > > > > > } > > > > > > > > > > +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB > > > > > + if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) || > > > > > + (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == > > > > > 0)) > > > > > + id = READING_FIRMWARE_REGULATORY_DB; > > > > > +#endif > > > > > > > > Whoa, no way. > > > > > > There are two methods for the kernel to verify firmware signatures. > > > > Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel > > mechanism to verify firmware it uses the request_firmware*() API for > > regulatory.db and regulatory.db.p7s, and IMA already can appraise these two > > files since the firmware API is used. > > IMA-appraisal can verify a signature stored as an xattr, but not a > detached signature. That support could be added, but isn't there > today. Today, a regulatory.db signature would have to be stored as an > xattr. Right, my point was that if someone has IMA installed: a) they would add those xattr to files in /lib/firmware/ already b) upon request_firmware*() calls a security hook would trigger which would enable IMA to appraise those files. So not only would the kernel in turn do double checks on regulatory.db, but also a check on regulatory.db.p7s as well. The difference I suppose is IMA would use a hash function instead of signature check, correct? > > As such I see no reason to add a new ID for them at all. > > K > > Its not providing an *alternative*, its providing an *extra* kernel measure. > > If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own > > stacked LSM. I'd be open to see patches which set that out. May be a > > cleaner interface. > > > > > If both are enabled, do we require both signatures or is one enough. > > > > Good question. Considering it as a stacked LSM (although not implemented > > as one), I'd say its up to who enabled the Kconfig entries. If IMA and > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone > > enabled > > IMA though, then surely I agree that enabling > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to > > the > > system integrator to decide. > > Just because IMA-appraisal is enabled in the kernel doesn't mean that > firmware signatures will be verified. That is a run time policy > decision. Sure, I accept this if IMA does not do signature verification. However signature verification seems like a stackable LSM decision, no? > > If we however want to make it clear that such things as > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we > > could just make the kconfig depend on !IMA or something? Or perhaps a new > > kconfig for IMA which if selected it means that drivers can opt to open code > > *further* kernel signature verification, even though IMA already is > > sufficient. > > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, a
Re: [PATCH] mm: provide a fallback for PAGE_KERNEL_RO for architectures
On Fri, Apr 27, 2018 at 08:18:10PM -0700, Matthew Wilcox wrote: > On Fri, Apr 27, 2018 at 05:15:26PM -0700, Luis R. Rodriguez wrote: > > Some architectures do not define PAGE_KERNEL_RO, best we can do > > for them is to provide a fallback onto PAGE_KERNEL. Remove the > > hack from the firmware loader and move it onto the asm-generic > > header, and document while at it the affected architectures > > which do not have a PAGE_KERNEL_RO: > > > > o alpha > > o ia64 > > o m68k > > o mips > > o sparc64 > > o sparc > > ia64 doesn't have it? > > *fx: riffles through architecture book* > > That seems like an oversight of the Linux port. Tony, Fenghua, any thoughts? Poke *Tony, Fenghua* ? > (also, Luis, maybe move the PAGE_KERNEL_EXEC fallback the same way you > moved the PAGE_KERNEL_RO fallback?) Done. Will queue in the generic PAGE_KERNEL_EXEC patch to my series. > --- >8 --- > > ia64: Add PAGE_KERNEL_RO and PAGE_KERNEL_EXEC > > The rest of the kernel was falling back to simple PAGE_KERNEL pages; using > PAGE_KERNEL_RO and PAGE_KERNEL_EXEC provide better protection against > unintended writes. > > Signed-off-by: Matthew Wilcox Nice, should I queue this into my series as well? Luis
Re: [RFC] vfs: skip extra attributes check on removal for symlinks
On Tue, May 08, 2018 at 05:17:41PM -0700, Darrick J. Wong wrote:
> On Wed, May 09, 2018 at 12:03:28AM +0000, Luis R. Rodriguez wrote:
> > Groovy, thanks, let's not forget the xfs_repair respective fix :) let me
> > know
> > if you have any feedback on that.
>
> TBH I've lost any proposed xfs_repair patches to the mists of time
> because some patch volcano keeps erupting on the lists. :P
>
> Uh... I think it's fine for xfs_{repair,scrub} to clear the immutable
> and append flags on any special inodes it finds, particularly since
> neither flag has any real meaning for block/char/fifo/socket/symlinks
> anyway.
Sure, my point during review of that series for xfs_repair in particular
though was that for symlinks the justification is different (half of the
commit log in this new patch), as such I'd prefer to deal with them in a
separate follow up patch.
Luis
Re: [RFC] vfs: skip extra attributes check on removal for symlinks
On Mon, May 07, 2018 at 05:30:55PM -0700, Darrick J. Wong wrote: > On Tue, May 01, 2018 at 05:45:12PM +0000, Luis R. Rodriguez wrote: > > On Tue, May 01, 2018 at 10:23:19AM -0700, Darrick J. Wong wrote: > > > On Thu, Apr 26, 2018 at 04:46:39PM -0700, Luis R. Rodriguez wrote: > > > > Linux filesystems cannot set extra file attributes (stx_attributes as > > > > per > > > > statx(2)) on a symbolic link. To set extra file attributes you issue > > > > ioctl(2) with FS_IOC_SETFLAGS, *all* ioctl(2) calls on a symbolic link > > > > yield EBADF. > > > > > > > > This is because ioctl(2) tries to obtain struct fd from the symbolic > > > > link > > > > file descriptor passed using fdget(), fdget() in turn always returns no > > > > file set when a file descriptor is open with O_PATH. As per symlink(2) > > > > O_PATH and O_NOFOLLOW must *always* be used when you want to get the > > > > file > > > > descriptor of a symbolic link, and this holds true for Linux, as such > > > > extra > > > > file attributes cannot possibly be set on symbolic links on Linux. > > > > > > > > Filesystems repair utilities should be updated to detect this as > > > > corruption and correct this, however, the VFS *does* respect these > > > > extra attributes on symlinks for removal. > > > > > > > > Since we cannot set these attributes we should special-case the > > > > immutable/append on delete for symlinks, this would be consistent with > > > > what we *do* allow on Linux for all filesystems. > > > > > > Ah, ok, so the problem here is that you can't rm an "immutable" symlink > > > nor can you clear the immutable flag on such a beast, so therefore > > > ignore the immutable (and append) flags if we're trying to delete a > > > symlink? > > > > Yup. > > > > > I think we ought to teach the xfs inode verifier to check for > > > immutable/append symlinks and return error so that we don't end up with > > > such things in core in the first place, > > > > Agreed. But note that one way to end up with these things is through > > corruption. Once a user finds this the first signs they'll run into > > (unless they have the awesome new online scrubber) is they cannot delete > > some odd file and not know why. And then they'll see they cannot change > > or remove either the immutable or append flag. The immutable attribute > > is known to not let you delete the file, but its less known that the > > append attribute implies the same. Folks would scratch their heads. > > > > Since one cannot *set* these attributes on symlinks on Linux, other than > > ignoring such attributes perhaps we should warn about it? As the only > > way you could end up with that is if your filesystem got corrupted. > > > > But without filesystems having a fix for that merged users can't do > > anything. > > > > > and fix xfs_repair to zap such things. > > > > This is what my patch for xfs_repair does, which is pending review. > > > > But note that special files are handled differently, I explain the logic on > > the > > RFC commit log, which is why I suggested splitting up adding a fix for > > special > > files as a separate patch. > > > > > That said, for the filesystems that aren't going to check their inodes, > > > I guess this is a (hackish) way to avoid presenting undeletable gunk in > > > the fs to the user... > > > > That's why its RFC. What is the right thing to do here? > > Ideally, none of the filesystems should ever feed garbage to the vfs, > but it's not all that obvious exactly what things the vfs will trip over. > And knowing that a lot of the filesystems do minimal checking if any, I > guess I'll just say that... > > ...XFS (and probably ext4) should catch immutable symlinks in the inode > verifiers so that xfs_iget returns -EFSCORRUPTED. For the rest of the > filesystems, it's probably fine to let them delete the symlink since the > user wants to kill it anyway. Alrighty, I have this implemented now. > > My own logic here was since we cannot possibly allow extended attributes on > > symlinks is to not use them then as well, in this case for delete. > > > > > (Were it up to me I'd make a common vfs_check_inode() to reject > > > struct inode containing garbage that the vfs won't deal with, > > > > There certainly are cas
[PATCH v6 01/13] firmware: wrap FW_OPT_* into an enum
From: Andres Rodriguez
This should let us associate enum kdoc to these values.
While at it, kdocify the fw_opt.
Signed-off-by: Andres Rodriguez
Acked-by: Luis R. Rodriguez
[mcgrof: coding style fixes, merge kdoc with enum move]
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_loader/fallback.c | 12
drivers/base/firmware_loader/fallback.h | 6 ++--
drivers/base/firmware_loader/firmware.h | 37 +++--
drivers/base/firmware_loader/main.c | 6 ++--
4 files changed, 42 insertions(+), 19 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..b57a7b3b4122 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -512,7 +512,7 @@ static const struct attribute_group *fw_dev_attr_groups[] =
{
static struct fw_sysfs *
fw_create_instance(struct firmware *firmware, const char *fw_name,
- struct device *device, unsigned int opt_flags)
+ struct device *device, enum fw_opt opt_flags)
{
struct fw_sysfs *fw_sysfs;
struct device *f_dev;
@@ -545,7 +545,7 @@ fw_create_instance(struct firmware *firmware, const char
*fw_name,
* In charge of constructing a sysfs fallback interface for firmware loading.
**/
static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
- unsigned int opt_flags, long timeout)
+ enum fw_opt opt_flags, long timeout)
{
int retval = 0;
struct device *f_dev = &fw_sysfs->dev;
@@ -599,7 +599,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
static int fw_load_from_user_helper(struct firmware *firmware,
const char *name, struct device *device,
- unsigned int opt_flags)
+ enum fw_opt opt_flags)
{
struct fw_sysfs *fw_sysfs;
long timeout;
@@ -640,7 +640,7 @@ static int fw_load_from_user_helper(struct firmware
*firmware,
return ret;
}
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
{
if (fw_fallback_config.force_sysfs_fallback)
return true;
@@ -649,7 +649,7 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
return true;
}
-static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
{
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl
knob\n");
@@ -664,7 +664,7 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
int ret)
{
if (!fw_run_sysfs_fallback(opt_flags))
diff --git a/drivers/base/firmware_loader/fallback.h
b/drivers/base/firmware_loader/fallback.h
index f8255670a663..a3b73a09db6c 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -5,6 +5,8 @@
#include
#include
+#include "firmware.h"
+
/**
* struct firmware_fallback_config - firmware fallback configuration settings
*
@@ -31,7 +33,7 @@ struct firmware_fallback_config {
#ifdef CONFIG_FW_LOADER_USER_HELPER
int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
int ret);
void kill_pending_fw_fallback_reqs(bool only_kill_custom);
@@ -43,7 +45,7 @@ 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,
+ enum fw_opt opt_flags,
int ret)
{
/* Keep carrying over the same error */
diff --git a/drivers/base/firmware_loader/firmware.h
b/drivers/base/firmware_loader/firmware.h
index 64acbb1a392c..4f433b447367 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -2,6 +2,7 @@
#ifndef __FIRMWARE_LOADER_H
#define __FIRMWARE_LOADER_H
+#include
#include
#include
#include
@@ -10,13 +11,33 @@
#include
-/* 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)
-#defi
[PATCH v6 02/13] firmware: use () to terminate kernel-doc function names
From: Andres Rodriguez The kernel-doc spec dictates a function name ends in (). Signed-off-by: Andres Rodriguez Acked-by: Randy Dunlap Acked-by: Luis R. Rodriguez [mcgrof: adjust since the wide API rename is not yet merged] Signed-off-by: Luis R. Rodriguez --- drivers/base/firmware_loader/fallback.c | 8 drivers/base/firmware_loader/main.c | 22 +++--- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index b57a7b3b4122..529f7013616f 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -125,7 +125,7 @@ static ssize_t timeout_show(struct class *class, struct class_attribute *attr, } /** - * firmware_timeout_store - set number of seconds to wait for firmware + * 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 @@ -239,7 +239,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv) } /** - * firmware_loading_store - set value in the 'loading' control file + * 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 @@ -431,7 +431,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int min_size) } /** - * firmware_data_write - write method for firmware + * firmware_data_write() - write method for firmware * @filp: open sysfs file * @kobj: kobject for the device * @bin_attr: bin_attr structure @@ -537,7 +537,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name, } /** - * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism + * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism * @fw_sysfs: firmware sysfs information for the firmware to load * @opt_flags: flags of options, FW_OPT_* * @timeout: timeout to wait for the load diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index 9919f0e6a7cc..4d11efadb3a4 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -597,7 +597,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name, } /** - * request_firmware: - send firmware request and wait for it + * request_firmware() - send firmware request and wait for it * @firmware_p: pointer to firmware image * @name: name of firmware file * @device: device for which firmware is being loaded @@ -632,7 +632,7 @@ request_firmware(const struct firmware **firmware_p, const char *name, EXPORT_SYMBOL(request_firmware); /** - * request_firmware_direct: - load firmware directly without usermode helper + * request_firmware_direct() - load firmware directly without usermode helper * @firmware_p: pointer to firmware image * @name: name of firmware file * @device: device for which firmware is being loaded @@ -657,7 +657,7 @@ int request_firmware_direct(const struct firmware **firmware_p, EXPORT_SYMBOL_GPL(request_firmware_direct); /** - * firmware_request_cache: - cache firmware for suspend so resume can use it + * firmware_request_cache() - cache firmware for suspend so resume can use it * @name: name of firmware file * @device: device for which firmware should be cached for * @@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const char *name) EXPORT_SYMBOL_GPL(firmware_request_cache); /** - * request_firmware_into_buf - load firmware into a previously allocated buffer + * request_firmware_into_buf() - load firmware into a previously allocated buffer * @firmware_p: pointer to firmware image * @name: name of firmware file * @device: device for which firmware is being loaded and DMA region allocated @@ -713,7 +713,7 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name, EXPORT_SYMBOL(request_firmware_into_buf); /** - * release_firmware: - release the resource associated with a firmware image + * release_firmware() - release the resource associated with a firmware image * @fw: firmware resource to release **/ void release_firmware(const struct firmware *fw) @@ -755,7 +755,7 @@ static void request_firmware_work_func(struct work_struct *work) } /** - * request_firmware_nowait - asynchronous version of request_firmware + * request_firmware_nowait() - asynchronous version of request_firmware * @module: module requesting the firmware * @uevent: sends uevent to copy the firmware image if this flag * is non-zero else the firmware copy must be done manually. @@ -824,7 +824,7 @@ EXPORT_SYMBOL(request_firmware_nowait); static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain); /** - * cache_firmware - cache one firmware image in kernel memory space + * cache_firmware() - cache one firmware im
[PATCH v6 04/13] firmware_loader: document firmware_sysfs_fallback()
This also sets the expecations for future fallback interfaces, even if they are not exported. Signed-off-by: Luis R. Rodriguez --- drivers/base/firmware_loader/fallback.c | 20 1 file changed, 20 insertions(+) diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index 3db9e0f225ac..9169e7b9800c 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -662,6 +662,26 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags) return fw_force_sysfs_fallback(opt_flags); } +/** + * firmware_fallback_sysfs() - use the fallback mechanism to find firmware + * @fw: pointer to firmware image + * @name: name of firmware file to look for + * @device: device for which firmware is being loaded + * @opt_flags: options to control firmware loading behaviour + * @ret: return value from direct lookup which triggered the fallback mechanism + * + * This function is called if direct lookup for the firmware failed, it enables + * a fallback mechanism through userspace by exposing a sysfs loading + * interface. Userspace is in charge of loading the firmware through the syfs + * loading interface. This syfs fallback mechanism may be disabled completely + * on a system by setting the proc sysctl value ignore_sysfs_fallback to true. + * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK + * flag, if so it would also disable the fallback mechanism. A system may want + * to enfoce the sysfs fallback mechanism at all times, it can do this by + * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true. + * Enabling force_sysfs_fallback is functionally equivalent to build a kernel + * with CONFIG_FW_LOADER_USER_HELPER_FALLBACK. + **/ int firmware_fallback_sysfs(struct firmware *fw, const char *name, struct device *device, enum fw_opt opt_flags, -- 2.17.0
[PATCH v6 05/13] firmware_loader: enhance Kconfig documentation over FW_LOADER
If you try to read FW_LOADER today it speaks of old riddles and unless you have been following development closely you will loose track of what is what. Even the documentation for PREVENT_FIRMWARE_BUILD is a bit fuzzy and how it fits into this big picture. Give the FW_LOADER kconfig documentation some love with more up to date developments and recommendations. While at it, wrap the FW_LOADER code into its own menu to compartamentalize and make it clearer which components really are part of the FW_LOADER. This should also make it easier to later move these kconfig entries into the firmware_loader/ directory later. This also now recommends using firmwared [0] for folks left needing a uevent handler in userspace for the sysfs firmware fallback mechanis given udev's uevent firmware mechanism was ripped out a while ago. [0] https://github.com/teg/firmwared Signed-off-by: Luis R. Rodriguez --- drivers/base/Kconfig | 165 ++- 1 file changed, 131 insertions(+), 34 deletions(-) diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 29b0eb452b3a..a4fe86caecca 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -70,39 +70,64 @@ config STANDALONE If unsure, say Y. config PREVENT_FIRMWARE_BUILD - bool "Prevent firmware from being built" + bool "Disable drivers features which enable custom firmware building" default y help - Say yes to avoid building firmware. Firmware is usually shipped - with the driver and only when updating the firmware should a - rebuild be made. - If unsure, say Y here. + Say yes to disable driver features which enable building a custom + driver firmwar at kernel build time. These drivers do not use the + kernel firmware API to load firmware (CONFIG_FW_LOADER), instead they + use their own custom loading mechanism. The required firmware is + usually shipped with the driver, building the driver firmware + should only be needed if you have an updated firmware source. + + Firmware should not be being built as part of kernel, these days + you should always prevent this and say Y here. There are only two + old drivers which enable building of its firmware at kernel build + time: + + o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE + o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE + +menu "Firmware loader" config FW_LOADER - tristate "Userspace firmware loading support" if EXPERT + tristate "Firmware loading facility" if EXPERT default y ---help--- - This option is provided for the case where none of the in-tree modules - require userspace firmware loading support, but a module built - out-of-tree does. + This enables the firmware loading facility in the kernel. The kernel + will first look for built-in firmware, if it has any. Next, it will + look for the requested firmware in a series of filesystem paths: + + o firmware_class path module parameter or kernel boot param + o /lib/firmware/updates/UTS_RELEASE + o /lib/firmware/updates + o /lib/firmware/UTS_RELEASE + o /lib/firmware + + Enabling this feature only increases your kernel image by about + 828 bytes, enable this option unless you are certain you don't + need firmware. + + You typically want this built-in (=y) but you can also enable this + as a module, in which case the firmware_class module will be built. + You also want to be sure to enable this built-in if you are going to + enable built-in firmware (CONFIG_EXTRA_FIRMWARE). + +if FW_LOADER config EXTRA_FIRMWARE - string "External firmware blobs to build into the kernel binary" - depends on FW_LOADER + string "Build these firmware blobs into the kernel binary" help - Various drivers in the kernel source tree may require firmware, - which is generally available in your distribution's linux-firmware - package. + Device drivers which require firmware can typically deal with + having the kernel load firmware from the various supported + /lib/firmware/ paths. This option enables you to build into the + kernel firmware files. Built-in firmware searches are preceeded + over firmware lookups using your filesystem over the supported + /lib/firmware paths documented on CONFIG_FW_LOADER. - The linux-firmware package should install firmware into - /lib/firmware/ on your system, so they can be loaded by userspace - helpers on request. - - This option allows firmware to be built into the kernel for the case - where the user either
[PATCH v2] mm: expland documentation over __read_mostly
__read_mostly can easily be misused by folks, its not meant for just read-only data. There are performance reasons for using it, but we also don't provide any guidance about its use. Provide a bit more guidance over it use. Acked-by: Christoph Lameter Signed-off-by: Luis R. Rodriguez --- include/linux/cache.h | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/include/linux/cache.h b/include/linux/cache.h index 750621e41d1c..4967566ed08c 100644 --- a/include/linux/cache.h +++ b/include/linux/cache.h @@ -15,8 +15,16 @@ /* * __read_mostly is used to keep rarely changing variables out of frequently - * updated cachelines. If an architecture doesn't support it, ignore the - * hint. + * updated cachelines. Its use should be reserved for data that is used + * frequently in hot paths. Performance traces can help decide when to use + * this. You want __read_mostly data to be tightly packed, so that in the + * best case multiple frequently read variables for a hot path will be next + * to each other in order to reduce the number of cachelines needed to + * execute a critial path. We should be mindful and selective of its use. + * ie: if you're going to use it please supply a *good* justification in your + * commit log. + * + * If an architecture doesn't support it, ignore the hint. */ #ifndef __read_mostly #define __read_mostly -- 2.17.0
[PATCH v6 06/13] firmware_loader: move kconfig FW_LOADER entries to its own file
This will make it easier to track and easier to understand
what components and features are part of the FW_LOADER. There
are some components related to firmware which have *nothing* to
do with the FW_LOADER, souch as PREVENT_FIRMWARE_BUILD.
Signed-off-by: Luis R. Rodriguez
---
drivers/base/Kconfig | 155 +--
drivers/base/firmware_loader/Kconfig | 154 ++
2 files changed, 155 insertions(+), 154 deletions(-)
create mode 100644 drivers/base/firmware_loader/Kconfig
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index a4fe86caecca..06d6e27784fa 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -88,160 +88,7 @@ config PREVENT_FIRMWARE_BUILD
o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
-menu "Firmware loader"
-
-config FW_LOADER
- tristate "Firmware loading facility" if EXPERT
- default y
- ---help---
- This enables the firmware loading facility in the kernel. The kernel
- will first look for built-in firmware, if it has any. Next, it will
- look for the requested firmware in a series of filesystem paths:
-
- o firmware_class path module parameter or kernel boot param
- o /lib/firmware/updates/UTS_RELEASE
- o /lib/firmware/updates
- o /lib/firmware/UTS_RELEASE
- o /lib/firmware
-
- Enabling this feature only increases your kernel image by about
- 828 bytes, enable this option unless you are certain you don't
- need firmware.
-
- You typically want this built-in (=y) but you can also enable this
- as a module, in which case the firmware_class module will be built.
- You also want to be sure to enable this built-in if you are going to
- enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
-
-if FW_LOADER
-
-config EXTRA_FIRMWARE
- string "Build these firmware blobs into the kernel binary"
- help
- Device drivers which require firmware can typically deal with
- having the kernel load firmware from the various supported
- /lib/firmware/ paths. This option enables you to build into the
- kernel firmware files. Built-in firmware searches are preceeded
- over firmware lookups using your filesystem over the supported
- /lib/firmware paths documented on CONFIG_FW_LOADER.
-
- This may be useful for testing or if the firmware is required early on
- in boot and cannot rely on the firmware being placed in an initrd or
- initramfs.
-
- This option is a string and takes the (space-separated) names of the
- firmware files -- the same names that appear in MODULE_FIRMWARE()
- and request_firmware() in the source. These files should exist under
- the directory specified by the EXTRA_FIRMWARE_DIR option, which is
- /lib/firmware by default.
-
- For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
- the usb8388.bin file into /lib/firmware, and build the kernel. Then
- any request_firmware("usb8388.bin") will be satisfied internally
- inside the kernel without ever looking at your filesystem at runtime.
-
- WARNING: If you include additional firmware files into your binary
- kernel image that are not available under the terms of the GPL,
- then it may be a violation of the GPL to distribute the resulting
- image since it combines both GPL and non-GPL work. You should
- consult a lawyer of your own before distributing such an image.
-
-config EXTRA_FIRMWARE_DIR
- string "Firmware blobs root directory"
- depends on EXTRA_FIRMWARE != ""
- default "/lib/firmware"
- help
- This option controls the directory in which the kernel build system
- looks for the firmware files listed in the EXTRA_FIRMWARE option.
-
-config FW_LOADER_USER_HELPER
- bool "Enable the firmware sysfs fallback mechanism"
- help
- This option enables a sysfs loading facility to enable firmware
- loading to the kernel through userspace as a fallback mechanism
- if and only if the kernel's direct filesystem lookup for the
- firmware failed using the different /lib/firmware/ paths, or the
- path specified in the firmware_class path module parameter, or the
- firmware_class path kernel boot parameter if the firmware_class is
- built-in. For details on how to work with the sysfs fallback mechanism
- refer to Documentation/driver-api/firmware/fallback-mechanisms.rst.
-
- The direct filesystem lookup for firwmare is always used first now.
-
- If the kernel's direct filesystem lo
Re: [PATCH] mm: expland documentation over __read_mostly
On Tue, May 08, 2018 at 08:39:30AM -0700, Randy Dunlap wrote: > On 05/08/2018 04:23 AM, Matthew Wilcox wrote: > > On Tue, May 08, 2018 at 09:28:14AM +0100, David Howells wrote: > >> Randy Dunlap wrote: > >> > + * execute a critial path. We should be mindful and selective if its > use. > >>> > >>> of its > >>> use. > >> > >>in its > >> use. > > with its > > use. > > > > Nah, just kidding. Let's go with "in". > > > > Yeah, no, I don't care. Just flip a 3-sided coin. Heh, the coin says "of". I also added: * ie: if you're going to use it please supply a *good* justification in your * commit log. Sending v2. Luis
[PATCH v6 09/13] ath10k: use firmware_request_nowarn() to load firmware
From: Andres Rodriguez This reduces the unnecessary spew when trying to load optional firmware: "Direct firmware load for ... failed with error -2" Signed-off-by: Andres Rodriguez Acked-by: Kalle Valo Signed-off-by: Luis R. Rodriguez --- drivers/net/wireless/ath/ath10k/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index 4cf54a7ef09a..ad4f6e3c0737 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -694,7 +694,7 @@ static const struct firmware *ath10k_fetch_fw_file(struct ath10k *ar, dir = "."; snprintf(filename, sizeof(filename), "%s/%s", dir, file); - ret = request_firmware(&fw, filename, ar->dev); + ret = firmware_request_nowarn(&fw, filename, ar->dev); ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot fw request '%s': %d\n", filename, ret); -- 2.17.0
[PATCH v6 10/13] ath10k: re-enable the firmware fallback mechanism for testmode
From: Andres Rodriguez The ath10k testmode uses request_firmware_direct() in order to avoid producing firmware load warnings. Disabling the fallback mechanism was a side effect of disabling warnings. We now have a new API that allows us to avoid warnings while keeping the fallback mechanism enabled. So use that instead. Signed-off-by: Andres Rodriguez Acked-by: Kalle Valo Signed-off-by: Luis R. Rodriguez --- drivers/net/wireless/ath/ath10k/testmode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath10k/testmode.c b/drivers/net/wireless/ath/ath10k/testmode.c index 568810b41657..c24ee616833c 100644 --- a/drivers/net/wireless/ath/ath10k/testmode.c +++ b/drivers/net/wireless/ath/ath10k/testmode.c @@ -157,7 +157,7 @@ static int ath10k_tm_fetch_utf_firmware_api_1(struct ath10k *ar, ar->hw_params.fw.dir, ATH10K_FW_UTF_FILE); /* load utf firmware image */ - ret = request_firmware_direct(&fw_file->firmware, filename, ar->dev); + ret = firmware_request_nowarn(&fw_file->firmware, filename, ar->dev); ath10k_dbg(ar, ATH10K_DBG_TESTMODE, "testmode fw request '%s': %d\n", filename, ret); -- 2.17.0
[PATCH v6 07/13] firmware_loader: make firmware_fallback_sysfs() print more useful
If we resort to using the sysfs fallback mechanism we don't print the filename. This can be deceiving given we could have a series of callers intertwined and it'd be unclear exactly for what firmware this was meant for. Additionally, although we don't currently use FW_OPT_NO_WARN when dealing with the fallback mechanism, we will soon, so just respect its use consistently. And even if you *don't* want to print always on failure, you may want to print when debugging so enable dynamic debug print when FW_OPT_NO_WARN is used. Signed-off-by: Luis R. Rodriguez --- drivers/base/firmware_loader/fallback.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index 9169e7b9800c..b676a99c469c 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -690,6 +690,11 @@ int firmware_fallback_sysfs(struct firmware *fw, const char *name, if (!fw_run_sysfs_fallback(opt_flags)) return ret; - dev_warn(device, "Falling back to user helper\n"); + if (!(opt_flags & FW_OPT_NO_WARN)) + dev_warn(device, "Falling back to syfs fallback for: %s\n", +name); + else + dev_dbg(device, "Falling back to sysfs fallback for: %s\n", + name); return fw_load_from_user_helper(fw, name, device, opt_flags); } -- 2.17.0
[PATCH v6 11/13] Documentation: fix few typos and clarifications for the firmware loader
Fix a few typos, and clarify a few sentences. Signed-off-by: Luis R. Rodriguez --- Documentation/driver-api/firmware/fallback-mechanisms.rst | 5 +++-- Documentation/driver-api/firmware/firmware_cache.rst | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst index f353783ae0be..a39323ef7d29 100644 --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst @@ -83,7 +83,7 @@ and a file upload firmware into: * /sys/$DEVPATH/data To upload firmware you will echo 1 onto the loading file to indicate -you are loading firmware. You then cat the firmware into the data file, +you are loading firmware. You then write the firmware into the data file, and you notify the kernel the firmware is ready by echo'ing 0 onto the loading file. @@ -136,7 +136,8 @@ by kobject uevents. This is specially exacerbated due to the fact that most distributions today disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK. Refer to do_firmware_uevent() for details of the kobject event variables -setup. Variables passwdd with a kobject add event: +setup. The variables currently passed to userspace with a "kobject add" +event are: * FIRMWARE=firmware name * TIMEOUT=timeout value diff --git a/Documentation/driver-api/firmware/firmware_cache.rst b/Documentation/driver-api/firmware/firmware_cache.rst index 2210e5bfb332..c2e69d9c6bf1 100644 --- a/Documentation/driver-api/firmware/firmware_cache.rst +++ b/Documentation/driver-api/firmware/firmware_cache.rst @@ -29,8 +29,8 @@ Some implementation details about the firmware cache setup: * If an asynchronous call is used the firmware cache is only set up for a device if if the second argument (uevent) to request_firmware_nowait() is true. When uevent is true it requests that a kobject uevent be sent to - userspace for the firmware request. For details refer to the Fackback - mechanism documented below. + userspace for the firmware request through the sysfs fallback mechanism + if the firmware file is not found. * If the firmware cache is determined to be needed as per the above two criteria the firmware cache is setup by adding a devres entry for the -- 2.17.0
[PATCH v6 08/13] firmware: add firmware_request_nowarn() - load firmware without warnings
From: Andres Rodriguez
Currently the firmware loader only exposes one silent path for querying
optional firmware, and that is firmware_request_direct(). This function
also disables the sysfs fallback mechanism, which might not always be the
desired behaviour [0].
This patch introduces a variations of request_firmware() that enable the
caller to disable the undesired warning messages but enables the sysfs
fallback mechanism. This is equivalent to adding FW_OPT_NO_WARN to the
old behaviour.
[0]: https://git.kernel.org/linus/c0cc00f250e1
Signed-off-by: Andres Rodriguez
Acked-by: Luis R. Rodriguez
[mcgrof: used the old API calls as the full rename is not done yet, and
add the caller for when FW_LOADER is disabled, enhance documentation ]
Signed-off-by: Luis R. Rodriguez
---
.../driver-api/firmware/request_firmware.rst | 5
drivers/base/firmware_loader/main.c | 27 +++
include/linux/firmware.h | 10 +++
3 files changed, 42 insertions(+)
diff --git a/Documentation/driver-api/firmware/request_firmware.rst
b/Documentation/driver-api/firmware/request_firmware.rst
index d5ec95a7195b..f62bdcbfed5b 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -20,6 +20,11 @@ request_firmware
.. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: request_firmware
+firmware_request_nowarn
+---
+.. kernel-doc:: drivers/base/firmware_loader/main.c
+ :functions: firmware_request_nowarn
+
request_firmware_direct
---
.. kernel-doc:: drivers/base/firmware_loader/main.c
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index abdc4e4d55f1..2be79ee773c0 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -631,6 +631,33 @@ request_firmware(const struct firmware **firmware_p, const
char *name,
}
EXPORT_SYMBOL(request_firmware);
+/**
+ * firmware_request_nowarn() - request for an optional fw module
+ * @firmware: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ * The sysfs fallback mechanism is enabled if direct filesystem lookup fails,
+ * however, however failures to find the firmware file with it are still
+ * supressed. It is therefore up to the driver to check for the return value
+ * of this call and to decide when to inform the users of errors.
+ **/
+int firmware_request_nowarn(const struct firmware **firmware, const char *name,
+ struct device *device)
+{
+ int ret;
+
+ /* Need to pin this module until return */
+ __module_get(THIS_MODULE);
+ ret = _request_firmware(firmware, name, device, NULL, 0,
+ FW_OPT_UEVENT | FW_OPT_NO_WARN);
+ module_put(THIS_MODULE);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(firmware_request_nowarn);
+
/**
* request_firmware_direct() - load firmware directly without usermode helper
* @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 41050417cafb..2dd566c91d44 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -42,6 +42,8 @@ struct builtin_fw {
#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) &&
defined(MODULE))
int request_firmware(const struct firmware **fw, const char *name,
struct device *device);
+int firmware_request_nowarn(const struct firmware **fw, const char *name,
+ struct device *device);
int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
@@ -59,6 +61,14 @@ static inline int request_firmware(const struct firmware
**fw,
{
return -EINVAL;
}
+
+static inline int firmware_request_nowarn(const struct firmware **fw,
+ const char *name,
+ struct device *device)
+{
+ return -EINVAL;
+}
+
static inline int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
--
2.17.0
[PATCH v6 12/13] Documentation: remove stale firmware API reference
It refers to a pending patch, but this was merged eons ago. Signed-off-by: Luis R. Rodriguez --- Documentation/dell_rbu.txt | 3 --- 1 file changed, 3 deletions(-) diff --git a/Documentation/dell_rbu.txt b/Documentation/dell_rbu.txt index 0fdb6aa2704c..077fdc29a0d0 100644 --- a/Documentation/dell_rbu.txt +++ b/Documentation/dell_rbu.txt @@ -121,9 +121,6 @@ read back the image downloaded. .. note:: - This driver requires a patch for firmware_class.c which has the modified - request_firmware_nowait function. - Also after updating the BIOS image a user mode application needs to execute code which sends the BIOS update request to the BIOS. So on the next reboot the BIOS knows about the new image downloaded and it updates itself. -- 2.17.0
[PATCH v6 03/13] firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()
From: Andres Rodriguez
This is done since this call is now exposed through kernel-doc,
and since this also paves the way for different future types of
fallback mechanims.
Signed-off-by: Andres Rodriguez
Acked-by: Luis R. Rodriguez
[mcgrof: small coding style changes]
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_loader/fallback.c | 8
drivers/base/firmware_loader/fallback.h | 16
drivers/base/firmware_loader/firmware.h | 2 +-
drivers/base/firmware_loader/main.c | 2 +-
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 529f7013616f..3db9e0f225ac 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,10 +662,10 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
}
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret)
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret)
{
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
diff --git a/drivers/base/firmware_loader/fallback.h
b/drivers/base/firmware_loader/fallback.h
index a3b73a09db6c..21063503e4ea 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -31,10 +31,10 @@ struct firmware_fallback_config {
};
#ifdef CONFIG_FW_LOADER_USER_HELPER
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret);
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret);
void kill_pending_fw_fallback_reqs(bool only_kill_custom);
void fw_fallback_set_cache_timeout(void);
@@ -43,10 +43,10 @@ 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,
- enum fw_opt opt_flags,
- int ret)
+static inline int firmware_fallback_sysfs(struct firmware *fw, const char
*name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret)
{
/* Keep carrying over the same error */
return ret;
diff --git a/drivers/base/firmware_loader/firmware.h
b/drivers/base/firmware_loader/firmware.h
index 4f433b447367..4c1395f8e7ed 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -20,7 +20,7 @@
* @FW_OPT_NOWAIT: Used to describe the firmware request is asynchronous.
* @FW_OPT_USERHELPER: Enable the fallback mechanism, in case the direct
* filesystem lookup fails at finding the firmware. For details refer to
- * fw_sysfs_fallback().
+ * firmware_fallback_sysfs().
* @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
* @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
* cache the firmware upon suspend, so that upon resume races against the
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index 4d11efadb3a4..abdc4e4d55f1 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -581,7 +581,7 @@ _request_firmware(const struct firmware **firmware_p, const
char *name,
dev_warn(device,
"Direct firmware load for %s failed with error
%d\n",
name, ret);
- ret = fw_sysfs_fallback(fw, name, device, opt_flags, ret);
+ ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
} else
ret = assign_fw(fw, device, opt_flags);
--
2.17.0
[PATCH v6 00/13] firmware_loader changes for v4.18
Greg, Here is what I have queued up for the firmware_loader for v4.18. It includes a slew of cleanup work, and the new firmware_request_nowarn() which is quiet but enables the sysfs fallback mechanism. I've gone ahead and also queued up a few minor fixes for the firmware loader documentation which have come up recently. These changes are available on my git tree both based on linux-next [0] and Linus' latest tree [1]. Folks working on new developments for the firmware loader can use my linux-next branch 20180508-firmware_loader_for-v4.18-try2 for now. 0-day sends its blessings. The patches from Mimi's series still require a bit more discussion and review. The discussion over the EFI firmware fallback mechanism is still ongoing. As for the rename that you wanted, perhaps we can do this late in the merge window considering we're at rc4 now. I can prep something up for that later. Question, and specially rants are warmly welcomed. [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180508-firmware_loader_for-v4.18-try2 [1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180508-firmware_loader_for-v4.18-try2 Andres Rodriguez (6): firmware: wrap FW_OPT_* into an enum firmware: use () to terminate kernel-doc function names firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs() firmware: add firmware_request_nowarn() - load firmware without warnings ath10k: use firmware_request_nowarn() to load firmware ath10k: re-enable the firmware fallback mechanism for testmode Luis R. Rodriguez (7): firmware_loader: document firmware_sysfs_fallback() firmware_loader: enhance Kconfig documentation over FW_LOADER firmware_loader: move kconfig FW_LOADER entries to its own file firmware_loader: make firmware_fallback_sysfs() print more useful Documentation: fix few typos and clarifications for the firmware loader Documentation: remove stale firmware API reference Documentation: clarify firmware_class provenance and why we can't rename the module Documentation/dell_rbu.txt| 3 - .../firmware/fallback-mechanisms.rst | 14 +- .../driver-api/firmware/firmware_cache.rst| 4 +- .../driver-api/firmware/request_firmware.rst | 5 + drivers/base/Kconfig | 90 ++ drivers/base/firmware_loader/Kconfig | 154 ++ drivers/base/firmware_loader/fallback.c | 53 -- drivers/base/firmware_loader/fallback.h | 18 +- drivers/base/firmware_loader/firmware.h | 37 - drivers/base/firmware_loader/main.c | 57 +-- drivers/net/wireless/ath/ath10k/core.c| 2 +- drivers/net/wireless/ath/ath10k/testmode.c| 2 +- include/linux/firmware.h | 10 ++ 13 files changed, 319 insertions(+), 130 deletions(-) create mode 100644 drivers/base/firmware_loader/Kconfig -- 2.17.0
[PATCH v6 13/13] Documentation: clarify firmware_class provenance and why we can't rename the module
Clarify the provenance of the firmware loader firmware_class module name and why we cannot rename the module in the future. Signed-off-by: Luis R. Rodriguez --- .../driver-api/firmware/fallback-mechanisms.rst | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst index a39323ef7d29..a8047be4a96e 100644 --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst @@ -72,9 +72,12 @@ the firmware requested, and establishes it in the device hierarchy by associating the device used to make the request as the device's parent. The sysfs directory's file attributes are defined and controlled through the new device's class (firmware_class) and group (fw_dev_attr_groups). -This is actually where the original firmware_class.c file name comes from, -as originally the only firmware loading mechanism available was the -mechanism we now use as a fallback mechanism. +This is actually where the original firmware_class module name came from, +given that originally the only firmware loading mechanism available was the +mechanism we now use as a fallback mechanism, which which registers a +struct class firmware_class. Because the attributes exposed are part of the +module name, the module name firmware_class cannot be renamed in the future, to +ensure backward compatibilty with old userspace. To load firmware using the sysfs interface we expose a loading indicator, and a file upload firmware into: -- 2.17.0
Re: [PATCH 00/18] Fix some build warnings/errors with Sphinx
On Tue, May 08, 2018 at 10:13:42AM -0600, Jonathan Corbet wrote: > On Mon, 7 May 2018 06:35:36 -0300 > Mauro Carvalho Chehab wrote: > > > I decided to give a try with Sphinx last stable version > > (1.17.4), and noticed several issues. The worse one was > > with the networking book: a non-standard footnote there > > with [*] instead of a number causes it to break PDF building. > > > > So, I took some spare time to address some warnings all over > > the tree, and moved a few text documents to a book. > > OK, I've applied the ones that seem to make sense for me to take now. > There's comments on the firmware one, I'll fold in the fixes for the firmware API which do apply to my queue. Luis
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote: > On Fri, 2018-05-04 at 00:07 +0000, Luis R. Rodriguez wrote: > > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote: > > > Allow LSMs and IMA to differentiate between signed regulatory.db and > > > other firmware. > > > > > > Signed-off-by: Mimi Zohar > > > Cc: Luis R. Rodriguez > > > Cc: David Howells > > > Cc: Kees Cook > > > Cc: Seth Forshee > > > Cc: Johannes Berg > > > --- > > > drivers/base/firmware_loader/main.c | 5 + > > > include/linux/fs.h | 1 + > > > 2 files changed, 6 insertions(+) > > > > > > diff --git a/drivers/base/firmware_loader/main.c > > > b/drivers/base/firmware_loader/main.c > > > index eb34089e4299..d7cdf04a8681 100644 > > > --- a/drivers/base/firmware_loader/main.c > > > +++ b/drivers/base/firmware_loader/main.c > > > @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device *device, > > > struct fw_priv *fw_priv) > > > break; > > > } > > > > > > +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB > > > + if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) || > > > + (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0)) > > > + id = READING_FIRMWARE_REGULATORY_DB; > > > +#endif > > > > Whoa, no way. > > There are two methods for the kernel to verify firmware signatures. Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel mechanism to verify firmware it uses the request_firmware*() API for regulatory.db and regulatory.db.p7s, and IMA already can appraise these two files since the firmware API is used. As such I see no reason to add a new ID for them at all. Its not providing an *alternative*, its providing an *extra* kernel measure. If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own stacked LSM. I'd be open to see patches which set that out. May be a cleaner interface. > If both are enabled, do we require both signatures or is one enough. Good question. Considering it as a stacked LSM (although not implemented as one), I'd say its up to who enabled the Kconfig entries. If IMA and CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone enabled IMA though, then surely I agree that enabling CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to the system integrator to decide. If we however want to make it clear that such things as CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we could just make the kconfig depend on !IMA or something? Or perhaps a new kconfig for IMA which if selected it means that drivers can opt to open code *further* kernel signature verification, even though IMA already is sufficient. Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it? > Assigning a different id for regdb signed firmware allows LSMs and IMA > to handle regdb files differently. That's not the main concern here, its the precedent we are setting here for any new kernel interface which open codes firmware signing on its own. What you are doing means other kernel users who open codes their own firmware signing may need to add yet-another reading ID. That doesn't either look well on code, and seems kind of silly from a coding perspective given the above, in which I clarify IMA still is doing its own appraisal on it. > > > fw_priv->size = 0; > > > rc = kernel_read_file_from_path(path, &fw_priv->data, &size, > > > msize, id); > > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > > index dc16a73c3d38..d1153c2884b9 100644 > > > --- a/include/linux/fs.h > > > +++ b/include/linux/fs.h > > > @@ -2811,6 +2811,7 @@ extern int do_pipe_flags(int *, int); > > > id(FIRMWARE, firmware) \ > > > id(FIRMWARE_PREALLOC_BUFFER, firmware) \ > > > id(FIRMWARE_FALLBACK, firmware) \ > > > + id(FIRMWARE_REGULATORY_DB, firmware)\ > > > > Why could IMA not appriase these files? They are part of the standard path. > > The subsequent patch attempts to verify the IMA-appraisal signature, but on > failure it falls back to allowing regdb signatures. > For systems that only want to load firmware based on IMA-appraisal, then >regdb wouldn't be enabled. I think we can codify this a bit better, without a new ID. Luis
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Fri, May 04, 2018 at 07:54:28AM +0200, Ard Biesheuvel wrote:
> On 4 May 2018 at 01:29, Luis R. Rodriguez wrote:
> > On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote:
> [...]
> >> diff --git a/Documentation/driver-api/firmware/request_firmware.rst
> >> b/Documentation/driver-api/firmware/request_firmware.rst
> >> index c8bddbdcfd10..560dfed76e38 100644
> >> --- a/Documentation/driver-api/firmware/request_firmware.rst
> >> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> >> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns
> >> non-zero and fw_entry
> >> is set to NULL. Once your driver is done with processing the firmware it
> >> can call call firmware_release(fw_entry) to release the firmware image
> >> and any related resource.
> >> +
> >> +EFI embedded firmware support
> >> +=
> >
> > This is a new fallback mechanism, please see:
> >
> > Documentation/driver-api/firmware/fallback-mechanisms.rst
> >
> > Refer to the section "Types of fallback mechanisms", augument the list there
> > and then move the section "Firmware sysfs loading facility" to a new file,
> > and
> > then add a new file for your own.
> >
> >> +
> >> +On some devices the system's EFI code / ROM may contain an embedded copy
> >> +of firmware for some of the system's integrated peripheral devices and
> >> +the peripheral's Linux device-driver needs to access this firmware.
> >
> > You in no way indicate this is a just an invented scheme, a custom solution
> > and
> > nothing standard. I realize Ard criticized that the EFI Firmware Volume
> > Protocol
> > is not part of the UEFI spec -- however it is a bit more widely used right?
> > Why can't Linux support it instead?
> >
>
> Most implementations of UEFI are based on PI,
That seems to be the UEFI Platform Initialization specification:
http://www.uefi.org/sites/default/files/resources/PI_Spec_1_6.pdf
> and so it is likely that
> the protocols are available. However, the PI spec does not cover
> firmware blobs,
Indeed, I cannot find anything about it on the PI Spec, but I *can* easily
find a few documents referring to the Firmware Volume Protocol:
http://wiki.phoenix.com/wiki/index.php/EFI_FIRMWARE_VOLUME_PROTOCOL
But this has no references at all...
I see stupid patents over some of this and authentication mechanisms for it:
https://patents.google.com/patent/US20170098084
> and so it is undefined whether such blobs are self
> contained (i.e., in separate files in the firmware volume), statically
> linked into the driver or maybe even encrypted or otherwise
> encapsulated, and the actual loadable image only lives in memory.
Got it, thanks this helps! There are two things then:
1) The "EFI Firmware Volume Protocol" ("FV" for short in your descriptions
below), and whether to support it or not in the future and recommend it
for future use cases.
b) Han's EFI scraper to help support 2 drivers, and whether or not to
recommend it for future use cases.
> Hans's case is the second one, i.e., the firmware is at an arbitrary
> offset in the driver image. Using the FV protocol in this case would
> result in a mix of both approaches: look up the driver file by GUID
> [which could change btw between different versions of the system
> firmware, although this is unlikely] and then still use the prefix/crc
> based approach to sift through the image itself.
Got it. And to be clear its a reversed engineered solution to what
two vendors decided to do.
> But my main objection is simply that from the UEFI forum point of
> view, there is a clear distinction between the OS visible interfaces
> in the UEFI spec and the internal interfaces in the PI spec (which for
> instance are not subject to the same rules when it comes to backward
> compatibility), and so I think we should not depend on PI at all.
Ah I see.
> This
> is all the more important considering that we are trying to encourage
> the creation of other implementations of UEFI that are not based on PI
> (e.g., uboot for arm64 implements the required UEFI interfaces for
> booting the kernel via GRUB), and adding dependencies on PI protocols
> makes that a moving target.
Got it!
> So in my view, we either take a ad-hoc approach which works for the
> few platforms we expect to support, in which case Hans's approach is
> sufficient,
Modulo it needs some work for ARM as it only works for x86 right now ;)
> or we architect it properly, in which case we shouldn't
> depend on PI because it does not belon
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
On Tue, May 08, 2018 at 03:38:05PM +, Luis R. Rodriguez wrote: > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote: > > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez > > wrote: > > > Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK. > > > > > > It would be good for us to hear from Android folks if their current use of > > > request_firmware_into_buf() is designed in practice to *never* use the > > > direct > > > filesystem firmware loading interface, and always rely instead on the > > > fallback mechanism. > > > > It's hard to answer this question for Android in general. As far as I > > can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK) > > are: > > 1) We have multiple different paths on our devices where firmware can > > be located, and the direct loader only supports one custom path FWIW I'd love to consider patches to address this, if this is something you may find a need for in the future to *avoid* the fallback, however would like a clean solution. > > 2) Most of those paths are not mounted by the time the corresponding > > drivers are loaded, because pretty much all Android kernels today are > > built without module support, and therefore drivers are loaded well > > before the firmware partition is mounted I've given this some more thought and you can address this with initramfs, this is how other Linux distributions are addressing this. One way to address this automatically is to scrape the drivers built-in or needed early on boot in initamfs and if the driver has a MODULE_FIRMWARE() its respective firmware is added to initramfs as well. If you *don't* use initramfs, then yes you can obviously run into issues where your firmware may not be accessible if the driver is somehow loaded early. > > 3) I think we use _FALLBACK because doing this with uevents is just > > the easiest thing to do; our init code has a firmware helper that > > deals with this and searches the paths that we care about > > > > 2) will change at some point, because Android is moving towards a > > model where device-specific peripheral drivers will be loaded as > > modules, and since those modules would likely come from the same > > partition as the firmware, it's possible that the direct load would > > succeed (depending on whether the custom path is configured there or > > not). But I don't think we can rely on the direct loader even in those > > cases, unless we could configure it with multiple custom paths. Using initramfs will help, but because of the custom path needs -- you're right, we don't have anything for that yet, its also a bit unclear if something nice and clean can be drawn up for it. So perhaps dealing with the fallback mechanism is the way to go for this for sure, since we already have support for it. Just keep in mind that the fallback mechanism costs you about ~13436 bytes. So, if someone comes up with a clean interface for custom paths I'd love to consider it to avoid those 13436 bytes. Luis
Re: [PATCH 02/18] docs: fix location of request_firmware & friends
On Mon, May 07, 2018 at 06:35:38AM -0300, Mauro Carvalho Chehab wrote:
> commit 5d6d1ddd2730 ("firmware: move firmware loader into its own directory")
> and other commits renamed the old firmware_class.c file and split it
> into separate files, but documentation was not changed accordingly,
> causing Sphinx errors.
>
> Change the location accordingly at the documentation files.
>
> While here, add a missing entry at request_firmware.rst for
> release_firmware() function.
>
> Fixes: 5d6d1ddd2730 ("firmware: move firmware loader into its own directory")
> Cc: Kees Cook
> Cc: Luis R. Rodriguez
> Cc: Greg Kroah-Hartman
> Signed-off-by: Mauro Carvalho Chehab
> ---
> Documentation/dell_rbu.txt | 4 ++--
> .../driver-api/firmware/fallback-mechanisms.rst | 2 +-
> .../driver-api/firmware/request_firmware.rst| 17 +++--
> Documentation/driver-api/infrastructure.rst | 2 +-
> Documentation/power/suspend-and-cpuhotplug.txt | 2 +-
> 5 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/dell_rbu.txt b/Documentation/dell_rbu.txt
> index 0fdb6aa2704c..befeff80e7ec 100644
> --- a/Documentation/dell_rbu.txt
> +++ b/Documentation/dell_rbu.txt
> @@ -121,8 +121,8 @@ read back the image downloaded.
>
> .. note::
>
> - This driver requires a patch for firmware_class.c which has the modified
> - request_firmware_nowait function.
> + This driver requires a patch for drivers/base/firmware_loader/main.c
> + which has the modified request_firmware_nowait() function.
>
> Also after updating the BIOS image a user mode application needs to
> execute
> code which sends the BIOS update request to the BIOS. So on the next
> reboot
This part looks good and is needed.
> diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> index f353783ae0be..7aed31b5a439 100644
> --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> @@ -72,7 +72,7 @@ the firmware requested, and establishes it in the device
> hierarchy by
> associating the device used to make the request as the device's parent.
> The sysfs directory's file attributes are defined and controlled through
> the new device's class (firmware_class) and group (fw_dev_attr_groups).
> -This is actually where the original firmware_class.c file name comes from,
> +This is actually where drivers/base/firmware_loader/fallback.c comes from,
Not this part.
What I meant to keep well documented here was not just only the old firmware
file name for the code, but also the module name firmware_class, and its
respective sysfs class name which is registered. From what I recall testing, we
could not rename the module now because of this. I believe it had to do with
the modular case, given the sysfs class could still be registered.
The fact that I forget the exact *issue* which prevented the module rename shows
how important it is to document this.
Folks 10 years from now may wonder why the hell that name stuck, and the point
was
to document that the *original* loader was the sysfs fallback mechanism.
> as originally the only firmware loading mechanism available was the
> mechanism we now use as a fallback mechanism.
>
> diff --git a/Documentation/driver-api/firmware/request_firmware.rst
> b/Documentation/driver-api/firmware/request_firmware.rst
> index cf4516dfbf96..8e34d29ea02d 100644
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -17,19 +17,24 @@ an error is returned.
>
> request_firmware
>
> -.. kernel-doc:: drivers/base/firmware_class.c
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> :functions: request_firmware
This is fixed on Hans de Goede's patch already merged on Greg's own tree.
> request_firmware_direct
> ---
> -.. kernel-doc:: drivers/base/firmware_class.c
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> :functions: request_firmware_direct
This is fixed on Hans de Goede's patch already merged on Greg's own tree.
> request_firmware_into_buf
> -
> -.. kernel-doc:: drivers/base/firmware_class.c
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> :functions: request_firmware_into_buf
This is fixed on Hans de Goede's patch already merged on Greg's own tree.
> +release_firmware
> +
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> + :functions: release_firmware
This is fixed on Hans de Goede's patch
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote: > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez wrote: > > Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK. > > > > It would be good for us to hear from Android folks if their current use of > > request_firmware_into_buf() is designed in practice to *never* use the > > direct > > filesystem firmware loading interface, and always rely instead on the > > fallback mechanism. > > It's hard to answer this question for Android in general. As far as I > can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK) > are: > 1) We have multiple different paths on our devices where firmware can > be located, and the direct loader only supports one custom path > 2) Most of those paths are not mounted by the time the corresponding > drivers are loaded, because pretty much all Android kernels today are > built without module support, and therefore drivers are loaded well > before the firmware partition is mounted > 3) I think we use _FALLBACK because doing this with uevents is just > the easiest thing to do; our init code has a firmware helper that > deals with this and searches the paths that we care about > > 2) will change at some point, because Android is moving towards a > model where device-specific peripheral drivers will be loaded as > modules, and since those modules would likely come from the same > partition as the firmware, it's possible that the direct load would > succeed (depending on whether the custom path is configured there or > not). But I don't think we can rely on the direct loader even in those > cases, unless we could configure it with multiple custom paths. > > I have no reason to believe request_firmware_into_buf() is special in > this regard; drivers that depend on it may have their corresponding > firmware in different locations, so just depending on the direct > loader would not be good enough. Thanks! This is very useful! This provides yet-another justification and use case to document for the fallback mechanism. I'll go and extend it. > > > > Is ptr below > > > > ret = request_firmware_into_buf(&seg_fw, fw_name, dev, > > ptr, phdr->p_filesz); > > > > Also part of the DMA buffer allocated earlier via: > > > > ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size); > > > > Android folks? > > I think the Qualcomm folks owning this (Andy, David, Bjorn, already > cc'd here) are better suited to answer that question. Andy, David, Bjorn? Luis
