| Message ID | 1376050286-8201-2-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri 9 August 2013 14:11:25 Laurent Pinchart wrote: > Commit b037c0fde22b1d3cd0b3c3717d28e54619fc1592 ("media: vb2: fix > potential deadlock in mmap vs. get_userptr handling") fixes an AB-BA > deadlock related to the mmap_sem and driver locks. The same deadlock can > occur in vb2_prepare_buffer(), fix it the same way. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> Regards, Hans > --- > drivers/media/v4l2-core/videobuf2-core.c | 52 ++++++++++++++++++++++++++------ > 1 file changed, 43 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 7f32860..7c2a8ce 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -1248,50 +1248,84 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) > */ > int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b) > { > + struct rw_semaphore *mmap_sem = NULL; > struct vb2_buffer *vb; > int ret; > > + /* > + * In case of user pointer buffers vb2 allocator needs to get direct > + * access to userspace pages. This requires getting read access on > + * mmap semaphore in the current process structure. The same > + * semaphore is taken before calling mmap operation, while both mmap > + * and prepare_buf are called by the driver or v4l2 core with driver's > + * lock held. To avoid a AB-BA deadlock (mmap_sem then driver's lock in > + * mmap and driver's lock then mmap_sem in prepare_buf) the videobuf2 > + * core release driver's lock, takes mmap_sem and then takes again > + * driver's lock. > + * > + * To avoid race with other vb2 calls, which might be called after > + * releasing driver's lock, this operation is performed at the > + * beggining of prepare_buf processing. This way the queue status is > + * consistent after getting driver's lock back. > + */ > + if (q->memory == V4L2_MEMORY_USERPTR) { > + mmap_sem = ¤t->mm->mmap_sem; > + call_qop(q, wait_prepare, q); > + down_read(mmap_sem); > + call_qop(q, wait_finish, q); > + } > + > if (q->fileio) { > dprintk(1, "%s(): file io in progress\n", __func__); > - return -EBUSY; > + ret = -EBUSY; > + goto unlock; > } > > if (b->type != q->type) { > dprintk(1, "%s(): invalid buffer type\n", __func__); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > if (b->index >= q->num_buffers) { > dprintk(1, "%s(): buffer index out of range\n", __func__); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > vb = q->bufs[b->index]; > if (NULL == vb) { > /* Should never happen */ > dprintk(1, "%s(): buffer is NULL\n", __func__); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > if (b->memory != q->memory) { > dprintk(1, "%s(): invalid memory type\n", __func__); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > if (vb->state != VB2_BUF_STATE_DEQUEUED) { > dprintk(1, "%s(): invalid buffer state %d\n", __func__, vb->state); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > ret = __verify_planes_array(vb, b); > if (ret < 0) > - return ret; > + goto unlock; > + > ret = __buf_prepare(vb, b); > if (ret < 0) > - return ret; > + goto unlock; > > __fill_v4l2_buffer(vb, b); > > - return 0; > +unlock: > + if (mmap_sem) > + up_read(mmap_sem); > + return ret; > } > EXPORT_SYMBOL_GPL(vb2_prepare_buf); > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 8/9/2013 2:11 PM, Laurent Pinchart wrote: > Commit b037c0fde22b1d3cd0b3c3717d28e54619fc1592 ("media: vb2: fix > potential deadlock in mmap vs. get_userptr handling") fixes an AB-BA > deadlock related to the mmap_sem and driver locks. The same deadlock can > occur in vb2_prepare_buffer(), fix it the same way. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/media/v4l2-core/videobuf2-core.c | 52 ++++++++++++++++++++++++++------ > 1 file changed, 43 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 7f32860..7c2a8ce 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -1248,50 +1248,84 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) > */ > int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b) > { > + struct rw_semaphore *mmap_sem = NULL; > struct vb2_buffer *vb; > int ret; > > + /* > + * In case of user pointer buffers vb2 allocator needs to get direct > + * access to userspace pages. This requires getting read access on > + * mmap semaphore in the current process structure. The same > + * semaphore is taken before calling mmap operation, while both mmap > + * and prepare_buf are called by the driver or v4l2 core with driver's > + * lock held. To avoid a AB-BA deadlock (mmap_sem then driver's lock in > + * mmap and driver's lock then mmap_sem in prepare_buf) the videobuf2 > + * core release driver's lock, takes mmap_sem and then takes again > + * driver's lock. > + * > + * To avoid race with other vb2 calls, which might be called after > + * releasing driver's lock, this operation is performed at the > + * beggining of prepare_buf processing. This way the queue status is > + * consistent after getting driver's lock back. > + */ > + if (q->memory == V4L2_MEMORY_USERPTR) { > + mmap_sem = ¤t->mm->mmap_sem; > + call_qop(q, wait_prepare, q); > + down_read(mmap_sem); > + call_qop(q, wait_finish, q); > + } > + > if (q->fileio) { > dprintk(1, "%s(): file io in progress\n", __func__); > - return -EBUSY; > + ret = -EBUSY; > + goto unlock; > } > > if (b->type != q->type) { > dprintk(1, "%s(): invalid buffer type\n", __func__); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > if (b->index >= q->num_buffers) { > dprintk(1, "%s(): buffer index out of range\n", __func__); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > vb = q->bufs[b->index]; > if (NULL == vb) { > /* Should never happen */ > dprintk(1, "%s(): buffer is NULL\n", __func__); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > if (b->memory != q->memory) { > dprintk(1, "%s(): invalid memory type\n", __func__); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > if (vb->state != VB2_BUF_STATE_DEQUEUED) { > dprintk(1, "%s(): invalid buffer state %d\n", __func__, vb->state); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > ret = __verify_planes_array(vb, b); > if (ret < 0) > - return ret; > + goto unlock; > + > ret = __buf_prepare(vb, b); > if (ret < 0) > - return ret; > + goto unlock; > > __fill_v4l2_buffer(vb, b); > > - return 0; > +unlock: > + if (mmap_sem) > + up_read(mmap_sem); > + return ret; > } > EXPORT_SYMBOL_GPL(vb2_prepare_buf); > Best regards
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 7f32860..7c2a8ce 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -1248,50 +1248,84 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) */ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b) { + struct rw_semaphore *mmap_sem = NULL; struct vb2_buffer *vb; int ret; + /* + * In case of user pointer buffers vb2 allocator needs to get direct + * access to userspace pages. This requires getting read access on + * mmap semaphore in the current process structure. The same + * semaphore is taken before calling mmap operation, while both mmap + * and prepare_buf are called by the driver or v4l2 core with driver's + * lock held. To avoid a AB-BA deadlock (mmap_sem then driver's lock in + * mmap and driver's lock then mmap_sem in prepare_buf) the videobuf2 + * core release driver's lock, takes mmap_sem and then takes again + * driver's lock. + * + * To avoid race with other vb2 calls, which might be called after + * releasing driver's lock, this operation is performed at the + * beggining of prepare_buf processing. This way the queue status is + * consistent after getting driver's lock back. + */ + if (q->memory == V4L2_MEMORY_USERPTR) { + mmap_sem = ¤t->mm->mmap_sem; + call_qop(q, wait_prepare, q); + down_read(mmap_sem); + call_qop(q, wait_finish, q); + } + if (q->fileio) { dprintk(1, "%s(): file io in progress\n", __func__); - return -EBUSY; + ret = -EBUSY; + goto unlock; } if (b->type != q->type) { dprintk(1, "%s(): invalid buffer type\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto unlock; } if (b->index >= q->num_buffers) { dprintk(1, "%s(): buffer index out of range\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto unlock; } vb = q->bufs[b->index]; if (NULL == vb) { /* Should never happen */ dprintk(1, "%s(): buffer is NULL\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto unlock; } if (b->memory != q->memory) { dprintk(1, "%s(): invalid memory type\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto unlock; } if (vb->state != VB2_BUF_STATE_DEQUEUED) { dprintk(1, "%s(): invalid buffer state %d\n", __func__, vb->state); - return -EINVAL; + ret = -EINVAL; + goto unlock; } ret = __verify_planes_array(vb, b); if (ret < 0) - return ret; + goto unlock; + ret = __buf_prepare(vb, b); if (ret < 0) - return ret; + goto unlock; __fill_v4l2_buffer(vb, b); - return 0; +unlock: + if (mmap_sem) + up_read(mmap_sem); + return ret; } EXPORT_SYMBOL_GPL(vb2_prepare_buf);
Commit b037c0fde22b1d3cd0b3c3717d28e54619fc1592 ("media: vb2: fix potential deadlock in mmap vs. get_userptr handling") fixes an AB-BA deadlock related to the mmap_sem and driver locks. The same deadlock can occur in vb2_prepare_buffer(), fix it the same way. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/v4l2-core/videobuf2-core.c | 52 ++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 9 deletions(-)