[PATCH 4/5] umh: fix processed error when UMH_WAIT_PROC is used

[Luis R. Rodriguez]

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, );
@@ -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

[Luis R. Rodriguez]

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()

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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()

[Luis R. Rodriguez]

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"

[Luis R. Rodriguez]

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"

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[PATCH] MAINTAINERS: update name change for myself

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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 11/11] proc/sched: remove unused sched_time_avg_ms

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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: [PATCH 0/2] Avoid firmware warning in imx-sdma

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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: bpf-next boot error: KASAN: use-after-free Write in call_usermodehelper_exec_work

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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 sysfs fallback interface
and don't want to use it to overr

Re: [PATCH v6 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

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 sysfs fallback interface
and don't want to use it to overr

[PATCH] Documentation: update firmware loader fallback reference

[Luis R. Rodriguez]

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

[PATCH] Documentation: update firmware loader fallback reference

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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 +++
>  

Re: [PATCH v6 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

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 +++
>  

Re: [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback)

[Luis R. Rodriguez]

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 v4 5/8] ima: based on policy require signed firmware (sysfs fallback)

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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: [PATCH v3 0/2] mm: PAGE_KERNEL_* fallbacks

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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 the plan to consider incorporating more PostgreSQL
li

Re: PostgreSQL licensed code on Linux

[Luis R. Rodriguez]

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 the plan to consider incorporating more PostgreSQL
li

PostgreSQL licensed code on Linux

[Luis R. Rodriguez]

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

PostgreSQL licensed code on Linux

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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: [PATCH v3 0/2] mm: PAGE_KERNEL_* fallbacks

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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: Are media drivers abusing of GFP_DMA? - was: Re: [LSF/MM TOPIC NOTES] x86 ZONE_DMA love

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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, _priv->data, ,
> > 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 firmware verification in userspace or in the

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

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, _priv->data, ,
> > 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 firmware verification in userspace or in the

Re: [PATCH 6/9] firmware: print firmware name on fallback path

[Luis R. Rodriguez]

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" <mcg...@kernel.org> 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 6/9] firmware: print firmware name on fallback path

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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, _priv->data, ,
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_FIRMWARE_FALLBACK)
for the old syfs loading facility.

So in th

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

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, _priv->data, ,
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_FIRMWARE_FALLBACK)
for the old syfs loading facility.

So in th

Re: [PATCH] coredump: rename umh_pipe_setup() to coredump_pipe_setup()

[Luis R. Rodriguez]

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()

[Luis R. Rodriguez]

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()

[Luis R. Rodriguez]

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 <mcg...@kernel.org>
> > ---
> >  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] coredump: rename umh_pipe_setup() to coredump_pipe_setup()

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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 <mcg...@kernel.org>
---
 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 2/4] xfs: add verifier check for symlink with append/immutable flags

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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 0/4] vfs: detect symlink corruption with attributes

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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 <mcg...@kernel.org>
---
 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 = _dir_inode_operations;
inode->i_fop = _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 = _encrypted_symlink_inode_operations;
ext4_set_aops(inode);
-- 
2.17.0

[RFC v2 3/4] ext4: add verifier check for symlink with append/immutable flags

[Luis R. Rodriguez]

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 = _dir_inode_operations;
inode->i_fop = _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 = _encrypted_symlink_inode_operations;
ext4_set_aops(inode);
-- 
2.17.0

[RFC v2 4/4] btrfs: verify symlinks with append/immutable flags

[Luis R. Rodriguez]

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 <mcg...@kernel.org>
---
 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 = _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 = _symlink_inode_operations;
inode_nohighmem(inode);
inode->i_mapping->a_ops = _symlink_aops;
-- 
2.17.0

[RFC v2 4/4] btrfs: verify symlinks with append/immutable flags

[Luis R. Rodriguez]

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 = _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 = _symlink_inode_operations;
inode_nohighmem(inode);
inode->i_mapping->a_ops = _symlink_aops;
-- 
2.17.0

[RFC v2 1/4] vfs: skip extra attributes check on removal for symlinks

[Luis R. Rodriguez]

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 <mcg...@kernel.org>
---
 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, i

[RFC v2 1/4] vfs: skip extra attributes check on removal for symlinks

[Luis R. Rodriguez]

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_attributes(inod

Re: [RFC] vfs: skip extra attributes check on removal for symlinks

[Luis R. Rodriguez]

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

Re: [RFC] vfs: skip extra attributes check on removal for symlinks

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This should let us associate enum kdoc to these values.
While at it, kdocify the fw_opt.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: coding style fixes, merge kdoc with enum move]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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 = _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  

[PATCH v7 01/14] firmware: wrap FW_OPT_* into an enum

[Luis R. Rodriguez]

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 = _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 &l

[PATCH v7 03/14] firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

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 <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: small coding style changes]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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 03/14] firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

The kernel-doc spec dictates a function name ends in ().

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Randy Dunlap <rdun...@infradead.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: adjust since the wide API rename is not yet merged]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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_now

[PATCH v7 02/14] firmware: use () to terminate kernel-doc function names

[Luis R. Rodriguez]

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_firmware() - cache one

[PATCH v7 05/14] firmware_loader: enhance Kconfig documentation over FW_LOADER

[Luis R. Rodriguez]

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 <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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 bu

[PATCH v7 05/14] firmware_loader: enhance Kconfig documentation over FW_LOADER

[Luis R. Rodriguez]

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
- where the use

[PATCH v7 04/14] firmware_loader: document firmware_sysfs_fallback()

[Luis R. Rodriguez]

This also sets the expecations for future fallback interfaces, even
if they are not exported.

Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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 04/14] firmware_loader: document firmware_sysfs_fallback()

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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 <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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.
-

[PATCH v7 07/14] firmware_loader: move kconfig FW_LOADER entries to its own file

[Luis R. Rodriguez]

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 filesystem lookup for

[PATCH v7 08/14] firmware_loader: make firmware_fallback_sysfs() print more useful

[Luis R. Rodriguez]

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 <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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 08/14] firmware_loader: make firmware_fallback_sysfs() print more useful

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

As per checkpatch using help is preferred over ---help---.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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 06/14] firmware_loader: replace ---help--- with help

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This reduces the unnecessary spew when trying to load optional firmware:
"Direct firmware load for ... failed with error -2"

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Kalle Valo <kv...@codeaurora.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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(, filename, ar->dev);
+   ret = firmware_request_nowarn(, filename, ar->dev);
ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot fw request '%s': %d\n",
   filename, ret);
 
-- 
2.17.0

[PATCH v7 10/14] ath10k: use firmware_request_nowarn() to load firmware

[Luis R. Rodriguez]

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(, filename, ar->dev);
+   ret = firmware_request_nowarn(, 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

[Luis R. Rodriguez]

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 <mchehab+sams...@kernel.org>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 .../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 14/14] Documentation: clarify firmware_class provenance and why we can't rename the module

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

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 <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Kalle Valo <kv...@codeaurora.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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(_file->firmware, filename, ar->dev);
+   ret = firmware_request_nowarn(_file->firmware, filename, ar->dev);
ath10k_dbg(ar, ATH10K_DBG_TESTMODE, "testmode fw request '%s': %d\n",
   filename, ret);
 
-- 
2.17.0

[PATCH v7 11/14] ath10k: re-enable the firmware fallback mechanism for testmode

[Luis R. Rodriguez]

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(_file->firmware, filename, ar->dev);
+   ret = firmware_request_nowarn(_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

[Luis R. Rodriguez]

It refers to a pending patch, but this was merged eons ago.

Reviewed-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 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 13/14] Documentation: remove stale firmware API reference

[Luis R. Rodriguez]

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

[Luis R. Rodriguez]

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 insertions(+), 131 deletions(-)
 create

[PATCH v7 00/14] firmware_loader changes for v4.18

[Luis R. Rodriguez]

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 insertions(+), 131 deletions(-)
 create

[PATCH v7 09/14] firmware: add firmware_request_nowarn() - load firmware without warnings

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

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 <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[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 <mcg...@kernel.org>
---
 .../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 09/14] firmware: add firmware_request_nowarn() - load firmware without warnings

[Luis R. Rodriguez]

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