[patch v2] [media] bttv: take correct lock in bttv_open()

[patch v2] [media] bttv: take correct lock in bttv_open()

[Dan Carpenter]

We're trying to make sure that no one is writing to the btv->init struct
while we copy it over to the newly allocated "fh" struct.  The original
code doesn't make sense because "fh->cap.vb_lock" hasn't been
initialized and no one else can be writing to it anyway.

Addresses: https://bugzilla.kernel.org/show_bug.cgi?id=24602 

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
Sergej could you test this one?

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index a529619..6c8f4b0 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -3302,9 +3302,9 @@ static int bttv_open(struct file *file)
 	 * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
 	 * with the rest of init, holding btv->lock.
 	 */
-	mutex_lock(&fh->cap.vb_lock);
+	mutex_lock(&btv->init.cap.vb_lock);
 	*fh = btv->init;
-	mutex_unlock(&fh->cap.vb_lock);
+	mutex_unlock(&btv->init.cap.vb_lock);
 
 	fh->type = type;
 	fh->ov.setup_ok = 0;

Re: [patch v2] [media] bttv: take correct lock in bttv_open()

[Sergej Pupykin]

On 12.12.2010 19:58, Dan Carpenter wrote:
> We're trying to make sure that no one is writing to the btv->init struct
> while we copy it over to the newly allocated "fh" struct.  The original
> code doesn't make sense because "fh->cap.vb_lock" hasn't been
> initialized and no one else can be writing to it anyway.
>
This patch also crashes the system. Unfortunately machine hangs, so I 
can not copy-paste trace. It was something about nosemaphore called from 
bttv_open. (something like previous reports)

I replace lock with btv->lock:

mutex_lock(&btv->lock);
*fh = btv->init;
mutex_unlock(&btv->lock);

Probably it is overkill and may be incorrect, but it starts working.

Also I found another issue: tvtime hangs on exit in D-state, so it looks 
like there is a problem near bttv_release function or something like this.

Re: [patch v2] [media] bttv: take correct lock in bttv_open()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

On Mon, Dec 13, 2010 at 01:42:49AM +0300, Sergej Pupykin wrote:
> mutex_lock(&btv->lock);
> *fh = btv->init;
> mutex_unlock(&btv->lock);
> 
> Probably it is overkill and may be incorrect, but it starts working.
>

Mauro would be the one to know for sure.
 
> Also I found another issue: tvtime hangs on exit in D-state, so it
> looks like there is a problem near bttv_release function or
> something like this.

Speaking of other bugs in this driver, I submitted a another fix
that hasn't been merged yet.  I've attached it.  Don't know if it's
related at all to the other bug you noticed but it can't hurt.

regards,
dan carpenter

[-- Attachment #2: bt8xx.diff --]
[-- Type: text/x-diff, Size: 1434 bytes --]

>From error27@gmail.com Thu Nov 18 07:19:15 2010
Date: Thu, 18 Nov 2010 06:55:59 +0300
From: Dan Carpenter <error27@gmail.com>
To: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: linux-media@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: [patch] [media] bt8xx: missing unlock in bttv_overlay()
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=utf-8
Status: RO

There is a missing unlock here.  This was introduced as part of BKL
removal in c37db91fd0d4 "V4L/DVB: bttv: fix driver lock and remove
explicit calls to BKL"

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index 3da6e80..aca755c 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -2779,16 +2779,14 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on)
 		mutex_lock(&fh->cap.vb_lock);
 		/* verify args */
 		if (unlikely(!btv->fbuf.base)) {
-			mutex_unlock(&fh->cap.vb_lock);
-			return -EINVAL;
-		}
-		if (unlikely(!fh->ov.setup_ok)) {
+			retval = -EINVAL;
+		} else if (unlikely(!fh->ov.setup_ok)) {
 			dprintk("bttv%d: overlay: !setup_ok\n", btv->c.nr);
 			retval = -EINVAL;
 		}
+		mutex_unlock(&fh->cap.vb_lock);
 		if (retval)
 			return retval;
-		mutex_unlock(&fh->cap.vb_lock);
 	}
 
 	if (!check_alloc_btres_lock(btv, fh, RESOURCE_OVERLAY))

Re: [patch v2] [media] bttv: take correct lock in bttv_open()

[Mauro Carvalho Chehab]

Em 14-12-2010 08:36, Dan Carpenter escreveu:
> On Mon, Dec 13, 2010 at 01:42:49AM +0300, Sergej Pupykin wrote:
>> mutex_lock(&btv->lock);
>> *fh = btv->init;
>> mutex_unlock(&btv->lock);
>>
>> Probably it is overkill and may be incorrect, but it starts working.
>>
> 
> Mauro would be the one to know for sure.
>  
>> Also I found another issue: tvtime hangs on exit in D-state, so it
>> looks like there is a problem near bttv_release function or
>> something like this.
> 
> Speaking of other bugs in this driver, I submitted a another fix
> that hasn't been merged yet.  I've attached it.  Don't know if it's
> related at all to the other bug you noticed but it can't hurt.

I'm preparing one machine in order to test bttv and try to fix the 
locking issues. Hopefully, I'll have something today.

Cheers,
Mauro