From: Arjan van de Ven <[email protected]>
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 <[email protected]>
---
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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
>
> From: Arjan van de Ven <[email protected]>
> 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 <[email protected]>
> ---
> 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
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
On Sun, 22 Jun 2008 19:33:37 +0200
Marcin Slusarz <[email protected]> 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Sun, 22 Jun 2008 19:33:37 +0200
Marcin Slusarz <[email protected]> wrote:
> On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
> >
> > From: Arjan van de Ven <[email protected]>
> > 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
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 <[email protected]> wrote:
>
> > On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
> > >
> > > From: Arjan van de Ven <[email protected]>
> > > 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
On Mon, 23 Jun 2008 20:49:42 +0200
Marcin Slusarz <[email protected]> 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 <[email protected]> wrote:
> >
> > > On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
> > > >
> > > > From: Arjan van de Ven <[email protected]>
> > > > 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
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 <[email protected]> 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 <[email protected]> wrote:
> > >
> > > > On Sun, Jun 22, 2008 at 10:05:07AM -0700, Arjan van de Ven wrote:
> > > > >
> > > > > From: Arjan van de Ven <[email protected]>
> > > > > 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
On Mon, 23 Jun 2008 21:22:03 +0200
Marcin Slusarz <[email protected]> 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 [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
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 <[email protected]> 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;
}