[PATCH 0/2] PM / Usermodehelper: Fix freezer failures due to racy usermodehelper_is_disabled()

[PATCH 0/2] PM / Usermodehelper: Fix freezer failures due to racy usermodehelper_is_disabled()

[Srivatsa S. Bhat]

Hi Tejun, Boris and Rafael,

I am revisiting the microcode vs freezer issue with this patchset, with
a much better solution this time (hopefully), since now I am attacking
the root cause of the problem (a race in usermodehelpers), in order
to solve the issue I reported earlier in:
http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591

:-)

Btw, Rafael, using PM notifiers for mutual exclusion from freezer (like we
did in CPU hotplug case), for x86 microcode driver wouldn't work out, since
there is no guarantee that our callback will get registered before freezing
starts, since the module init code itself needs to be mutexed from the freezer.
And the callback registration would also have to happen from the init code
itself! (we didn't have this problem in CPU hotplug case).

And since Boris pointed out several times that in real world, microcode won't
be applied all that often, I decided to focus on the root-cause and leave
the microcode driver alone :-)

---
Commit a144c6a (PM: Print a warning if firmware is requested when tasks
are frozen) introduced usermodehelper_is_disabled() to warn and exit
immediately if firmware is requested when usermodehelpers are disabled.

However, it is racy. Consider the following scenario, currently used in
drivers/base/firmware_class.c:

...
if (usermodehelper_is_disabled())
        goto out;

/* Do actual work */
...

out:
        return err;

Nothing prevents someone from disabling usermodehelpers just after the
check in the 'if' condition, which means that it is quite possible to try
doing the "actual work" with usermodehelpers disabled, leading to undesirable
consequences.

In particular, this race condition in _request_firmware() causes task
freezing failures whenever suspend/hibernation is in progress because, it
wrongly waits to get the firmware/microcode image from userspace when actually
the usermodehelpers are disabled or userspace has been frozen. Some of the
example scenarios that cause freezing failures due to this race are those
that depend on userspace via request_firmware(), such as x86 microcode module
initialization and microcode image reload.

Previous discussions about this issue can be found at:
http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591

This patchset adds proper synchronization to fix this issue.

It is to be noted that this patchset fixes the freezing failures but doesn't
remove the warnings. IOW, it does not attempt to add explicit synchronization
to x86 microcode driver to avoid requesting microcode image at inopportune
moments. Because, the warnings were introduced to highlight such cases, in
the first place. And we need not silence the warnings, since we take care
of the *real* problem (freezing failure) and hence, after that, the warnings
are pretty harmless anyway.

--
 Srivatsa S. Bhat (2):
      PM / Usermodehelper: Introduce reference counting to solve usermodehelper_disabled race
      PM / request_firmware(): Use the refcounting solution to fix the race


  drivers/base/firmware_class.c |    3 ++
 include/linux/kmod.h          |    2 +
 kernel/kmod.c                 |   74 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 78 insertions(+), 1 deletions(-)


Regards,
Srivatsa S. Bhat
IBM Linux Technology Center

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

[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.

 include/linux/kmod.h |    2 ++
 kernel/kmod.c        |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index b16f653..845fe3d 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -117,5 +117,7 @@ extern void usermodehelper_init(void);
 extern int usermodehelper_disable(void);
 extern void usermodehelper_enable(void);
 extern bool usermodehelper_is_disabled(void);
+extern void get_usermodehelper(void);
+extern void put_usermodehelper(void);
 
 #endif /* __LINUX_KMOD_H__ */
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 2142687..acb52af 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -49,6 +49,70 @@ static kernel_cap_t usermodehelper_bset = CAP_FULL_SET;
 static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;
 static DEFINE_SPINLOCK(umh_sysctl_lock);
 
+static struct {
+	struct task_struct *active_writer;
+	struct mutex lock; /* Synchronizes accesses to refcount, */
+	/*
+	 * Also blocks the new readers during an ongoing update to the
+	 * 'usermodehelper_disabled' flag.
+	 */
+	int refcount;
+} umhelper = {
+	.active_writer = NULL,
+	.lock = __MUTEX_INITIALIZER(umhelper.lock),
+	.refcount = 0,
+};
+
+void get_usermodehelper(void)
+{
+	might_sleep();
+	if (umhelper.active_writer == current)
+		return;
+	mutex_lock(&umhelper.lock);
+	umhelper.refcount++;
+	mutex_unlock(&umhelper.lock);
+}
+EXPORT_SYMBOL_GPL(get_usermodehelper);
+
+void put_usermodehelper(void)
+{
+	if (umhelper.active_writer == current)
+		return;
+	mutex_lock(&umhelper.lock);
+	if (!--umhelper.refcount && unlikely(umhelper.active_writer))
+		wake_up_process(umhelper.active_writer);
+	mutex_unlock(&umhelper.lock);
+}
+EXPORT_SYMBOL_GPL(put_usermodehelper);
+
+/*
+ * This ensures that enabling or disabling usermodehelpers can begin
+ * only when the refcount goes to zero.
+ *
+ * Note that during an ongoing usermodehelper enable/disable operation,
+ * the new readers, if any, will be blocked by umhelper.lock
+ */
+static void umh_control_begin(void)
+{
+	umhelper.active_writer = current;
+
+	for (;;) {
+		mutex_lock(&umhelper.lock);
+		if (likely(!umhelper.refcount))
+			break;
+		__set_current_state(TASK_UNINTERRUPTIBLE);
+		mutex_unlock(&umhelper.lock);
+		schedule();
+	}
+}
+
+static void umh_control_done(void)
+{
+	umhelper.active_writer = NULL;
+	mutex_unlock(&umhelper.lock);
+}
+
+
 #ifdef CONFIG_MODULES
 
 /*

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

[Tejun Heo]

On Tue, Dec 06, 2011 at 02:56:15AM +0530, Srivatsa S. Bhat wrote:
> 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.

I haven't really read the patch yet but, from the description, why not
use rwsem?

Thanks.

-- 
tejun

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

[Srivatsa S. Bhat]

On 12/06/2011 03:08 AM, Tejun Heo wrote:

> On Tue, Dec 06, 2011 at 02:56:15AM +0530, Srivatsa S. Bhat wrote:
>> 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.
> 
> I haven't really read the patch yet but, from the description, why not
> use rwsem?
> 

Hey that would be perfect! That hadn't crossed my mind.. Thanks for pointing
it out. I'll respin the patchset using rwsemaphores.

Regards,
Srivatsa S. Bhat

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

[Namhyung Kim]

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.

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?

Thanks.
Namhyung Kim


>   include/linux/kmod.h |    2 ++
>   kernel/kmod.c        |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 66 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index b16f653..845fe3d 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -117,5 +117,7 @@ extern void usermodehelper_init(void);
>   extern int usermodehelper_disable(void);
>   extern void usermodehelper_enable(void);
>   extern bool usermodehelper_is_disabled(void);
> +extern void get_usermodehelper(void);
> +extern void put_usermodehelper(void);
>
>   #endif /* __LINUX_KMOD_H__ */
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 2142687..acb52af 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -49,6 +49,70 @@ static kernel_cap_t usermodehelper_bset = CAP_FULL_SET;
>   static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;
>   static DEFINE_SPINLOCK(umh_sysctl_lock);
>
> +static struct {
> +	struct task_struct *active_writer;
> +	struct mutex lock; /* Synchronizes accesses to refcount, */
> +	/*
> +	 * Also blocks the new readers during an ongoing update to the
> +	 * 'usermodehelper_disabled' flag.
> +	 */
> +	int refcount;
> +} umhelper = {
> +	.active_writer = NULL,
> +	.lock = __MUTEX_INITIALIZER(umhelper.lock),
> +	.refcount = 0,
> +};
> +
> +void get_usermodehelper(void)
> +{
> +	might_sleep();
> +	if (umhelper.active_writer == current)
> +		return;
> +	mutex_lock(&umhelper.lock);
> +	umhelper.refcount++;
> +	mutex_unlock(&umhelper.lock);
> +}
> +EXPORT_SYMBOL_GPL(get_usermodehelper);
> +
> +void put_usermodehelper(void)
> +{
> +	if (umhelper.active_writer == current)
> +		return;
> +	mutex_lock(&umhelper.lock);
> +	if (!--umhelper.refcount&&  unlikely(umhelper.active_writer))
> +		wake_up_process(umhelper.active_writer);
> +	mutex_unlock(&umhelper.lock);
> +}
> +EXPORT_SYMBOL_GPL(put_usermodehelper);
> +
> +/*
> + * This ensures that enabling or disabling usermodehelpers can begin
> + * only when the refcount goes to zero.
> + *
> + * Note that during an ongoing usermodehelper enable/disable operation,
> + * the new readers, if any, will be blocked by umhelper.lock
> + */
> +static void umh_control_begin(void)
> +{
> +	umhelper.active_writer = current;
> +
> +	for (;;) {
> +		mutex_lock(&umhelper.lock);
> +		if (likely(!umhelper.refcount))
> +			break;
> +		__set_current_state(TASK_UNINTERRUPTIBLE);
> +		mutex_unlock(&umhelper.lock);
> +		schedule();
> +	}
> +}
> +
> +static void umh_control_done(void)
> +{
> +	umhelper.active_writer = NULL;
> +	mutex_unlock(&umhelper.lock);
> +}
> +
> +
>   #ifdef CONFIG_MODULES
>
>   /*
>

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

[Srivatsa S. Bhat]

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

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

[Namhyung Kim]

2011-12-07 (수), 18:00 +0530, Srivatsa S. Bhat:
> On 12/06/2011 06:29 AM, Namhyung Kim wrote:
> 
> > Hello,
> > 
> > 2011-12-06 오전 6:26, Srivatsa S. Bhat 쓴 글:
> >> 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.
> 

OK.


> > 
> > 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!
> 

Yeah, I think the rwsem would be more reasonable, too.


> [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! :-)]
> 

My email client was screwed up at that time, maybe due to stupid
security stuff in my company :(

Thanks.


-- 
Regards,
Namhyung Kim

[PATCH 2/2] PM / request_firmware(): Use the refcounting solution to fix the race

[Srivatsa S. Bhat]

It is racy to merely call usermodehelper_is_disabled() to check whether
we can successfully use usermodehelpers to get firmware from userspace,
because, an event such as suspend/hibernation in progress could disable
the usermodehelpers at the same time.
So use the pair [get|put]_usermodehelper() around the above check (reader),
to solve this race. Also, the writers, namely usermodehelper_[disable|enable]
should use umh_control_[begin|done] for this synchronization to be effective.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 drivers/base/firmware_class.c |    3 +++
 kernel/kmod.c                 |   10 +++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 06ed6b4..8a04946 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -534,6 +534,8 @@ static int _request_firmware(const struct firmware **firmware_p,
 		return 0;
 	}
 
+	get_usermodehelper();
+
 	if (WARN_ON(usermodehelper_is_disabled())) {
 		dev_err(device, "firmware: %s will not be loaded\n", name);
 		retval = -EBUSY;
@@ -572,6 +574,7 @@ static int _request_firmware(const struct firmware **firmware_p,
 	fw_destroy_instance(fw_priv);
 
 out:
+	put_usermodehelper();
 	if (retval) {
 		release_firmware(firmware);
 		*firmware_p = NULL;
diff --git a/kernel/kmod.c b/kernel/kmod.c
index acb52af..e6951b3 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -337,6 +337,8 @@ static void __call_usermodehelper(struct work_struct *work)
  * If set, call_usermodehelper_exec() will exit immediately returning -EBUSY
  * (used for preventing user land processes from being created after the user
  * land has been frozen during a system-wide hibernation or suspend operation).
+ * Should always be manipulated under umhelper.lock, by using the functions
+ * umh_control_begin() and umh_control_done().
  */
 static int usermodehelper_disabled = 1;
 
@@ -362,8 +364,10 @@ int usermodehelper_disable(void)
 {
 	long retval;
 
+	umh_control_begin();
 	usermodehelper_disabled = 1;
-	smp_mb();
+	umh_control_done();
+
 	/*
 	 * From now on call_usermodehelper_exec() won't start any new
 	 * helpers, so it is sufficient if running_helpers turns out to
@@ -376,7 +380,9 @@ int usermodehelper_disable(void)
 	if (retval)
 		return 0;
 
+	umh_control_begin();
 	usermodehelper_disabled = 0;
+	umh_control_done();
 	return -EAGAIN;
 }
 
@@ -385,7 +391,9 @@ int usermodehelper_disable(void)
  */
 void usermodehelper_enable(void)
 {
+	umh_control_begin();
 	usermodehelper_disabled = 0;
+	umh_control_done();
 }
 
 /**