Re: [PATCH] bttv: fix mutex use before init

[Dave Young <hidave.darkstar@gmail.com>]

On Sun, Dec 12, 2010 at 05:13:47PM +0100, Torsten Kaiser wrote:
> On Sun, Dec 12, 2010 at 2:15 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> > oops happen in bttv_open while locking uninitialized mutex fh->cap.vb_lock
> > add mutex_init before usage
> 
> I have seen the same problem twice since I switched of the BKL in
> 2.6.37-rc2, but only had the time today to search for the cause.
> (It only happend on two boots out of probably more then 50, so I only
> investigated after it happened a second time with -rc5)
> 
> The cause was making this change to bttv_open() and radio_open() to
> remove the BKL from them:
> +	mutex_lock(&fh->cap.vb_lock);
>  	*fh = btv->init;
> +	mutex_unlock(&fh->cap.vb_lock);
> 
> This is wrong because of two things:
> First: The fh->cap.vb_lock has not been initialised, as you noted.
> Second: The middle line will overwrite the mutex!
> 
> Just adding mutex_init() will not really help the second problem and
> seems to be wrong from the point that cap.vb_lock already has a
> mutex_init() via videobuf_queue_sg_init().

Yes, you are right. I wonder if the above lock code in bttv_open and radio_open can be removed or just use btv->lock.

> 
> I'm not sure what the correct fix is, but I would rather suggest this:
> 
>  * 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()

I will test this when the bttv card is available for me tommorrow.

> 
> > Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
> > Tested-by: Chris Clayton <chris2553@googlemail.com>
> > ---
> >  drivers/media/video/bt8xx/bttv-driver.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > --- linux-2.6.orig/drivers/media/video/bt8xx/bttv-driver.c      2010-11-27 11:21:30.000000000 +0800
> > +++ linux-2.6/drivers/media/video/bt8xx/bttv-driver.c   2010-12-12 16:31:39.633333338 +0800
> > @@ -3291,6 +3291,8 @@ static int bttv_open(struct file *file)
> >        fh = kmalloc(sizeof(*fh), GFP_KERNEL);
> >        if (unlikely(!fh))
> >                return -ENOMEM;
> > +
> > +       mutex_init(&fh->cap.vb_lock);
> >        file->private_data = fh;
> >
> >        /*
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >