| Message ID | 1349188056-4886-20-git-send-email-t.stanislaws@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Tue October 2 2012 16:27:30 Tomasz Stanislawski wrote: > This patch adds extension to videobuf2-core. It allow to export a mmap buffer > as a file descriptor. > > Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/video/videobuf2-core.c | 82 ++++++++++++++++++++++++++++++++++ > include/media/videobuf2-core.h | 4 ++ > 2 files changed, 86 insertions(+) > > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > index 05da3b4..a97815b 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c <snip> > @@ -2455,6 +2528,15 @@ int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i) > } > EXPORT_SYMBOL_GPL(vb2_ioctl_streamoff); > > +int vb2_ioctl_expbuf(struct file *file, void *priv, struct v4l2_exportbuffer *p) > +{ > + struct video_device *vdev = video_devdata(file); > + > + /* No need to call vb2_queue_is_busy(), anyone can export buffers. */ After thinking about this some more I'm not so sure we should allow this. Exporting a buffer also means that the memory can't be freed as long as the exported filehandle remains open. That means that it is possible to make a malicious application that exports the buffers and never frees them, which can cause havoc. I think that only the filehandle that called REQBUFS/CREATE_BUFS should be allowed to export buffers. What do you think? Regards, Hans > + return vb2_expbuf(vdev->queue, p); > +} > +EXPORT_SYMBOL_GPL(vb2_ioctl_expbuf); > + > /* v4l2_file_operations helpers */ > > int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma)
On 10/06/2012 02:22 PM, Hans Verkuil wrote: > On Tue October 2 2012 16:27:30 Tomasz Stanislawski wrote: >> This patch adds extension to videobuf2-core. It allow to export a mmap buffer >> as a file descriptor. >> >> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> drivers/media/video/videobuf2-core.c | 82 ++++++++++++++++++++++++++++++++++ >> include/media/videobuf2-core.h | 4 ++ >> 2 files changed, 86 insertions(+) >> >> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c >> index 05da3b4..a97815b 100644 >> --- a/drivers/media/video/videobuf2-core.c >> +++ b/drivers/media/video/videobuf2-core.c > > <snip> > >> @@ -2455,6 +2528,15 @@ int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i) >> } >> EXPORT_SYMBOL_GPL(vb2_ioctl_streamoff); >> >> +int vb2_ioctl_expbuf(struct file *file, void *priv, struct v4l2_exportbuffer *p) >> +{ >> + struct video_device *vdev = video_devdata(file); >> + >> + /* No need to call vb2_queue_is_busy(), anyone can export buffers. */ Hi Hans, > > After thinking about this some more I'm not so sure we should allow this. > Exporting a buffer also means that the memory can't be freed as long as the > exported filehandle remains open. > You are completely right. But the problem is much more complex ... see below. > That means that it is possible to make a malicious application that exports > the buffers and never frees them, which can cause havoc. What kind of havoc do you mean? Resource leakage? > I think that only > the filehandle that called REQBUFS/CREATE_BUFS should be allowed to export > buffers. Notice that you should allow to call mmap() only for the file handles that called REQBUFS/CREATE_BUFS. The mmap() creates a vma that holds a reference to a buffer. As long as the mapping exists, the buffers cannot be freed and REQBUFS(count=0) returns -EBUSY. According to V4L2 spec all the nodes share the same context, therefore one process can call REQBUFS, and the other can call QUERYBUF and mmap(). Therefore if EXPBUF has to check vb2_queue_is_busy() then vb2_fop_mmap() should do the same check too. IMO, it is very difficult to develop a useful multi-client API that would protect V4L2 from creating non-freeable buffers by a rogue applications. I think that the requirements below: - the buffers should be sharable between processes by mmap(), or DMABUF exporting - the REQBUFS(count=0) should release the buffers are contradictory and cannot be *reliably* satisfied at the same time. Notice that REQBUFS(count=0) that unexpectedly return -EBUSY is not a reliable solution. The application cannot do anything fix the problem after receiving -EBUSY. Anyway, I will apply the check for vb2_queue_is_busy() in v2b_ioctl_expbuf(). Regards, Tomasz Stanislawski > > What do you think? > > Regards, > > Hans > >> + return vb2_expbuf(vdev->queue, p); >> +} >> +EXPORT_SYMBOL_GPL(vb2_ioctl_expbuf); >> + >> /* v4l2_file_operations helpers */ >> >> int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma)
diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c index 05da3b4..a97815b 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -1753,6 +1753,79 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, } /** + * vb2_expbuf() - Export a buffer as a file descriptor + * @q: videobuf2 queue + * @eb: export buffer structure passed from userspace to vidioc_expbuf + * handler in driver + * + * The return values from this function are intended to be directly returned + * from vidioc_expbuf handler in driver. + */ +int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb) +{ + struct vb2_buffer *vb = NULL; + struct vb2_plane *vb_plane; + int ret; + struct dma_buf *dbuf; + + if (q->memory != V4L2_MEMORY_MMAP) { + dprintk(1, "Queue is not currently set up for mmap\n"); + return -EINVAL; + } + + if (!q->mem_ops->get_dmabuf) { + dprintk(1, "Queue does not support DMA buffer exporting\n"); + return -EINVAL; + } + + if (eb->flags & ~O_CLOEXEC) { + dprintk(1, "Queue does support only O_CLOEXEC flag\n"); + return -EINVAL; + } + + if (eb->type != q->type) { + dprintk(1, "qbuf: invalid buffer type\n"); + return -EINVAL; + } + + if (eb->index >= q->num_buffers) { + dprintk(1, "buffer index out of range\n"); + return -EINVAL; + } + + vb = q->bufs[eb->index]; + + if (eb->plane >= vb->num_planes) { + dprintk(1, "buffer plane out of range\n"); + return -EINVAL; + } + + vb_plane = &vb->planes[eb->plane]; + + dbuf = call_memop(q, get_dmabuf, vb_plane->mem_priv); + if (IS_ERR_OR_NULL(dbuf)) { + dprintk(1, "Failed to export buffer %d, plane %d\n", + eb->index, eb->plane); + return -EINVAL; + } + + ret = dma_buf_fd(dbuf, eb->flags); + if (ret < 0) { + dprintk(3, "buffer %d, plane %d failed to export (%d)\n", + eb->index, eb->plane, ret); + dma_buf_put(dbuf); + return ret; + } + + dprintk(3, "buffer %d, plane %d exported as %d descriptor\n", + eb->index, eb->plane, ret); + eb->fd = ret; + + return 0; +} +EXPORT_SYMBOL_GPL(vb2_expbuf); + +/** * vb2_mmap() - map video buffers into application address space * @q: videobuf2 queue * @vma: vma passed to the mmap file operation handler in the driver @@ -2455,6 +2528,15 @@ int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i) } EXPORT_SYMBOL_GPL(vb2_ioctl_streamoff); +int vb2_ioctl_expbuf(struct file *file, void *priv, struct v4l2_exportbuffer *p) +{ + struct video_device *vdev = video_devdata(file); + + /* No need to call vb2_queue_is_busy(), anyone can export buffers. */ + return vb2_expbuf(vdev->queue, p); +} +EXPORT_SYMBOL_GPL(vb2_ioctl_expbuf); + /* v4l2_file_operations helpers */ int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma) diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index c306fec..3f57ccb 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -81,6 +81,7 @@ struct vb2_fileio_data; struct vb2_mem_ops { void *(*alloc)(void *alloc_ctx, unsigned long size); void (*put)(void *buf_priv); + struct dma_buf *(*get_dmabuf)(void *buf_priv); void *(*get_userptr)(void *alloc_ctx, unsigned long vaddr, unsigned long size, int write); @@ -363,6 +364,7 @@ int vb2_queue_init(struct vb2_queue *q); void vb2_queue_release(struct vb2_queue *q); int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b); +int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb); int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking); int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type); @@ -472,6 +474,8 @@ int vb2_ioctl_qbuf(struct file *file, void *priv, struct v4l2_buffer *p); int vb2_ioctl_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p); int vb2_ioctl_streamon(struct file *file, void *priv, enum v4l2_buf_type i); int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i); +int vb2_ioctl_expbuf(struct file *file, void *priv, + struct v4l2_exportbuffer *p); /* struct v4l2_file_operations helpers */