linux-kernel - Re: [PATCH 05/26] omap3isp: Make isp_video_buffer_prepare_user() use get_user_pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <11048370.rLWI050cLv@avalon>
Date:	Wed, 02 Oct 2013 21:41:10 +0200
From:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:	Jan Kara <jack@...e.cz>
Cc:	LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org,
	linux-media@...r.kernel.org
Subject: Re: [PATCH 05/26] omap3isp: Make isp_video_buffer_prepare_user() use get_user_pages_fast()

Hi Jan,

Thank you for the patch.

On Wednesday 02 October 2013 16:27:46 Jan Kara wrote:
> CC: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> CC: linux-media@...r.kernel.org
> Signed-off-by: Jan Kara <jack@...e.cz>

Acked-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>

Could you briefly explain where you're headed with this ? The V4L2 subsystem 
has suffered for quite a long time from potentiel AB-BA deadlocks, due the 
drivers taking the mmap_sem semaphore while holding the V4L2 buffers queue 
lock in the code path below, and taking them in reverse order in the mmap() 
path (as the mm core takes the mmap_sem semaphore before calling the mmap() 
operation). We've solved that by releasing the V4L2 buffers queue lock before 
taking the mmap_sem lock below and taking it back after releasing the mmap_sem 
lock. Having an explicit indication that the mmap_sem lock was taken helped 
keeping the problem in mind. I'm not against hiding it in 
get_user_pages_fast(), but I'd like to know what the next step is. Maybe it 
would also be worth it adding a /* get_user_pages_fast() takes the mmap_sem */ 
comment before the call ?

> ---
>  drivers/media/platform/omap3isp/ispqueue.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispqueue.c
> b/drivers/media/platform/omap3isp/ispqueue.c index
> e15f01342058..bed380395e6c 100644
> --- a/drivers/media/platform/omap3isp/ispqueue.c
> +++ b/drivers/media/platform/omap3isp/ispqueue.c
> @@ -331,13 +331,9 @@ static int isp_video_buffer_prepare_user(struct
> isp_video_buffer *buf) if (buf->pages == NULL)
>  		return -ENOMEM;
> 
> -	down_read(&current->mm->mmap_sem);
> -	ret = get_user_pages(current, current->mm, data & PAGE_MASK,
> -			     buf->npages,
> -			     buf->vbuf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
> -			     buf->pages, NULL);
> -	up_read(&current->mm->mmap_sem);
> -
> +	ret = get_user_pages_fast(data & PAGE_MASK, buf->npages,
> +				  buf->vbuf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE,
> +				  buf->pages);
>  	if (ret != buf->npages) {
>  		buf->npages = ret < 0 ? 0 : ret;
>  		isp_video_buffer_cleanup(buf);
-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/