v4l: saa7134: fix multiple clients access (and oops)

While looking for a reason of multiple oopses in empress_querycap as reported by kerneloops.org I noticed that only first open of device initializes struct_file->private_data properly. (Closing the device was broken too).

So initialize private_date and free all resources on last close. I think this change will fix oops in empress_querycap.

http://kerneloops.org/guilty.php?guilty=empress_querycap&version=2.6.25-release&start=1671168&end=1703935&class=oops

---

Compile tested only. Please test on real hardware.

---
 drivers/media/video/saa7134/saa7134-empress.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

index 81431ee..e543074 100644
@@ -96,11 +96,11 @@ static int ts_open(struct inode *inode, struct file *file)
 	saa_writeb(SAA7134_AUDIO_MUTE_CTRL,
 		saa_readb(SAA7134_AUDIO_MUTE_CTRL) & ~(1 << 6));
 
-	dev->empress_users++;
-	file->private_data = dev;
 	err = 0;
 
 done_up:
+	dev->empress_users++;
+	file->private_data = dev;
 	mutex_unlock(&dev->empress_tsq.vb_lock);
 done:
 	return err;

@@ -110,16 +110,19 @@ static int ts_release(struct inode *inode, struct file *file)
 {
 	struct saa7134_dev *dev = file->private_data;
 
-	videobuf_stop(&dev->empress_tsq);
-	videobuf_mmap_free(&dev->empress_tsq);
-	dev->empress_users--;
+	mutex_lock(&dev->empress_tsq.vb_lock);
+	if (--dev->empress_users == 0) {
+		videobuf_stop(&dev->empress_tsq);
+		videobuf_mmap_free(&dev->empress_tsq);
 
-	/* stop the encoder */
-	ts_reset_encoder(dev);
+		/* stop the encoder */
+		ts_reset_encoder(dev);
 
-	/* Mute audio */
-	saa_writeb(SAA7134_AUDIO_MUTE_CTRL,
-		saa_readb(SAA7134_AUDIO_MUTE_CTRL) | (1 << 6));
+		/* Mute audio */
+		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.4.5

This patch is stupid. Please ignore.

Marcin

Patch ignored. Yet, it seemed ok to my eyes. We shouldn't stop/deallocate resources if there is still someone using the module.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

But when there's at least one user, dev->empress_users will be bigger than 0, so second call to ts_open will return -EBUSY.
And it lead to conclusion, that there's only one possible user...

Marcin

Ok, I think this one should be considered for inclusion.
I don't think it has anything to do with oops in empress_querycap, but who knows ;)
Compile tested only.
---
From: Marcin Slusarz <[email protected]>
Subject: [PATCH] v4l: saa7134: fix race between opening and closing the device

decrementing dev->empress_users should be done as last action of ts_release, because it sleeps and write access to dev->empress_started is not protected in any way
(additionally closing thread could mute audio after opening thread unmuted it)

---
 drivers/media/video/saa7134/saa7134-empress.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

index 81431ee..0da683a 100644
@@ -112,7 +112,6 @@ static int ts_release(struct inode *inode, struct file *file)
 
 	videobuf_stop(&dev->empress_tsq);
 	videobuf_mmap_free(&dev->empress_tsq);
-	dev->empress_users--;
 
 	/* stop the encoder */
 	ts_reset_encoder(dev);

@@ -121,6 +120,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));
 
+	dev->empress_users--;
+
 	return 0;
 }
 

1.5.4.5

Thanks.

The patch looks sane to my eyes. I'll apply at the tree, in order to allow more people to test it.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list