linux-kernel - Re: [PATCH] bttv: fix mutex use before init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 17 Dec 2010 08:07:24 -0800
From:	Brandon Philips <brandon@...p.org>
To:	Mauro Carvalho Chehab <mchehab@...radead.org>
Cc:	chris2553@...glemail.com,
	Torsten Kaiser <just.for.lkml@...glemail.com>,
	Dave Young <hidave.darkstar@...il.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	Guennadi Liakhovetski <g.liakhovetski@....de>
Subject: Re: [PATCH] bttv: fix mutex use before init

On 19:45 Wed 15 Dec 2010, Mauro Carvalho Chehab wrote:
> Em 15-12-2010 16:44, Chris Clayton escreveu:
> > On Tuesday 14 December 2010, Brandon Philips wrote:
> >> On 17:13 Sun 12 Dec 2010, Torsten Kaiser wrote:
> >>>  * change &fh->cap.vb_lock in bttv_open() AND radio_open() to
> >>>  &btv->init.cap.vb_lock * add a mutex_init(&btv->init.cap.vb_lock)
> >>>  to the setup of init in bttv_probe()
> >>
> >> That seems like a reasonable suggestion. An openSUSE user submitted
> >> this bug to our tracker too. Here is the patch I am having him
> >> test.
> >>
> >> Would you mind testing it?
> >>
> >> From 456dc0ce36db523c4c0c8a269f4eec43a72de1dc Mon Sep 17 00:00:00
> >> 2001 From: Brandon Philips <bphilips@...e.de> Date: Mon, 13 Dec
> >> 2010 16:21:55 -0800 Subject: [PATCH] bttv: fix locking for
> >> btv->init
> >>
> >> Fix locking for the btv->init by using btv->init.cap.vb_lock and in
> >> the process fix uninitialized deref introduced in c37db91fd0d.
> >>
> >> Signed-off-by: Brandon Philips <bphilips@...e.de>
> 
> While your patch fixes the issue, it has some other troubles, like to
> the presence of lock code at free_btres_lock(). It is possible to fix,
> but the better is to just use the core-assisted locking schema. This
> way, V4L2 core will serialize access to all
> ioctl's/open/close/mmap/read/poll operations, avoiding to have two
> processes accessing the hardware at the same time. Also, as there's
> just one lock, instead of 3, there's no risk of dead locks.

Thanks, but, why wasn't this done instead of c37db91f?

Will this make it in before 2.6.37 is released? Otherwise 2.6.37 will
need to be fixed in -stable immediatly after release.

> The net result is a cleaner code, with just one lock.

Could you take this patch to remove all of the comments about locking
order with btv->lock since it doesn't seem to matter any longer.

Cheers,

	Brandon

P.S. Your mail client creates really long lines- somewhere around 90
characters. Could you fix that?

>From 7643db7bf5e9e557a27e3783786a1abecbdf82a7 Mon Sep 17 00:00:00 2001
From: Brandon Philips <brandon@...p.org>
Date: Fri, 17 Dec 2010 07:58:22 -0800
Subject: [PATCH] bttv: remove unneeded locking comments

After Mauro's "bttv: Fix locking issues due to BKL removal code" there
are a number of comments that are no longer needed about lock ordering.
Remove them.

Signed-off-by: Brandon Philips <bphilips@...e.de>
---
 drivers/media/video/bt8xx/bttv-driver.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index 25e1ca0..0902ec0 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -2358,13 +2358,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv,
 	fh->ov.field    = win->field;
 	fh->ov.setup_ok = 1;
 
-	/*
-	 * FIXME: btv is protected by btv->lock mutex, while btv->init
-	 *	  is protected by fh->cap.vb_lock. This seems to open the
-	 *	  possibility for some race situations. Maybe the better would
-	 *	  be to unify those locks or to use another way to store the
-	 *	  init values that will be consumed by videobuf callbacks
-	 */
 	btv->init.ov.w.width   = win->w.width;
 	btv->init.ov.w.height  = win->w.height;
 	btv->init.ov.field     = win->field;
@@ -3219,15 +3212,6 @@ static int bttv_open(struct file *file)
 		return -ENOMEM;
 	file->private_data = fh;
 
-	/*
-	 * btv is protected by btv->lock mutex, while btv->init and other
-	 * streaming vars are protected by fh->cap.vb_lock. We need to take
-	 * care of both locks to avoid troubles. However, vb_lock is used also
-	 * inside videobuf, without calling buf->lock. So, it is a very bad
-	 * idea to hold both locks at the same time.
-	 * Let's first copy btv->init at fh, holding cap.vb_lock, and then work
-	 * with the rest of init, holding btv->lock.
-	 */
 	*fh = btv->init;
 
 	fh->type = type;
@@ -3302,10 +3286,6 @@ static int bttv_release(struct file *file)
 
 	/* free stuff */
 
-	/*
-	 * videobuf uses cap.vb_lock - we should avoid holding btv->lock,
-	 * otherwise we may have dead lock conditions
-	 */
 	videobuf_mmap_free(&fh->cap);
 	videobuf_mmap_free(&fh->vbi);
 	v4l2_prio_close(&btv->prio, fh->prio);
-- 
1.7.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/