Re: [PATCH 1/2] PM / Usermodehelper: Introduce reference counting to solve usermodehelper_disabled race

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 12/06/2011 06:29 AM, Namhyung Kim wrote:

> Hello,
> 
> 2011-12-06 오전 6:26, Srivatsa S. Bhat 쓴 글:
>> This patch adds the necessary synchronization framework to fix the race
>> condition with the 'usermodehelper_disabled' flag, by implementing a
>> refcounting solution. Specifically, it introduces the pair
>> get_usermodehelper()
>> and put_usermodehelper(), which can be used by the readers (those who
>> want to
>> read the value of the usermodehelper_disabled flag, such as
>> _request_firmware()
>> in this case). The writers (those who enable/disable usermodehelpers by
>> setting/resetting that flag) can use the pair umh_control_begin() and
>> umh_control_done().
>>
>> The reason for using a refcounting solution and not just a plain
>> mutex, is
>> that we don't want to unnecessarily serialize all users of
>> request_firmware(),
>> which act as readers. But note that we cannot use reader-writer locks
>> here
>> because the readers sleep (waiting for the firmware load from
>> user-space),
>> and sleeping with spinlocks held is not allowed. So refcounting
>> implemented
>> using mutex locks underneath, seems to be the best fit here.
>>
>> Signed-off-by: Srivatsa S. Bhat<srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>> The refcounting solution implemented here is adapted from the one used in
>> the CPU hotplug infrastructure (kernel/cpu.c). If this patchset sounds
>> reasonable, I plan to make the refcounting generic (in a later patch) and
>> expose it via include/linux/refcount.h or something similar, and then
>> use it
>> at these 2 places instead of duplicating code.
>>
> 
> IMHO it seems that the write path of the cpu_hotplug is protected by
> another mutex (cpu_add_remove_lock) to guarantee that the only one
> writer is active at a time. But I'm not sure this is the case for the
> umhelper too.


For the umhelper, I had not added anything explicit for this serialization
because, all the users of usermodehelper_disable/enable are callers
from hibernate/suspend code (which all take the 'pm_mutex' lock before
doing anything) or from reboot/shutdown code.

> 
> If more than 2 tasks call umh_control_begin() at the same time (is it
> possible though?), it will lost tasks except for the winner and
> active_writer AFAICS. Am I missing something?
> 


See my thoughts above about the callers of umh_control_begin(). 

Anyways, I'll use rwsemaphores as Tejun suggested, since that would be
the most logical choice here, and it also makes the code much simpler.

Thanks a lot for your review!

[Btw I was wondering why your mail didn't land in my inbox. Now I see,
I am neither in your "To" or "Cc" list! :-)]

Thanks,
Srivatsa S. Bhat