Hi Hans,
Thank you for the patch.
On Monday 04 August 2014 14:12:32 Hans Verkuil wrote:
> Changes since v1: use a spinlock instead of playing around with mmap_sem
> since current->mm might point to different entities as pointed out by
> Marek.
>
> The following lockdep warning has been there ever since commit
> a517cca6b24fc54ac209e44118ec8962051662e3 one year ago:
[snip]
> The reason is that vb2_fop_mmap and vb2_fop_get_unmapped_area take the core
> lock while they are called with the mmap_sem semaphore held. But elsewhere
> in the code the core lock is taken first but calls to copy_to/from_user()
> can take the mmap_sem semaphore as well, potentially causing a classical
> A-B/B-A deadlock.
>
> However, the mmap/get_unmapped_area calls really shouldn't take the core
> lock at all. So what would happen if they don't take the core lock anymore?
>
> There are two situations that need to be taken into account: calling mmap
> while new buffers are being added and calling mmap while buffers are being
> deleted.
>
> The first case works fine without a lock: in all cases mmap relies on
> correctly filled-in q->num_buffers/q->num_planes values and those are only
> updated after any new buffers have been initialized completely. So it will
> never get partially initialized buffer information.
>
> The second case does pose a problem: buffers may be in the process of being
> deleted, without the internal structure being updated.
>
> The core issue is that mem_priv may be non-NULL when it is already freed.
> The solution I chose is to add a spinlock that is taken whenever mem_priv
> is changed and when vb2_mmap() and vb2_fop_get_unmapped_area() need it.
> That way those functions can be certain that mem_priv can be relied upon.
I think you introduce a race condition. __reqbufs() will first check whether
any buffer is in use by calling __buffers_in_use() and will the proceed to
free the buffers with __vb2_queue_free(). If a buffer is mmap()ed between the
two calls I believe vb2 will happily free the memory with this patch applied.
Also, are all the memops mmap() implementations safe to be called with a
spinlock held, or could some of them need to sleep ? How costly can they be in
terms of CPU time ?
> As an additional bonus the hack in __buf_prepare, the USERPTR case, can be
> removed as well since mmap() no longer takes the core lock.
>
> All-in-all a much cleaner solution.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> drivers/media/v4l2-core/videobuf2-core.c | 105 ++++++++++++++--------------
> include/media/videobuf2-core.h | 2 +
> 2 files changed, 58 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index c359006..18bf059 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -205,7 +205,9 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> goto free;
>
> /* Associate allocator private data with this plane */
> + spin_lock(&vb->planes_lock);
> vb->planes[plane].mem_priv = mem_priv;
> + spin_unlock(&vb->planes_lock);
> vb->v4l2_planes[plane].length = q->plane_sizes[plane];
> }
>
> @@ -213,8 +215,12 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> free:
> /* Free already allocated memory if one of the allocations failed */
> for (; plane > 0; --plane) {
> - call_void_memop(vb, put, vb->planes[plane - 1].mem_priv);
> + mem_priv = vb->planes[plane - 1].mem_priv;
> +
> + spin_lock(&vb->planes_lock);
> vb->planes[plane - 1].mem_priv = NULL;
> + spin_unlock(&vb->planes_lock);
> + call_void_memop(vb, put, mem_priv);
> }
>
> return -ENOMEM;
> @@ -228,8 +234,21 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
> unsigned int plane;
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> - call_void_memop(vb, put, vb->planes[plane].mem_priv);
> + void *mem_priv = vb->planes[plane].mem_priv;
> +
> + /*
> + * There is a potential race condition between mmap() and
> + * __vb2_buf_mem_free() where mmap() might map a buffer that is
> + * about to be freed. So before we delete it we take the
> + * planes_lock, set mem_priv to NULL and release the spinlock
> + * again. Since vb2_mmap() uses the same spinlock it can never
> + * read a stale mem_priv pointer.
> + */
> + spin_lock(&vb->planes_lock);
> vb->planes[plane].mem_priv = NULL;
> + spin_unlock(&vb->planes_lock);
> +
> + call_void_memop(vb, put, mem_priv);
> dprintk(3, "freed plane %d of buffer %d\n", plane,
> vb->v4l2_buf.index);
> }
> @@ -244,9 +263,14 @@ static void __vb2_buf_userptr_put(struct vb2_buffer
> *vb) unsigned int plane;
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> - if (vb->planes[plane].mem_priv)
> - call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> - vb->planes[plane].mem_priv = NULL;
> + void *mem_priv = vb->planes[plane].mem_priv;
> +
> + if (mem_priv) {
> + spin_lock(&vb->planes_lock);
> + vb->planes[plane].mem_priv = NULL;
> + spin_unlock(&vb->planes_lock);
> + call_void_memop(vb, put_userptr, mem_priv);
> + }
> }
> }
>
> @@ -256,13 +280,18 @@ static void __vb2_buf_userptr_put(struct vb2_buffer
> *vb) */
> static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane
> *p) {
> - if (!p->mem_priv)
> + void *mem_priv = p->mem_priv;
> +
> + if (!mem_priv)
> return;
>
> + spin_lock(&vb->planes_lock);
> + p->mem_priv = NULL;
> + spin_unlock(&vb->planes_lock);
> if (p->dbuf_mapped)
> - call_void_memop(vb, unmap_dmabuf, p->mem_priv);
> + call_void_memop(vb, unmap_dmabuf, mem_priv);
>
> - call_void_memop(vb, detach_dmabuf, p->mem_priv);
> + call_void_memop(vb, detach_dmabuf, mem_priv);
> dma_buf_put(p->dbuf);
> memset(p, 0, sizeof(*p));
> }
> @@ -366,6 +395,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum
> v4l2_memory memory, vb->v4l2_buf.index = q->num_buffers + buffer;
> vb->v4l2_buf.type = q->type;
> vb->v4l2_buf.memory = memory;
> + spin_lock_init(&vb->planes_lock);
>
> /* Allocate video buffer memory for the MMAP type */
> if (memory == V4L2_MEMORY_MMAP) {
> @@ -1347,7 +1377,6 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) {
> struct v4l2_plane planes[VIDEO_MAX_PLANES];
> struct vb2_queue *q = vb->vb2_queue;
> - void *mem_priv;
> unsigned int plane;
> int ret;
> int write = !V4L2_TYPE_IS_OUTPUT(q->type);
> @@ -1358,6 +1387,8 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) __fill_vb2_buffer(vb, b, planes);
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> + void *mem_priv = vb->planes[plane].mem_priv;
> +
> /* Skip the plane if already verified */
> if (vb->v4l2_planes[plane].m.userptr &&
> vb->v4l2_planes[plane].m.userptr == planes[plane].m.userptr
> @@ -1378,15 +1409,17 @@ static int __qbuf_userptr(struct vb2_buffer *vb,
> const struct v4l2_buffer *b) }
>
> /* Release previously acquired memory if present */
> - if (vb->planes[plane].mem_priv) {
> + if (mem_priv) {
> if (!reacquired) {
> reacquired = true;
> call_void_vb_qop(vb, buf_cleanup, vb);
> }
> - call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> + spin_lock(&vb->planes_lock);
> + vb->planes[plane].mem_priv = NULL;
> + spin_unlock(&vb->planes_lock);
> + call_void_memop(vb, put_userptr, mem_priv);
> }
>
> - vb->planes[plane].mem_priv = NULL;
> memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
>
> /* Acquire each plane's memory */
> @@ -1399,7 +1432,9 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
> goto err;
> }
> + spin_lock(&vb->planes_lock);
> vb->planes[plane].mem_priv = mem_priv;
> + spin_unlock(&vb->planes_lock);
> }
>
> /*
> @@ -1510,7 +1545,9 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) }
>
> vb->planes[plane].dbuf = dbuf;
> + spin_lock(&vb->planes_lock);
> vb->planes[plane].mem_priv = mem_priv;
> + spin_unlock(&vb->planes_lock);
> }
>
> /* TODO: This pins the buffer(s) with dma_buf_map_attachment()).. but
> @@ -1582,7 +1619,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer
> *b) {
> struct vb2_queue *q = vb->vb2_queue;
> - struct rw_semaphore *mmap_sem;
> int ret;
>
> ret = __verify_length(vb, b);
> @@ -1619,26 +1655,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) ret = __qbuf_mmap(vb, b);
> break;
> case V4L2_MEMORY_USERPTR:
> - /*
> - * In case of user pointer buffers vb2 allocators need to get
> - * direct access to userspace pages. This requires getting
> - * the mmap semaphore for read access in the current process
> - * structure. The same semaphore is taken before calling mmap
> - * operation, while both qbuf/prepare_buf and mmap are called
> - * by the driver or v4l2 core with the driver's lock held.
> - * To avoid an AB-BA deadlock (mmap_sem then driver's lock in
> - * mmap and driver's lock then mmap_sem in qbuf/prepare_buf),
> - * the videobuf2 core releases the driver's lock, takes
> - * mmap_sem and then takes the driver's lock again.
> - */
> - mmap_sem = ¤t->mm->mmap_sem;
> - call_void_qop(q, wait_prepare, q);
> - down_read(mmap_sem);
> - call_void_qop(q, wait_finish, q);
> -
> ret = __qbuf_userptr(vb, b);
> -
> - up_read(mmap_sem);
> break;
> case V4L2_MEMORY_DMABUF:
> ret = __qbuf_dmabuf(vb, b);
> @@ -2485,7 +2502,9 @@ int vb2_mmap(struct vb2_queue *q, struct
> vm_area_struct *vma) return -EINVAL;
> }
>
> + spin_lock(&vb->planes_lock);
> ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
> + spin_unlock(&vb->planes_lock);
> if (ret)
> return ret;
>
> @@ -2504,6 +2523,7 @@ unsigned long vb2_get_unmapped_area(struct vb2_queue
> *q, unsigned long off = pgoff << PAGE_SHIFT;
> struct vb2_buffer *vb;
> unsigned int buffer, plane;
> + void *vaddr;
> int ret;
>
> if (q->memory != V4L2_MEMORY_MMAP) {
> @@ -2520,7 +2540,8 @@ unsigned long vb2_get_unmapped_area(struct vb2_queue
> *q,
>
> vb = q->bufs[buffer];
>
> - return (unsigned long)vb2_plane_vaddr(vb, plane);
> + vaddr = vb2_plane_vaddr(vb, plane);
> + return vaddr ? (unsigned long)vaddr : -EINVAL;
> }
> EXPORT_SYMBOL_GPL(vb2_get_unmapped_area);
> #endif
> @@ -3346,15 +3367,8 @@ EXPORT_SYMBOL_GPL(vb2_ioctl_expbuf);
> int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma)
> {
> struct video_device *vdev = video_devdata(file);
> - struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
> - int err;
>
> - if (lock && mutex_lock_interruptible(lock))
> - return -ERESTARTSYS;
> - err = vb2_mmap(vdev->queue, vma);
> - if (lock)
> - mutex_unlock(lock);
> - return err;
> + return vb2_mmap(vdev->queue, vma);
> }
> EXPORT_SYMBOL_GPL(vb2_fop_mmap);
>
> @@ -3473,15 +3487,8 @@ unsigned long vb2_fop_get_unmapped_area(struct file
> *file, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned
> long flags)
> {
> struct video_device *vdev = video_devdata(file);
> - struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
> - int ret;
>
> - if (lock && mutex_lock_interruptible(lock))
> - return -ERESTARTSYS;
> - ret = vb2_get_unmapped_area(vdev->queue, addr, len, pgoff, flags);
> - if (lock)
> - mutex_unlock(lock);
> - return ret;
> + return vb2_get_unmapped_area(vdev->queue, addr, len, pgoff, flags);
> }
> EXPORT_SYMBOL_GPL(vb2_fop_get_unmapped_area);
> #endif
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index fc910a6..551d334 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -187,6 +187,7 @@ struct vb2_queue;
> * buffers queued from userspace
> * @done_entry: entry on the list that stores all buffers ready to
> * be dequeued to userspace
> + * @planes_lock: private spinlock for the per-plane information
> * @planes: private per-plane information; do not change
> */
> struct vb2_buffer {
> @@ -203,6 +204,7 @@ struct vb2_buffer {
> struct list_head queued_entry;
> struct list_head done_entry;
>
> + spinlock_t planes_lock;
> struct vb2_plane planes[VIDEO_MAX_PLANES];
>
> #ifdef CONFIG_VIDEO_ADV_DEBUG
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
