| Message ID | 20181119110903.24383-4-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vb2: fix syzkaller race conditions | expand |
Hi Hans, On Mon, Nov 19, 2018 at 8:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > If queue->lock is different from the video_device lock, then > you need to serialize queue_setup with VIDIOC_S_FMT, and this > should be done by the driver. > > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> > --- > .../media/common/videobuf2/videobuf2-core.c | 51 +++++++++++++------ > include/media/videobuf2-core.h | 19 +++++++ > 2 files changed, 55 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index f7e7e633bcd7..269485920beb 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -465,7 +465,8 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) > */ > if (q->num_buffers) { > bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming || > - q->cnt_wait_prepare != q->cnt_wait_finish; > + q->cnt_wait_prepare != q->cnt_wait_finish || > + q->cnt_queue_setup_lock != q->cnt_queue_setup_unlock; > > if (unbalanced || debug) { > pr_info("counters for queue %p:%s\n", q, > @@ -473,10 +474,14 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) > pr_info(" setup: %u start_streaming: %u stop_streaming: %u\n", > q->cnt_queue_setup, q->cnt_start_streaming, > q->cnt_stop_streaming); > + pr_info(" queue_setup_lock: %u queue_setup_unlock: %u\n", > + q->cnt_queue_setup_lock, q->cnt_queue_setup_unlock); > pr_info(" wait_prepare: %u wait_finish: %u\n", > q->cnt_wait_prepare, q->cnt_wait_finish); > } > q->cnt_queue_setup = 0; > + q->cnt_queue_setup_lock = 0; > + q->cnt_queue_setup_unlock = 0; > q->cnt_wait_prepare = 0; > q->cnt_wait_finish = 0; > q->cnt_start_streaming = 0; > @@ -717,6 +722,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > q->memory = memory; > + call_void_qop(q, queue_setup_lock, q); > > /* > * Ask the driver how many buffers and planes per buffer it requires. > @@ -725,22 +731,27 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > ret = call_qop(q, queue_setup, q, &num_buffers, &num_planes, > plane_sizes, q->alloc_devs); > if (ret) > - return ret; > + goto unlock; > > /* Check that driver has set sane values */ > - if (WARN_ON(!num_planes)) > - return -EINVAL; > + if (WARN_ON(!num_planes)) { > + ret = -EINVAL; > + goto unlock; > + } > > for (i = 0; i < num_planes; i++) > - if (WARN_ON(!plane_sizes[i])) > - return -EINVAL; > + if (WARN_ON(!plane_sizes[i])) { > + ret = -EINVAL; > + goto unlock; > + } > > /* Finally, allocate buffers and video memory */ > allocated_buffers = > __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes); > if (allocated_buffers == 0) { > dprintk(1, "memory allocation failed\n"); > - return -ENOMEM; > + ret = -ENOMEM; > + goto unlock; > } > > /* > @@ -775,19 +786,19 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > */ > } > > - mutex_lock(&q->mmap_lock); > q->num_buffers = allocated_buffers; Why is it safe to stop acquiring mmap_lock here? Looking at other code, I feel like it's assumed that q->num_buffers is actually protected by it. > + call_void_qop(q, queue_setup_unlock, q); > > if (ret < 0) { > /* > * Note: __vb2_queue_free() will subtract 'allocated_buffers' > * from q->num_buffers. > */ > + mutex_lock(&q->mmap_lock); > __vb2_queue_free(q, allocated_buffers); > mutex_unlock(&q->mmap_lock); > return ret; > } > - mutex_unlock(&q->mmap_lock); > > /* > * Return the number of successfully allocated buffers > @@ -795,8 +806,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > */ > *count = allocated_buffers; > q->waiting_for_buffers = !q->is_output; > - > return 0; > + > +unlock: > + call_void_qop(q, queue_setup_unlock, q); > + return ret; > } > EXPORT_SYMBOL_GPL(vb2_core_reqbufs); > > @@ -813,10 +827,12 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > return -ENOBUFS; > } > > + call_void_qop(q, queue_setup_lock, q); > if (!q->num_buffers) { > if (q->waiting_in_dqbuf && *count) { > dprintk(1, "another dup()ped fd is waiting for a buffer\n"); > - return -EBUSY; > + ret = -EBUSY; > + goto unlock; > } > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > q->memory = memory; > @@ -837,14 +853,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > ret = call_qop(q, queue_setup, q, &num_buffers, > &num_planes, plane_sizes, q->alloc_devs); > if (ret) > - return ret; > + goto unlock; > > /* Finally, allocate buffers and video memory */ > allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers, > num_planes, plane_sizes); > if (allocated_buffers == 0) { > dprintk(1, "memory allocation failed\n"); > - return -ENOMEM; > + ret = -ENOMEM; > + goto unlock; > } > > /* > @@ -869,19 +886,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > */ > } > > - mutex_lock(&q->mmap_lock); > q->num_buffers += allocated_buffers; Ditto. > + call_void_qop(q, queue_setup_unlock, q); > > if (ret < 0) { > /* > * Note: __vb2_queue_free() will subtract 'allocated_buffers' > * from q->num_buffers. > */ > + mutex_lock(&q->mmap_lock); > __vb2_queue_free(q, allocated_buffers); > mutex_unlock(&q->mmap_lock); > return -ENOMEM; > } > - mutex_unlock(&q->mmap_lock); > > /* > * Return the number of successfully allocated buffers > @@ -890,6 +907,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > *count = allocated_buffers; > > return 0; > + > +unlock: > + call_void_qop(q, queue_setup_unlock, q); > + return ret; > } > EXPORT_SYMBOL_GPL(vb2_core_create_bufs); > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 613f22910174..92861b6fe7f8 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -333,6 +333,20 @@ struct vb2_buffer { > * \*num_buffers are being allocated additionally to > * q->num_buffers. If either \*num_planes or the requested > * sizes are invalid callback must return %-EINVAL. > + * @queue_setup_lock: called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS() > + * to serialize @queue_setup with ioctls like > + * VIDIOC_S_FMT() that change the buffer size. Only > + * required if queue->lock differs from the mutex that is > + * used to serialize the ioctls that change the buffer > + * size. This callback should lock the ioctl serialization > + * mutex. > + * @queue_setup_unlock: called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS() > + * to serialize @queue_setup with ioctls like > + * VIDIOC_S_FMT() that change the buffer size. Only > + * required if queue->lock differs from the mutex that is > + * used to serialize the ioctls that change the buffer > + * size. This callback should unlock the ioctl > + * serialization mutex. > * @wait_prepare: release any locks taken while calling vb2 functions; > * it is called before an ioctl needs to wait for a new > * buffer to arrive; required to avoid a deadlock in > @@ -403,10 +417,13 @@ struct vb2_ops { > int (*queue_setup)(struct vb2_queue *q, > unsigned int *num_buffers, unsigned int *num_planes, > unsigned int sizes[], struct device *alloc_devs[]); > + void (*queue_setup_lock)(struct vb2_queue *q); > + void (*queue_setup_unlock)(struct vb2_queue *q); > > void (*wait_prepare)(struct vb2_queue *q); > void (*wait_finish)(struct vb2_queue *q); > > + Stray blank line? Best regards, Tomasz
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index f7e7e633bcd7..269485920beb 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -465,7 +465,8 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) */ if (q->num_buffers) { bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming || - q->cnt_wait_prepare != q->cnt_wait_finish; + q->cnt_wait_prepare != q->cnt_wait_finish || + q->cnt_queue_setup_lock != q->cnt_queue_setup_unlock; if (unbalanced || debug) { pr_info("counters for queue %p:%s\n", q, @@ -473,10 +474,14 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) pr_info(" setup: %u start_streaming: %u stop_streaming: %u\n", q->cnt_queue_setup, q->cnt_start_streaming, q->cnt_stop_streaming); + pr_info(" queue_setup_lock: %u queue_setup_unlock: %u\n", + q->cnt_queue_setup_lock, q->cnt_queue_setup_unlock); pr_info(" wait_prepare: %u wait_finish: %u\n", q->cnt_wait_prepare, q->cnt_wait_finish); } q->cnt_queue_setup = 0; + q->cnt_queue_setup_lock = 0; + q->cnt_queue_setup_unlock = 0; q->cnt_wait_prepare = 0; q->cnt_wait_finish = 0; q->cnt_start_streaming = 0; @@ -717,6 +722,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); q->memory = memory; + call_void_qop(q, queue_setup_lock, q); /* * Ask the driver how many buffers and planes per buffer it requires. @@ -725,22 +731,27 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, ret = call_qop(q, queue_setup, q, &num_buffers, &num_planes, plane_sizes, q->alloc_devs); if (ret) - return ret; + goto unlock; /* Check that driver has set sane values */ - if (WARN_ON(!num_planes)) - return -EINVAL; + if (WARN_ON(!num_planes)) { + ret = -EINVAL; + goto unlock; + } for (i = 0; i < num_planes; i++) - if (WARN_ON(!plane_sizes[i])) - return -EINVAL; + if (WARN_ON(!plane_sizes[i])) { + ret = -EINVAL; + goto unlock; + } /* Finally, allocate buffers and video memory */ allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes); if (allocated_buffers == 0) { dprintk(1, "memory allocation failed\n"); - return -ENOMEM; + ret = -ENOMEM; + goto unlock; } /* @@ -775,19 +786,19 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, */ } - mutex_lock(&q->mmap_lock); q->num_buffers = allocated_buffers; + call_void_qop(q, queue_setup_unlock, q); if (ret < 0) { /* * Note: __vb2_queue_free() will subtract 'allocated_buffers' * from q->num_buffers. */ + mutex_lock(&q->mmap_lock); __vb2_queue_free(q, allocated_buffers); mutex_unlock(&q->mmap_lock); return ret; } - mutex_unlock(&q->mmap_lock); /* * Return the number of successfully allocated buffers @@ -795,8 +806,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, */ *count = allocated_buffers; q->waiting_for_buffers = !q->is_output; - return 0; + +unlock: + call_void_qop(q, queue_setup_unlock, q); + return ret; } EXPORT_SYMBOL_GPL(vb2_core_reqbufs); @@ -813,10 +827,12 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, return -ENOBUFS; } + call_void_qop(q, queue_setup_lock, q); if (!q->num_buffers) { if (q->waiting_in_dqbuf && *count) { dprintk(1, "another dup()ped fd is waiting for a buffer\n"); - return -EBUSY; + ret = -EBUSY; + goto unlock; } memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); q->memory = memory; @@ -837,14 +853,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, ret = call_qop(q, queue_setup, q, &num_buffers, &num_planes, plane_sizes, q->alloc_devs); if (ret) - return ret; + goto unlock; /* Finally, allocate buffers and video memory */ allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes); if (allocated_buffers == 0) { dprintk(1, "memory allocation failed\n"); - return -ENOMEM; + ret = -ENOMEM; + goto unlock; } /* @@ -869,19 +886,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, */ } - mutex_lock(&q->mmap_lock); q->num_buffers += allocated_buffers; + call_void_qop(q, queue_setup_unlock, q); if (ret < 0) { /* * Note: __vb2_queue_free() will subtract 'allocated_buffers' * from q->num_buffers. */ + mutex_lock(&q->mmap_lock); __vb2_queue_free(q, allocated_buffers); mutex_unlock(&q->mmap_lock); return -ENOMEM; } - mutex_unlock(&q->mmap_lock); /* * Return the number of successfully allocated buffers @@ -890,6 +907,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, *count = allocated_buffers; return 0; + +unlock: + call_void_qop(q, queue_setup_unlock, q); + return ret; } EXPORT_SYMBOL_GPL(vb2_core_create_bufs); diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 613f22910174..92861b6fe7f8 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -333,6 +333,20 @@ struct vb2_buffer { * \*num_buffers are being allocated additionally to * q->num_buffers. If either \*num_planes or the requested * sizes are invalid callback must return %-EINVAL. + * @queue_setup_lock: called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS() + * to serialize @queue_setup with ioctls like + * VIDIOC_S_FMT() that change the buffer size. Only + * required if queue->lock differs from the mutex that is + * used to serialize the ioctls that change the buffer + * size. This callback should lock the ioctl serialization + * mutex. + * @queue_setup_unlock: called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS() + * to serialize @queue_setup with ioctls like + * VIDIOC_S_FMT() that change the buffer size. Only + * required if queue->lock differs from the mutex that is + * used to serialize the ioctls that change the buffer + * size. This callback should unlock the ioctl + * serialization mutex. * @wait_prepare: release any locks taken while calling vb2 functions; * it is called before an ioctl needs to wait for a new * buffer to arrive; required to avoid a deadlock in @@ -403,10 +417,13 @@ struct vb2_ops { int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers, unsigned int *num_planes, unsigned int sizes[], struct device *alloc_devs[]); + void (*queue_setup_lock)(struct vb2_queue *q); + void (*queue_setup_unlock)(struct vb2_queue *q); void (*wait_prepare)(struct vb2_queue *q); void (*wait_finish)(struct vb2_queue *q); + int (*buf_init)(struct vb2_buffer *vb); int (*buf_prepare)(struct vb2_buffer *vb); void (*buf_finish)(struct vb2_buffer *vb); @@ -599,6 +616,8 @@ struct vb2_queue { * called. Used to check for unbalanced ops. */ u32 cnt_queue_setup; + u32 cnt_queue_setup_lock; + u32 cnt_queue_setup_unlock; u32 cnt_wait_prepare; u32 cnt_wait_finish; u32 cnt_start_streaming;
If queue->lock is different from the video_device lock, then you need to serialize queue_setup with VIDIOC_S_FMT, and this should be done by the driver. Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> --- .../media/common/videobuf2/videobuf2-core.c | 51 +++++++++++++------ include/media/videobuf2-core.h | 19 +++++++ 2 files changed, 55 insertions(+), 15 deletions(-)