linux-kernel - Re: [PATCH 1/2] PM / Usermodehelper: Introduce reference counting to solve usermodehelper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 07 Dec 2011 18:00:25 +0530
From:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
To:	Namhyung Kim <namhyung@...il.com>
CC:	linux-kernel@...r.kernel.org, gregkh@...e.de, dhowells@...hat.com,
	eparis@...hat.com, rjw@...k.pl, kay.sievers@...y.org,
	jmorris@...ei.org, tj@...nel.org, bp@...64.org,
	linux-pm@...r.kernel.org
Subject: Re: [PATCH 1/2] PM / Usermodehelper: Introduce reference counting
 to solve usermodehelper_disabled race

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@...ux.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/