Re: [PATCH 6/6] PM / Sleep: Mitigate race between the freezer and request_firmware()

[Stephen Boyd <sboyd@codeaurora.org>]

On 03/26/12 13:06, Rafael J. Wysocki wrote:
> On Monday, March 26, 2012, Stephen Boyd wrote:
>> On 03/25/12 15:04, Rafael J. Wysocki wrote:
>>> Index: linux/kernel/power/process.c
>>> ===================================================================
>>> --- linux.orig/kernel/power/process.c
>>> +++ linux/kernel/power/process.c
>>> @@ -135,6 +135,7 @@ int freeze_processes(void)
>>>  	error = try_to_freeze_tasks(true);
>>>  	if (!error) {
>>>  		printk("done.");
>>> +		__usermodehelper_reset(UMH_DISABLED);
>>>  		oom_killer_disable();
>>>  	}
>>>  	printk("\n");
>> nitpick: This doesn't seem to be doing a reset, more like a set. Perhaps
>> this function should be called __usermodehelper_set()?
> Yes, you are right.  I changed that before, but somehow managed to send an old
> patch.  The new version follows.

Looks good. Thanks.

> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM / Sleep: Mitigate race between the freezer and request_firmware()
>
> There is a race condition between the freezer and request_firmware()
> such that if request_firmware() is run on one CPU and
> freeze_processes() is run on another CPU and usermodehelper_disable()
> called by it succeeds to grab umhelper_sem for writing before
> usermodehelper_read_trylock() called from request_firmware()
> acquires it for reading, the request_firmware() will fail and
> trigger a WARN_ON() complaining that it was called at a wrong time.
> However, in fact, it wasn't called at a wrong time and
> freeze_processes() simply happened to be executed simultaneously.
>
> To avoid this race, at least in some cases, modify
> usermodehelper_read_trylock() so that it doesn't fail if the
> freezing of tasks has just started and hasn't been completed yet.
> Instead, during the freezing of tasks, it will try to freeze the
> task that has called it so that it can wait until user space is
> thawed without triggering the scary warning.
>
> For this purpose, change usermodehelper_disabled so that it can
> take three different values, UMH_ENABLED (0), UMH_FREEZING and
> UMH_DISABLED.  The first one means that usermode helpers are
> enabled, the last one means "hard disable" (i.e. the system is not
> ready for usermode helpers to be used) and the second one
> is reserved for the freezer.  Namely, when freeze_processes() is
> started, it sets usermodehelper_disabled to UMH_FREEZING which
> tells usermodehelper_read_trylock() that it shouldn't fail just
> yet and should call try_to_freeze() if woken up and cannot
> return immediately.  This way all freezable tasks that happen
> to call request_firmware() right before freeze_processes() is
> started and lose the race for umhelper_sem with it will be
> frozen and will sleep until thaw_processes() unsets
> usermodehelper_disabled.  [For the non-freezable callers of
> request_firmware() the race for umhelper_sem against
> freeze_processes() is unfortunately unavoidable.]

Reported-by: Stephen Boyd <sboyd@codeaurora.org>

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.