[PATCH] Fix open/close race in saa7134

[PATCH] Fix open/close race in saa7134

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Date: Sun, 22 Jun 2008 10:03:02 -0700
Subject: [PATCH] Fix open/close race in saa7134

The saa7134 driver uses a (non-atomic) variable in an attempt to
only allow one opener of the device (how it deals with sending
the fd over unix sockets I don't know).

Unfortunately, the release function first decrements this variable,
and THEN goes on to disable more of the device. This allows for
a race where another opener of the device comes in after the decrement of
the variable, configures the hardware just to then see the hardware
be disabled by the rest of the release function.

This patch makes the release function use the same lock as the open
function to protect the hardware as well as the variable (which now
at least has some locking to protect it).

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/media/video/saa7134/saa7134-empress.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/saa7134/saa7134-empress.c b/drivers/media/video/saa7134/saa7134-empress.c
index 81431ee..9108843 100644
--- a/drivers/media/video/saa7134/saa7134-empress.c
+++ b/drivers/media/video/saa7134/saa7134-empress.c
@@ -110,6 +110,8 @@ static int ts_release(struct inode *inode, struct file *file)
 {
 	struct saa7134_dev *dev = file->private_data;
 
+	mutex_lock(&dev->empress_tsq.vb_lock);
+
 	videobuf_stop(&dev->empress_tsq);
 	videobuf_mmap_free(&dev->empress_tsq);
 	dev->empress_users--;
@@ -121,6 +123,8 @@ static int ts_release(struct inode *inode, struct file *file)
 	saa_writeb(SAA7134_AUDIO_MUTE_CTRL,
 		saa_readb(SAA7134_AUDIO_MUTE_CTRL) | (1 << 6));
 
+	mutex_unlock(&dev->empress_tsq.vb_lock);
+
 	return 0;
 }
 
-- 
1.5.5.1



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] Fix open/close race in saa7134

[Marcin Slusarz]

On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
> 
> From: Arjan van de Ven <arjan@linux.intel.com>
> Date: Sun, 22 Jun 2008 10:03:02 -0700
> Subject: [PATCH] Fix open/close race in saa7134
> 
> The saa7134 driver uses a (non-atomic) variable in an attempt to
> only allow one opener of the device (how it deals with sending
> the fd over unix sockets I don't know).
> 
> Unfortunately, the release function first decrements this variable,
> and THEN goes on to disable more of the device. This allows for
> a race where another opener of the device comes in after the decrement of
> the variable, configures the hardware just to then see the hardware
> be disabled by the rest of the release function.

Simplier fix:
http://lkml.org/lkml/2008/6/9/308
But I don't remember whether it was applied or not...

> 
> This patch makes the release function use the same lock as the open
> function to protect the hardware as well as the variable (which now
> at least has some locking to protect it).
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  drivers/media/video/saa7134/saa7134-empress.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/saa7134/saa7134-empress.c b/drivers/media/video/saa7134/saa7134-empress.c
> index 81431ee..9108843 100644
> --- a/drivers/media/video/saa7134/saa7134-empress.c
> +++ b/drivers/media/video/saa7134/saa7134-empress.c
> @@ -110,6 +110,8 @@ static int ts_release(struct inode *inode, struct file *file)
>  {
>  	struct saa7134_dev *dev = file->private_data;
>  
> +	mutex_lock(&dev->empress_tsq.vb_lock);
> +
>  	videobuf_stop(&dev->empress_tsq);
>  	videobuf_mmap_free(&dev->empress_tsq);
>  	dev->empress_users--;
> @@ -121,6 +123,8 @@ static int ts_release(struct inode *inode, struct file *file)
>  	saa_writeb(SAA7134_AUDIO_MUTE_CTRL,
>  		saa_readb(SAA7134_AUDIO_MUTE_CTRL) | (1 << 6));
>  
> +	mutex_unlock(&dev->empress_tsq.vb_lock);
> +
>  	return 0;
>  }
>  
> -- 
> 1.5.5.1

PS: I can't access 2.6.25 oopses anymore (timeout). Can you fix it?
http://kerneloops.org/version.php?start=1671168&end=1703935&version=25-release&count=4509

Marcin

Re: [PATCH] Fix open/close race in saa7134

[Mauro Carvalho Chehab]

Marcin,

> Simplier fix:
> http://lkml.org/lkml/2008/6/9/308
> But I don't remember whether it was applied or not...

I've applied your patch at my -git. I'll probably send later today to upstream.

Anyway, it seems valuable to use the same mutex also here, so I'm committing
Arjan patch as well.

Cheers,
Mauro

Re: [PATCH] Fix open/close race in saa7134

[Arjan van de Ven]

On Sun, 22 Jun 2008 19:33:37 +0200
Marcin Slusarz <marcin.slusarz@gmail.com> wrote:

> PS: I can't access 2.6.25 oopses anymore (timeout). Can you fix it?
> http://kerneloops.org/version.php?start=1671168&end=1703935&version=25-release&count=4509

hmmm... seems the database server got a bit overloaded.
I just added a new index that should speed this page up; and it now
works for me again..... (sigh.. I'm a kernel guy not a DBA, the things
one must do to be a kernel guy)

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] Fix open/close race in saa7134

[Arjan van de Ven]

On Sun, 22 Jun 2008 19:33:37 +0200
Marcin Slusarz <marcin.slusarz@gmail.com> wrote:

> On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
> > 
> > From: Arjan van de Ven <arjan@linux.intel.com>
> > Date: Sun, 22 Jun 2008 10:03:02 -0700
> > Subject: [PATCH] Fix open/close race in saa7134
> > 
> > The saa7134 driver uses a (non-atomic) variable in an attempt to
> > only allow one opener of the device (how it deals with sending
> > the fd over unix sockets I don't know).
> > 
> > Unfortunately, the release function first decrements this variable,
> > and THEN goes on to disable more of the device. This allows for
> > a race where another opener of the device comes in after the
> > decrement of the variable, configures the hardware just to then see
> > the hardware be disabled by the rest of the release function.
> 
> Simplier fix:
> http://lkml.org/lkml/2008/6/9/308
> But I don't remember whether it was applied or not...


the patch might be simpler, but it's not fully correct...
the decrement is non-atomic and not protected by any lock whatsoever.

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] Fix open/close race in saa7134

[Marcin Slusarz]

On Sun, Jun 22, 2008 at 10:58:32AM -0700, Arjan van de Ven wrote:
> On Sun, 22 Jun 2008 19:33:37 +0200
> Marcin Slusarz <marcin.slusarz@gmail.com> wrote:
> 
> > On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
> > > 
> > > From: Arjan van de Ven <arjan@linux.intel.com>
> > > Date: Sun, 22 Jun 2008 10:03:02 -0700
> > > Subject: [PATCH] Fix open/close race in saa7134
> > > 
> > > The saa7134 driver uses a (non-atomic) variable in an attempt to
> > > only allow one opener of the device (how it deals with sending
> > > the fd over unix sockets I don't know).
> > > 
> > > Unfortunately, the release function first decrements this variable,
> > > and THEN goes on to disable more of the device. This allows for
> > > a race where another opener of the device comes in after the
> > > decrement of the variable, configures the hardware just to then see
> > > the hardware be disabled by the rest of the release function.
> > 
> > Simplier fix:
> > http://lkml.org/lkml/2008/6/9/308
> > But I don't remember whether it was applied or not...
> 
> 
> the patch might be simpler, but it's not fully correct...
> the decrement is non-atomic and not protected by any lock whatsoever.

It's not atomic, but it don't have to, because there's only one thread which
"owns" the device. But I agree that having a mutex here makes locking more obvious.

PS: Thanks for fixing kerneloops.org.

Marcin

Re: [PATCH] Fix open/close race in saa7134

[Arjan van de Ven]

On Mon, 23 Jun 2008 20:49:42 +0200
Marcin Slusarz <marcin.slusarz@gmail.com> wrote:

> On Sun, Jun 22, 2008 at 10:58:32AM -0700, Arjan van de Ven wrote:
> > On Sun, 22 Jun 2008 19:33:37 +0200
> > Marcin Slusarz <marcin.slusarz@gmail.com> wrote:
> > 
> > > On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
> > > > 
> > > > From: Arjan van de Ven <arjan@linux.intel.com>
> > > > Date: Sun, 22 Jun 2008 10:03:02 -0700
> > > > Subject: [PATCH] Fix open/close race in saa7134
> > > > 
> > > > The saa7134 driver uses a (non-atomic) variable in an attempt to
> > > > only allow one opener of the device (how it deals with sending
> > > > the fd over unix sockets I don't know).
> > > > 
> > > > Unfortunately, the release function first decrements this
> > > > variable, and THEN goes on to disable more of the device. This
> > > > allows for a race where another opener of the device comes in
> > > > after the decrement of the variable, configures the hardware
> > > > just to then see the hardware be disabled by the rest of the
> > > > release function.
> > > 
> > > Simplier fix:
> > > http://lkml.org/lkml/2008/6/9/308
> > > But I don't remember whether it was applied or not...
> > 
> > 
> > the patch might be simpler, but it's not fully correct...
> > the decrement is non-atomic and not protected by any lock
> > whatsoever.
> 
> It's not atomic, but it don't have to, because there's only one
> thread which "owns" the device.

this is not correct!

this is a close->open race, not an open->close!
which means the guy who's closing it and the guy who's then opening it
again do not have to be the same guy

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] Fix open/close race in saa7134

[Marcin Slusarz]

On Mon, Jun 23, 2008 at 11:54:32AM -0700, Arjan van de Ven wrote:
> On Mon, 23 Jun 2008 20:49:42 +0200
> Marcin Slusarz <marcin.slusarz@gmail.com> wrote:
> 
> > On Sun, Jun 22, 2008 at 10:58:32AM -0700, Arjan van de Ven wrote:
> > > On Sun, 22 Jun 2008 19:33:37 +0200
> > > Marcin Slusarz <marcin.slusarz@gmail.com> wrote:
> > > 
> > > > On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
> > > > > 
> > > > > From: Arjan van de Ven <arjan@linux.intel.com>
> > > > > Date: Sun, 22 Jun 2008 10:03:02 -0700
> > > > > Subject: [PATCH] Fix open/close race in saa7134
> > > > > 
> > > > > The saa7134 driver uses a (non-atomic) variable in an attempt to
> > > > > only allow one opener of the device (how it deals with sending
> > > > > the fd over unix sockets I don't know).
> > > > > 
> > > > > Unfortunately, the release function first decrements this
> > > > > variable, and THEN goes on to disable more of the device. This
> > > > > allows for a race where another opener of the device comes in
> > > > > after the decrement of the variable, configures the hardware
> > > > > just to then see the hardware be disabled by the rest of the
> > > > > release function.
> > > > 
> > > > Simplier fix:
> > > > http://lkml.org/lkml/2008/6/9/308
> > > > But I don't remember whether it was applied or not...
> > > 
> > > 
> > > the patch might be simpler, but it's not fully correct...
> > > the decrement is non-atomic and not protected by any lock
> > > whatsoever.
> > 
> > It's not atomic, but it don't have to, because there's only one
> > thread which "owns" the device.
> 
> this is not correct!
> 
> this is a close->open race, not an open->close!
> which means the guy who's closing it and the guy who's then opening it
> again do not have to be the same guy

If dev->empress_users could be > 1 then ok - it could break, but it can only be 1 or 0.
If it's 1 you won't open the device. If it's 0 you won't reach ts_close.

If you still see the race, please show me the sequence, because I don't
(of course when decrementing is the last operation of ts_close).

Marcin

Re: [PATCH] Fix open/close race in saa7134

[Arjan van de Ven]

On Mon, 23 Jun 2008 21:22:03 +0200
Marcin Slusarz <marcin.slusarz@gmail.com> wrote:

> 
> If dev->empress_users could be > 1 then ok - it could break, but it
> can only be 1 or 0. If it's 1 you won't open the device. If it's 0
> you won't reach ts_close.
> 
> If you still see the race, please show me the sequence, because I
> don't (of course when decrementing is the last operation of ts_close).

a decrement in C, without locking, is NOT atomic.
It in fact is 
"load variable into register"
"increment register value"
"write register to variable"

which the compiler then takes and orders for maximum performance....



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] Fix open/close race in saa7134

[Marcin Slusarz]

On Mon, Jun 23, 2008 at 12:53:48PM -0700, Arjan van de Ven wrote:
> On Mon, 23 Jun 2008 21:22:03 +0200
> Marcin Slusarz <marcin.slusarz@gmail.com> wrote:
> 
> > 
> > If dev->empress_users could be > 1 then ok - it could break, but it
> > can only be 1 or 0. If it's 1 you won't open the device. If it's 0
> > you won't reach ts_close.
> > 
> > If you still see the race, please show me the sequence, because I
> > don't (of course when decrementing is the last operation of ts_close).
> 
> a decrement in C, without locking, is NOT atomic.

I know about it. But in this code, 2 threads cannot modify empress_users!
This variable should be named empress_used_now or something like that.

Look:

static int ts_open(struct inode *inode, struct file *file)
{
	(...)
	err = -EBUSY;
	if (!mutex_trylock(&dev->empress_tsq.vb_lock))
		goto done;
	if (dev->empress_users)                 <-------------
		goto done_up;

	/* Unmute audio */
	saa_writeb(SAA7134_AUDIO_MUTE_CTRL,
		saa_readb(SAA7134_AUDIO_MUTE_CTRL) & ~(1 << 6));

	dev->empress_users++;    <------ this should be "dev->empress_users = 1;"
	file->private_data = dev;
	err = 0;

done_up:
	mutex_unlock(&dev->empress_tsq.vb_lock);
done:
	return err;
}

static int ts_release(struct inode *inode, struct file *file)
{
	(...)
	dev->empress_users--;   <------------ this should be "dev->empress_users = 0;"
	return 0;
}