Hi Luis,
Thanks for the review.
I did not think this patch would be the final solution either
as indicated in the original cover letter and code comment.
Some comments inline.
On 2019-08-18 10:39 p.m., Luis Chamberlain wrote:
On Thu, Aug 15, 2019 at 05:09:45PM -0700, Scott Branden wrote:
A race condition exists between _request_firmware_prepare checking
if firmware is assigned and firmware_fallback_sysfs creating a sysfs
entry (kernel trace below). To avoid such condition add a mutex
fw_lock_fallback to protect against such condition.
I am not buying this fix, and it seems sloppy. More below.
misc test_firmware: Falling back to sysfs fallback for: nope-test-firmware.bin
So the fallback kicks in with the file that is not there.
sysfs: cannot create duplicate filename '/devices/virtual/misc/test_firmware/nope-test-firmware.bin'
And we have a duplicate entry, for the *device* created to allow us to
create a file entry to allow us to copy the file. Your tests had a loop,
so there is actually a race between two entries being created while
one one failed.
CPU: 4 PID: 2059 Comm: test_firmware-3 Not tainted 5.3.0-rc4 #1
Hardware name: Dell Inc. OptiPlex 7010/0KRC95, BIOS A13 03/25/2013
Call Trace:
dump_stack+0x67/0x90
sysfs_warn_dup.cold+0x17/0x24
sysfs_create_dir_ns+0xb3/0xd0
kobject_add_internal+0xa6/0x2a0
kobject_add+0x7e/0xb0
Note: kobject_add().
? _cond_resched+0x15/0x30
device_add+0x121/0x670
firmware_fallback_sysfs+0x15c/0x3c9
_request_firmware+0x432/0x5a0
? devres_find+0x63/0xc0
request_firmware_into_buf+0x63/0x80
test_fw_run_batch_request+0x96/0xe0
kthread+0xfb/0x130
? reset_store+0x30/0x30
? kthread_park+0x80/0x80
ret_from_fork+0x3a/0x50
kobject_add_internal failed for nope-test-firmware.bin with -EEXIST, don't try to register things with the same name in the same directory.
So above it makes it even clearer, two kobjets with the same name.
Signed-off-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
---
drivers/base/firmware_loader/main.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index bf44c79beae9..ce9896e3b782 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -88,6 +88,7 @@ static inline struct fw_priv *to_fw_priv(struct kref *ref)
/* fw_lock could be moved to 'struct fw_sysfs' but since it is just
* guarding for corner cases a global lock should be OK */
DEFINE_MUTEX(fw_lock);
+DEFINE_MUTEX(fw_lock_fallback);
The reason I don't like this fix is that this mutex is named after ther
fallback interface... but...
static struct firmware_cache fw_cache;
@@ -758,6 +759,17 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
if (!firmware_p)
return -EINVAL;
+ /*
+ * There is a race condition between _request_firmware_prepare checking
+ * if firmware is assigned and firmware_fallback_sysfs creating sysfs
+ * entries with duplicate names.
+ * Yet, with this lock the firmware_test locks up with cache enabled
+ * and no event used during firmware test.
+ * This points to some very racy code I don't know how to entirely fix.
+ */
+ if (opt_flags & FW_OPT_NOCACHE)
+ mutex_lock(&fw_lock_fallback);
Whoa.. What does no-cache have anything to do with the fallback interface
other than the fact we enable this feature for the fallback interface?
We don't need to penalize non-fallback users who *also* may want to
enable the no-cache feature.
So, the fix should be within the boundaries of the creation / deletion
of the kobject, not this nocache feature. Can you please re-evaluate
this code and look for a more compartamentalized solution to the
fallback code only?
To be honest, I find the entire firmware code sloppy. I don't think the
cache/no-cache feature is
implemented or tested properly nor fallback to begin with. I'm not
claiming this patch is the final
solution and indicated such in the cover letter and the comment above.
I hope there is someone more familiar with this code to comment further
and come up with a proper solution.
I have found numerous issues and race conditions with the firmware code
(I simply added a test).
1) Try loading the same valid firmware using no-cache once it has
already been loaded with cache.
It won't work, which is why I had to use a different filename in the
test for request_firmware_into_buf.
2) Try removing the "if (opt_flags & FW_OPT_NOCACHE)" in my patch and
always call the mutex.
The firmware test will lock up during a "no uevent" test. I am not
familiar with the code to
know why such is true and what issue this exposes in the code.
3) I have a driver that uses request_firmware_into_buf and have multiple
instances of the driver
loading the same firmware in parallel. Some of the data is not read
correctly in each instance.
I haven't yet to reproduce this issue with the firmware test but
currently have a mutex around the entire
call to request_firmware_into_buf in our driver.
Perhaps it is better at this point to add a mutex in
request_firmware_into_buf to make is entirely safe?
(Perhaps even with every request_firmware functions as none seems to be
tested properly.)
Or, add a new function called safe_request_firmware_into_buf with such
mutex to protect the function.
The current racey request_firmware functions could then be left alone
and those who want reliable
firmware loading can use the safe calls?
Luis
