Re: [PATCH 3/3] firmware: add mutex fw_lock_fallback for race condition

Scott Branden
2019-08-19T16:19:57Z

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@broadcom.com>
>> ---
>> 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