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

[Scott Branden <scott.branden@broadcom.com>]

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