Hello,
On Wednesday, November 02, 2011 2:54 PM Laurent Pinchart wrote:
> On Wednesday 02 November 2011 11:52:02 Andrzej Pietrasiewicz wrote:
> > vmalloc-based allocator user pointer handling
> >
> > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > ---
> > drivers/media/video/videobuf2-vmalloc.c | 86 +++++++++++++++++++++++++++-
> > 1 files changed, 85 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/media/video/videobuf2-vmalloc.c
> > b/drivers/media/video/videobuf2-vmalloc.c index a3a8842..ee0ee37 100644
> > --- a/drivers/media/video/videobuf2-vmalloc.c
> > +++ b/drivers/media/video/videobuf2-vmalloc.c
> > @@ -12,6 +12,7 @@
> >
> > #include <linux/module.h>
> > #include <linux/mm.h>
> > +#include <linux/sched.h>
> > #include <linux/slab.h>
> > #include <linux/vmalloc.h>
> >
> > @@ -20,7 +21,10 @@
> >
> > struct vb2_vmalloc_buf {
> > void *vaddr;
> > + struct page **pages;
> > + int write;
> > unsigned long size;
> > + unsigned int n_pages;
> > atomic_t refcount;
> > struct vb2_vmarea_handler handler;
> > };
> > @@ -66,6 +70,83 @@ static void vb2_vmalloc_put(void *buf_priv)
> > }
> > }
> >
> > +static void *vb2_vmalloc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> > + unsigned long size, int write)
> > +{
> > + struct vb2_vmalloc_buf *buf;
> > +
> > + unsigned long first, last;
> > + int n_pages_from_user, offset;
>
> Doesn't the kernel coding style prefer one variable declaration per line ?
>
> > +
> > + buf = kzalloc(sizeof *buf, GFP_KERNEL);
> > + if (!buf)
> > + return NULL;
> > +
> > + buf->vaddr = NULL;
> > + buf->write = write;
> > + offset = vaddr & ~PAGE_MASK;
> > + buf->size = size;
> > +
> > + first = (vaddr & PAGE_MASK) >> PAGE_SHIFT;
> > + last = ((vaddr + size - 1) & PAGE_MASK) >> PAGE_SHIFT;
> > + buf->n_pages = last - first + 1;
> > + buf->pages = kzalloc(buf->n_pages * sizeof(struct page *), GFP_KERNEL);
> > + if (!buf->pages)
> > + goto userptr_fail_pages_array_alloc;
> > +
> > + down_read(¤t->mm->mmap_sem);
> > + n_pages_from_user = get_user_pages(current, current->mm,
> > + vaddr & PAGE_MASK,
> > + buf->n_pages,
> > + write,
> > + 1, /* force */
> > + buf->pages,
> > + NULL);
> > + up_read(¤t->mm->mmap_sem);
>
> This can cause an AB-BA deadlock, and will be reported by deadlock detection
> if enabled.
>
> The issue is that the mmap() handler is called by the MM core with current-
> >mm->mmap_sem held, and then takes the driver's lock before calling
> videobuf2's mmap handler. The VIDIOC_QBUF handler, on the other hand, will
> first take the driver's lock and will then try to take current->mm->mmap_sem
> here.
>
> This can result in a deadlock if both MMAP and USERPTR buffers are used by the
> same driver at the same time.
>
> If we assume that MMAP and USERPTR buffers can't be used on the same queue at
> the same time (VIDIOC_CREATEBUFS doesn't allow that if I'm not mistaken, so we
> should be safe, at least for now), this can be fixed by having a per-queue
> lock in the driver instead of a global device lock. However, that means that
> drivers that want to support USERPTR will not be allowed to delegate lock
> handling to the V4L2 core and video_ioctl2().
Thanks for pointing this issue! This problem is already present in the other
videobuf2 memory allocators as well as the old videobuf and other v4l2 drivers
which implement queue handling by themselves.
The only solution that will not complicate the videobuf2 and allocators code
is to move taking current->mm->mmap_sem lock into videobuf2 core. Before acquiring
this lock, vb2 calls wait_prepare to release device lock and then once mmap_sem is
locked, calls wait_finish to get it again. This way the deadlock is avoided and
allocators are free to call get_user_pages() without further messing with locks.
The only drawback is the fact that a bit more code will be executed under mmap_sem
lock.
What do you think about such solution?
> > + if (n_pages_from_user != buf->n_pages)
> > + goto userptr_fail_get_user_pages;
> > +
> > + buf->vaddr = vm_map_ram(buf->pages, buf->n_pages, -1, PAGE_KERNEL);
>
> Will this create a second kernel mapping ?
Yes, it is very similar to vmalloc function which grabs a set of pages and
creates contiguous virtual kernel mapping for them.
> What if the user tries to pass
> framebuffer memory that has been mapped uncacheable to userspace ?
get_user_pages() fails if it is called for framebuffer memory (VM_PFNMAP type
mappings).
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
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
