Skip to content

Commit

Permalink
V4L/DVB: gspca - main: Don't use the frame buffer flags
Browse files Browse the repository at this point in the history 
This patch fixes possible race conditions in queue management with SMP:
when a frame was completed, the irq function tried to use the next frame
buffer. At this time, it was possible that the application on an other
processor updated the frame pointer, making the image to point to a bad
buffer.
The patch contains two main changes:
- the image transfer uses the queue indexes which are protected against
  simultaneous memory access,
- the image pointer which is used for image concatenation is only set at
  interrupt level.
Some subdrivers which used the image pointer have been updated.

Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Jean-François Moine <moinejf@free.fr>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
  • Loading branch information
Jean-François Moine authored and Mauro Carvalho Chehab committed Aug 2, 2010
1 parent 02bbcb9 commit f7059ea
Show file tree
Hide file tree
Showing 9 changed files with 58 additions and 106 deletions.
10 changes: 3 additions & 7 deletions drivers/media/video/gspca/cpia1.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1765,14 +1765,10 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
atomic_set(&sd->cam_exposure, data[39] * 2);
atomic_set(&sd->fps, data[41]);

image = gspca_dev->image;
if (image == NULL) {
gspca_dev->last_packet_type = DISCARD_PACKET;
return;
}

/* Check for proper EOF for last frame */
if (gspca_dev->image_len > 4 &&
image = gspca_dev->image;
if (image != NULL &&
gspca_dev->image_len > 4 &&
image[gspca_dev->image_len - 4] == 0xff &&
image[gspca_dev->image_len - 3] == 0xff &&
image[gspca_dev->image_len - 2] == 0xff &&
Expand Down
112 changes: 46 additions & 66 deletions drivers/media/video/gspca/gspca.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,8 +315,6 @@ static void fill_frame(struct gspca_dev *gspca_dev,
urb->status = 0;
goto resubmit;
}
if (gspca_dev->image == NULL)
gspca_dev->last_packet_type = DISCARD_PACKET;
pkt_scan = gspca_dev->sd_desc->pkt_scan;
for (i = 0; i < urb->number_of_packets; i++) {

Expand Down Expand Up @@ -428,16 +426,19 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,

PDEBUG(D_PACK, "add t:%d l:%d", packet_type, len);

/* check the availability of the frame buffer */
if (gspca_dev->image == NULL)
return;

if (packet_type == FIRST_PACKET) {
i = gspca_dev->fr_i;
i = atomic_read(&gspca_dev->fr_i);

/* if there are no queued buffer, discard the whole frame */
if (i == atomic_read(&gspca_dev->fr_q)) {
gspca_dev->last_packet_type = DISCARD_PACKET;
return;
}
j = gspca_dev->fr_queue[i];
frame = &gspca_dev->frame[j];
frame->v4l2_buf.timestamp = ktime_to_timeval(ktime_get());
frame->v4l2_buf.sequence = ++gspca_dev->sequence;
gspca_dev->image = frame->data;
gspca_dev->image_len = 0;
} else if (gspca_dev->last_packet_type == DISCARD_PACKET) {
if (packet_type == LAST_PACKET)
Expand All @@ -460,31 +461,24 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
}
gspca_dev->last_packet_type = packet_type;

/* if last packet, wake up the application and advance in the queue */
/* if last packet, invalidate packet concatenation until
* next first packet, wake up the application and advance
* in the queue */
if (packet_type == LAST_PACKET) {
i = gspca_dev->fr_i;
i = atomic_read(&gspca_dev->fr_i);
j = gspca_dev->fr_queue[i];
frame = &gspca_dev->frame[j];
frame->v4l2_buf.bytesused = gspca_dev->image_len;
frame->v4l2_buf.flags = (frame->v4l2_buf.flags
| V4L2_BUF_FLAG_DONE)
& ~V4L2_BUF_FLAG_QUEUED;
i = (i + 1) % GSPCA_MAX_FRAMES;
atomic_set(&gspca_dev->fr_i, i);
wake_up_interruptible(&gspca_dev->wq); /* event = new frame */
i = (i + 1) % gspca_dev->nframes;
gspca_dev->fr_i = i;
PDEBUG(D_FRAM, "frame complete len:%d q:%d i:%d o:%d",
frame->v4l2_buf.bytesused,
gspca_dev->fr_q,
i,
gspca_dev->fr_o);
j = gspca_dev->fr_queue[i];
frame = &gspca_dev->frame[j];
if ((frame->v4l2_buf.flags & BUF_ALL_FLAGS)
== V4L2_BUF_FLAG_QUEUED) {
gspca_dev->image = frame->data;
} else {
gspca_dev->image = NULL;
}
PDEBUG(D_FRAM, "frame complete len:%d",
frame->v4l2_buf.bytesused);
gspca_dev->image = NULL;
gspca_dev->image_len = 0;
}
}
EXPORT_SYMBOL(gspca_frame_add);
Expand Down Expand Up @@ -514,8 +508,8 @@ static int frame_alloc(struct gspca_dev *gspca_dev,
PDEBUG(D_STREAM, "frame alloc frsz: %d", frsz);
frsz = PAGE_ALIGN(frsz);
gspca_dev->frsz = frsz;
if (count > GSPCA_MAX_FRAMES)
count = GSPCA_MAX_FRAMES;
if (count >= GSPCA_MAX_FRAMES)
count = GSPCA_MAX_FRAMES - 1;
gspca_dev->frbuf = vmalloc_32(frsz * count);
if (!gspca_dev->frbuf) {
err("frame alloc failed");
Expand All @@ -534,11 +528,9 @@ static int frame_alloc(struct gspca_dev *gspca_dev,
frame->data = gspca_dev->frbuf + i * frsz;
frame->v4l2_buf.m.offset = i * frsz;
}
gspca_dev->fr_i = gspca_dev->fr_o = gspca_dev->fr_q = 0;
gspca_dev->image = NULL;
gspca_dev->image_len = 0;
gspca_dev->last_packet_type = DISCARD_PACKET;
gspca_dev->sequence = 0;
atomic_set(&gspca_dev->fr_q, 0);
atomic_set(&gspca_dev->fr_i, 0);
gspca_dev->fr_o = 0;
return 0;
}

Expand Down Expand Up @@ -776,6 +768,12 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
goto out;
}

/* reset the streaming variables */
gspca_dev->image = NULL;
gspca_dev->image_len = 0;
gspca_dev->last_packet_type = DISCARD_PACKET;
gspca_dev->sequence = 0;

gspca_dev->usb_err = 0;

/* set the higher alternate setting and
Expand Down Expand Up @@ -1591,7 +1589,7 @@ static int vidioc_streamoff(struct file *file, void *priv,
enum v4l2_buf_type buf_type)
{
struct gspca_dev *gspca_dev = priv;
int i, ret;
int ret;

if (buf_type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
return -EINVAL;
Expand All @@ -1615,12 +1613,10 @@ static int vidioc_streamoff(struct file *file, void *priv,
gspca_stream_off(gspca_dev);
mutex_unlock(&gspca_dev->usb_lock);

/* empty the application queues */
for (i = 0; i < gspca_dev->nframes; i++)
gspca_dev->frame[i].v4l2_buf.flags &= ~BUF_ALL_FLAGS;
gspca_dev->fr_i = gspca_dev->fr_o = gspca_dev->fr_q = 0;
gspca_dev->last_packet_type = DISCARD_PACKET;
gspca_dev->sequence = 0;
/* empty the transfer queues */
atomic_set(&gspca_dev->fr_q, 0);
atomic_set(&gspca_dev->fr_i, 0);
gspca_dev->fr_o = 0;
ret = 0;
out:
mutex_unlock(&gspca_dev->queue_lock);
Expand Down Expand Up @@ -1697,7 +1693,7 @@ static int vidioc_s_parm(struct file *filp, void *priv,
int n;

n = parm->parm.capture.readbuffers;
if (n == 0 || n > GSPCA_MAX_FRAMES)
if (n == 0 || n >= GSPCA_MAX_FRAMES)
parm->parm.capture.readbuffers = gspca_dev->nbufread;
else
gspca_dev->nbufread = n;
Expand Down Expand Up @@ -1800,21 +1796,17 @@ static int dev_mmap(struct file *file, struct vm_area_struct *vma)
static int frame_wait(struct gspca_dev *gspca_dev,
int nonblock_ing)
{
struct gspca_frame *frame;
int i, j, ret;
int i, ret;

/* check if a frame is ready */
i = gspca_dev->fr_o;
j = gspca_dev->fr_queue[i];
frame = &gspca_dev->frame[j];

if (!(frame->v4l2_buf.flags & V4L2_BUF_FLAG_DONE)) {
if (i == atomic_read(&gspca_dev->fr_i)) {
if (nonblock_ing)
return -EAGAIN;

/* wait till a frame is ready */
ret = wait_event_interruptible_timeout(gspca_dev->wq,
(frame->v4l2_buf.flags & V4L2_BUF_FLAG_DONE) ||
i != atomic_read(&gspca_dev->fr_i) ||
!gspca_dev->streaming || !gspca_dev->present,
msecs_to_jiffies(3000));
if (ret < 0)
Expand All @@ -1823,11 +1815,7 @@ static int frame_wait(struct gspca_dev *gspca_dev,
return -EIO;
}

gspca_dev->fr_o = (i + 1) % gspca_dev->nframes;
PDEBUG(D_FRAM, "frame wait q:%d i:%d o:%d",
gspca_dev->fr_q,
gspca_dev->fr_i,
gspca_dev->fr_o);
gspca_dev->fr_o = (i + 1) % GSPCA_MAX_FRAMES;

if (gspca_dev->sd_desc->dq_callback) {
mutex_lock(&gspca_dev->usb_lock);
Expand All @@ -1836,7 +1824,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 j;
return gspca_dev->fr_queue[i];
}

/*
Expand Down Expand Up @@ -1941,15 +1929,9 @@ static int vidioc_qbuf(struct file *file, void *priv,
}

/* put the buffer in the 'queued' queue */
i = gspca_dev->fr_q;
i = atomic_read(&gspca_dev->fr_q);
gspca_dev->fr_queue[i] = index;
if (gspca_dev->fr_i == i)
gspca_dev->image = frame->data;
gspca_dev->fr_q = (i + 1) % gspca_dev->nframes;
PDEBUG(D_FRAM, "qbuf q:%d i:%d o:%d",
gspca_dev->fr_q,
gspca_dev->fr_i,
gspca_dev->fr_o);
atomic_set(&gspca_dev->fr_q, (i + 1) % GSPCA_MAX_FRAMES);

v4l2_buf->flags |= V4L2_BUF_FLAG_QUEUED;
v4l2_buf->flags &= ~V4L2_BUF_FLAG_DONE;
Expand Down Expand Up @@ -2005,7 +1987,7 @@ static int read_alloc(struct gspca_dev *gspca_dev,
static unsigned int dev_poll(struct file *file, poll_table *wait)
{
struct gspca_dev *gspca_dev = file->private_data;
int i, ret;
int ret;

PDEBUG(D_FRAM, "poll");

Expand All @@ -2023,11 +2005,9 @@ static unsigned int dev_poll(struct file *file, poll_table *wait)
if (mutex_lock_interruptible(&gspca_dev->queue_lock) != 0)
return POLLERR;

/* check the next incoming buffer */
i = gspca_dev->fr_o;
i = gspca_dev->fr_queue[i];
if (gspca_dev->frame[i].v4l2_buf.flags & V4L2_BUF_FLAG_DONE)
ret = POLLIN | POLLRDNORM; /* something to read */
/* check if an image has been received */
if (gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i))
ret = POLLIN | POLLRDNORM; /* yes */
else
ret = 0;
mutex_unlock(&gspca_dev->queue_lock);
Expand Down
8 changes: 4 additions & 4 deletions drivers/media/video/gspca/gspca.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,11 @@ struct gspca_dev {
u8 *image; /* image beeing filled */
__u32 frsz; /* frame size */
u32 image_len; /* current length of image */
char nframes; /* number of frames */
char fr_i; /* frame being filled */
char fr_q; /* next frame to queue */
char fr_o; /* next frame to dequeue */
atomic_t fr_q; /* next frame to queue */
atomic_t fr_i; /* frame being filled */
signed char fr_queue[GSPCA_MAX_FRAMES]; /* frame queue */
char nframes; /* number of frames */
u8 fr_o; /* next frame to dequeue */
__u8 last_packet_type;
__s8 empty_packet; /* if (-1) don't check empty packets */
__u8 streaming;
Expand Down
5 changes: 0 additions & 5 deletions drivers/media/video/gspca/m5602/m5602_core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,11 +307,6 @@ static void m5602_urb_complete(struct gspca_dev *gspca_dev,
} else {
int cur_frame_len;

if (gspca_dev->image == NULL) {
gspca_dev->last_packet_type = DISCARD_PACKET;
return;
}

cur_frame_len = gspca_dev->image_len;
/* Remove urb header */
data += 4;
Expand Down
2 changes: 0 additions & 2 deletions drivers/media/video/gspca/ov534.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -988,8 +988,6 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
/* If this packet is marked as EOF, end the frame */
} else if (data[1] & UVC_STREAM_EOF) {
sd->last_pts = 0;
if (gspca_dev->image == NULL)
goto discard;
if (gspca_dev->image_len + len - 12 !=
gspca_dev->width * gspca_dev->height * 2) {
PDEBUG(D_PACK, "wrong sized frame");
Expand Down
10 changes: 3 additions & 7 deletions drivers/media/video/gspca/pac7302.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -835,12 +835,6 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
if (sof) {
int n, lum_offset, footer_length;

image = gspca_dev->image;
if (image == NULL) {
gspca_dev->last_packet_type = DISCARD_PACKET;
return;
}

/* 6 bytes after the FF D9 EOF marker a number of lumination
bytes are send corresponding to different parts of the
image, the 14th and 15th byte after the EOF seem to
Expand All @@ -856,7 +850,9 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
} else {
gspca_frame_add(gspca_dev, INTER_PACKET, data, n);
}
if (gspca_dev->last_packet_type != DISCARD_PACKET

image = gspca_dev->image;
if (image != NULL
&& image[gspca_dev->image_len - 2] == 0xff
&& image[gspca_dev->image_len - 1] == 0xd9)
gspca_frame_add(gspca_dev, LAST_PACKET, NULL, 0);
Expand Down
9 changes: 2 additions & 7 deletions drivers/media/video/gspca/pac7311.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -630,12 +630,6 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
if (sof) {
int n, lum_offset, footer_length;

image = gspca_dev->image;
if (image == NULL) {
gspca_dev->last_packet_type = DISCARD_PACKET;
return;
}

/* 6 bytes after the FF D9 EOF marker a number of lumination
bytes are send corresponding to different parts of the
image, the 14th and 15th byte after the EOF seem to
Expand All @@ -651,7 +645,8 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
} else {
gspca_frame_add(gspca_dev, INTER_PACKET, data, n);
}
if (gspca_dev->last_packet_type != DISCARD_PACKET
image = gspca_dev->image;
if (image != NULL
&& image[gspca_dev->image_len - 2] == 0xff
&& image[gspca_dev->image_len - 1] == 0xd9)
gspca_frame_add(gspca_dev, LAST_PACKET, NULL, 0);
Expand Down
4 changes: 0 additions & 4 deletions drivers/media/video/gspca/sonixb.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1254,10 +1254,6 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
int used;
int size = cam->cam_mode[gspca_dev->curr_mode].sizeimage;

if (gspca_dev->image == NULL) {
gspca_dev->last_packet_type = DISCARD_PACKET;
return;
}
used = gspca_dev->image_len;
if (used + len > size)
len = size - used;
Expand Down
4 changes: 0 additions & 4 deletions drivers/media/video/gspca/vc032x.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3728,10 +3728,6 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
if (sd->bridge == BRIDGE_VC0321) {
int size, l;

if (gspca_dev->image == NULL) {
gspca_dev->last_packet_type = DISCARD_PACKET;
return;
}
l = gspca_dev->image_len;
size = gspca_dev->frsz;
if (len > size - l)
Expand Down

0 comments on commit f7059ea

Please sign in to comment.