[media] gspca_main: Locking fixes 2

Before this patch vidioc_dqbuf is using its own read_lock, where as
other queue related functions use queue_lock. This means that dqbuf is
accessing several variables in a racy manor. The most important one
being fr_o, which may be changed from underneath dqbuf by vidioc_reqbufs
or vidioc_streamoff. Other variables which it accesses unprotected
are gspca_dev->memory, gspca_dev->streaming and gspca_dev->capt_file.

This patch fixes this by changing vidioc_dqbuf to also use the queue_lock.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jean-Francois Moine <moinejf@free.fr>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 74626b6a9eb43ac9a7e8bf2c11ce4266017c56a4..64ecf669d655cac10b10e7b99113a7970f3b2b83 100644 (file)
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -1831,33 +1831,77 @@ out:
        return ret;
 }
 
+static int frame_ready_nolock(struct gspca_dev *gspca_dev, struct file *file,
+                               enum v4l2_memory memory)
+{
+       if (!gspca_dev->present)
+               return -ENODEV;
+       if (gspca_dev->capt_file != file || gspca_dev->memory != memory ||
+                       !gspca_dev->streaming)
+               return -EINVAL;
+
+       /* check if a frame is ready */
+       return gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i);
+}
+
+static int frame_ready(struct gspca_dev *gspca_dev, struct file *file,
+                       enum v4l2_memory memory)
+{
+       int ret;
+
+       if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+               return -ERESTARTSYS;
+       ret = frame_ready_nolock(gspca_dev, file, memory);
+       mutex_unlock(&gspca_dev->queue_lock);
+       return ret;
+}
+
 /*
- * wait for a video frame
+ * dequeue a video buffer
  *
- * If a frame is ready, its index is returned.
+ * If nonblock_ing is false, block until a buffer is available.
  */
-static int frame_wait(struct gspca_dev *gspca_dev,
-                       int nonblock_ing)
+static int vidioc_dqbuf(struct file *file, void *priv,
+                       struct v4l2_buffer *v4l2_buf)
 {
-       int i, ret;
+       struct gspca_dev *gspca_dev = priv;
+       struct gspca_frame *frame;
+       int i, j, ret;
 
-       /* check if a frame is ready */
-       i = gspca_dev->fr_o;
-       if (i == atomic_read(&gspca_dev->fr_i)) {
-               if (nonblock_ing)
+       PDEBUG(D_FRAM, "dqbuf");
+
+       if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+               return -ERESTARTSYS;
+
+       for (;;) {
+               ret = frame_ready_nolock(gspca_dev, file, v4l2_buf->memory);
+               if (ret < 0)
+                       goto out;
+               if (ret > 0)
+                       break;
+
+               mutex_unlock(&gspca_dev->queue_lock);
+
+               if (file->f_flags & O_NONBLOCK)
                        return -EAGAIN;
 
                /* wait till a frame is ready */
                ret = wait_event_interruptible_timeout(gspca_dev->wq,
-                       i != atomic_read(&gspca_dev->fr_i) ||
-                       !gspca_dev->streaming || !gspca_dev->present,
+                       frame_ready(gspca_dev, file, v4l2_buf->memory),
                        msecs_to_jiffies(3000));
                if (ret < 0)
                        return ret;
-               if (ret == 0 || !gspca_dev->streaming || !gspca_dev->present)
+               if (ret == 0)
                        return -EIO;
+
+               if (mutex_lock_interruptible(&gspca_dev->queue_lock))
+                       return -ERESTARTSYS;
        }
 
+       i = gspca_dev->fr_o;
+       j = gspca_dev->fr_queue[i];
+       frame = &gspca_dev->frame[j];
+
        gspca_dev->fr_o = (i + 1) % GSPCA_MAX_FRAMES;
 
        if (gspca_dev->sd_desc->dq_callback) {
@@ -1867,46 +1911,7 @@ static int frame_wait(struct gspca_dev *gspca_dev,
                        gspca_dev->sd_desc->dq_callback(gspca_dev);
                mutex_unlock(&gspca_dev->usb_lock);
        }
-       return gspca_dev->fr_queue[i];
-}
-
-/*
- * dequeue a video buffer
- *
- * If nonblock_ing is false, block until a buffer is available.
- */
-static int vidioc_dqbuf(struct file *file, void *priv,
-                       struct v4l2_buffer *v4l2_buf)
-{
-       struct gspca_dev *gspca_dev = priv;
-       struct gspca_frame *frame;
-       int i, ret;
-
-       PDEBUG(D_FRAM, "dqbuf");
-       if (v4l2_buf->memory != gspca_dev->memory)
-               return -EINVAL;
 
-       if (!gspca_dev->present)
-               return -ENODEV;
-
-       /* if not streaming, be sure the application will not loop forever */
-       if (!(file->f_flags & O_NONBLOCK)
-           && !gspca_dev->streaming && gspca_dev->users == 1)
-               return -EINVAL;
-
-       /* only the capturing file may dequeue */
-       if (gspca_dev->capt_file != file)
-               return -EINVAL;
-
-       /* only one dequeue / read at a time */
-       if (mutex_lock_interruptible(&gspca_dev->read_lock))
-               return -ERESTARTSYS;
-
-       ret = frame_wait(gspca_dev, file->f_flags & O_NONBLOCK);
-       if (ret < 0)
-               goto out;
-       i = ret;                                /* frame index */
-       frame = &gspca_dev->frame[i];
        if (gspca_dev->memory == V4L2_MEMORY_USERPTR) {
                if (copy_to_user((__u8 __user *) frame->v4l2_buf.m.userptr,
                                 frame->data,
@@ -1919,10 +1924,10 @@ static int vidioc_dqbuf(struct file *file, void *priv,
        }
        frame->v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
        memcpy(v4l2_buf, &frame->v4l2_buf, sizeof *v4l2_buf);
-       PDEBUG(D_FRAM, "dqbuf %d", i);
+       PDEBUG(D_FRAM, "dqbuf %d", j);
        ret = 0;
 out:
-       mutex_unlock(&gspca_dev->read_lock);
+       mutex_unlock(&gspca_dev->queue_lock);
        return ret;
 }
 
@@ -2270,7 +2275,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
                goto out;
 
        mutex_init(&gspca_dev->usb_lock);
-       mutex_init(&gspca_dev->read_lock);
        mutex_init(&gspca_dev->queue_lock);
        init_waitqueue_head(&gspca_dev->wq);
 

diff --git a/drivers/media/video/gspca/gspca.h b/drivers/media/video/gspca/gspca.h
index 97b77a26a2ebf46736ffe57c116e49bf55b8e3aa..a2a1a6aa0606cfe4d7075cdb6665d9cf13327b18 100644 (file)
--- a/drivers/media/video/gspca/gspca.h
+++ b/drivers/media/video/gspca/gspca.h
@@ -205,7 +205,6 @@ struct gspca_dev {
 
        wait_queue_head_t wq;           /* wait queue */
        struct mutex usb_lock;          /* usb exchange protection */
-       struct mutex read_lock;         /* read protection */
        struct mutex queue_lock;        /* ISOC queue protection */
        int usb_err;                    /* USB error - protected by usb_lock */
        u16 pkt_size;                   /* ISOC packet size */