[PATCH] apm-emulation: Fix mutex race condition

[PATCH] apm-emulation: Fix mutex race condition

[Paul Parsons]

A process suspending the system (e.g. apm -s) locks apm_mutex at the start of apm_ioctl() and keeps it locked while calling pm_suspend(). A second process trying to ACK the suspend (e.g. apmd) may then block in apm_ioctl() trying to lock the same mutex. The first process tries to freeze processes, but the ACK process cannot be frozen because mutex_lock() is uninterruptible. Consequently pm_suspend() will ultimately fail in try_to_freeze_tasks(). This patch allows the ACK process to be frozen if it is blocked in apm_ioctl().

Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
---
--- clean-2.6.39-rc5/drivers/char/apm-emulation.c	2011-03-15 01:20:32.000000000 +0000
+++ linux-2.6.39-rc5/drivers/char/apm-emulation.c	2011-05-03 00:34:05.467660807 +0100
@@ -275,7 +275,8 @@ apm_ioctl(struct file *filp, u_int cmd, 
 	if (!as->suser || !as->writer)
 		return -EPERM;
 
-	mutex_lock(&apm_mutex);
+	while (mutex_lock_interruptible(&apm_mutex) < 0)
+		try_to_freeze();
 	switch (cmd) {
 	case APM_IOC_SUSPEND:
 		mutex_lock(&state_lock);

Re: [PATCH] apm-emulation: Fix mutex race condition

[Rafael J. Wysocki]

On Tuesday, May 03, 2011, Paul Parsons wrote:
> A process suspending the system (e.g. apm -s) locks apm_mutex at the
> start of apm_ioctl() and keeps it locked while calling pm_suspend().
> A second process trying to ACK the suspend (e.g. apmd) may then block
> in apm_ioctl() trying to lock the same mutex. The first process tries
> to freeze processes, but the ACK process cannot be frozen because
> mutex_lock() is uninterruptible. Consequently pm_suspend() will
> ultimately fail in try_to_freeze_tasks(). This patch allows the ACK
> process to be frozen if it is blocked in apm_ioctl().
> 
> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
> ---
> --- clean-2.6.39-rc5/drivers/char/apm-emulation.c	2011-03-15 01:20:32.000000000 +0000
> +++ linux-2.6.39-rc5/drivers/char/apm-emulation.c	2011-05-03 00:34:05.467660807 +0100
> @@ -275,7 +275,8 @@ apm_ioctl(struct file *filp, u_int cmd, 
>  	if (!as->suser || !as->writer)
>  		return -EPERM;
>  
> -	mutex_lock(&apm_mutex);
> +	while (mutex_lock_interruptible(&apm_mutex) < 0)
> +		try_to_freeze();

Hmm.  I'd rather use mutex_trylock() and call try_to_freeze() if
that fails.

Is there anything wrong with this approach?

>  	switch (cmd) {
>  	case APM_IOC_SUSPEND:
>  		mutex_lock(&state_lock);

Jiri, are you going to maintain apm-emulation.c too?

Thanks,
Rafael

Re: [PATCH] apm-emulation: Fix mutex race condition

[Paul Parsons]

Hi Rafael,

--- On Tue, 3/5/11, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Hmm.  I'd rather use mutex_trylock() and call try_to_freeze() if
> that fails.
> 
> Is there anything wrong with this approach?

Well, mutex_trylock() doesn't wait if it cannot acquire the mutex, so won't the calling process enter a busy loop if it fails?

Regards,
Paul

Re: [PATCH] apm-emulation: Fix mutex race condition

[Rafael J. Wysocki]

On Tuesday, May 03, 2011, Paul Parsons wrote:
> Hi Rafael,
> 
> --- On Tue, 3/5/11, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Hmm.  I'd rather use mutex_trylock() and call try_to_freeze() if
> > that fails.
> > 
> > Is there anything wrong with this approach?
> 
> Well, mutex_trylock() doesn't wait if it cannot acquire the mutex, so won't the calling process enter a busy loop if it fails?

Right.  So I don't see how we could do better that in your patch at the
moment.

I'm not sure if Jiri is going to take your patch.  I will if he doesn't.

Thanks,
Rafael

Re: [PATCH] apm-emulation: Fix mutex race condition

[Jiri Kosina]

On Tue, 3 May 2011, Rafael J. Wysocki wrote:

> > --- On Tue, 3/5/11, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > Hmm.  I'd rather use mutex_trylock() and call try_to_freeze() if
> > > that fails.
> > > 
> > > Is there anything wrong with this approach?
> > 
> > Well, mutex_trylock() doesn't wait if it cannot acquire the mutex, so 
> > won't the calling process enter a busy loop if it fails?
> 
> Right.  So I don't see how we could do better that in your patch at the
> moment.

Right, neither do I.

> I'm not sure if Jiri is going to take your patch.  I will if he doesn't.

I can add drivers/char/apm-emulation.c as the one maintained under the 
'APM' roof, and take this, no problem.

Rafael, I take it that I can add your Ack to this patch, right?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] apm-emulation: Fix mutex race condition

[Rafael J. Wysocki]

On Wednesday, May 04, 2011, Jiri Kosina wrote:
> On Tue, 3 May 2011, Rafael J. Wysocki wrote:
> 
> > > --- On Tue, 3/5/11, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > Hmm.  I'd rather use mutex_trylock() and call try_to_freeze() if
> > > > that fails.
> > > > 
> > > > Is there anything wrong with this approach?
> > > 
> > > Well, mutex_trylock() doesn't wait if it cannot acquire the mutex, so 
> > > won't the calling process enter a busy loop if it fails?
> > 
> > Right.  So I don't see how we could do better that in your patch at the
> > moment.
> 
> Right, neither do I.
> 
> > I'm not sure if Jiri is going to take your patch.  I will if he doesn't.
> 
> I can add drivers/char/apm-emulation.c as the one maintained under the 
> 'APM' roof, and take this, no problem.
> 
> Rafael, I take it that I can add your Ack to this patch, right?

That's correct.

Thanks,
Rafael

Re: [PATCH] apm-emulation: Fix mutex race condition

[Jiri Kosina]

On Wed, 4 May 2011, Rafael J. Wysocki wrote:

> > I can add drivers/char/apm-emulation.c as the one maintained under the 
> > 'APM' roof, and take this, no problem.
> > 
> > Rafael, I take it that I can add your Ack to this patch, right?
> 
> That's correct.

Applied, thanks everybody.

-- 
Jiri Kosina
SUSE Labs