[PATCH] nommu: remap_pfn_range: fix addr parameter check

[PATCH] nommu: remap_pfn_range: fix addr parameter check

[Bob Liu]

The addr parameter may not page aligned eg. when it's come from
vfb_mmap():vma->vm_start in video driver.

This patch fix the check in remap_pfn_range() else some driver like v4l2 will
fail in this function while calling mmap() on nommu arch like blackfin and st.

Reported-by: Bhupesh SHARMA <bhupesh.sharma@st.com>
Reported-by: Scott Jiang <scott.jiang.linux@gmail.com>
Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/nommu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/nommu.c b/mm/nommu.c
index d4b0c10..5d6068b 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1819,7 +1819,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
 int remap_pfn_range(struct vm_area_struct *vma, unsigned long addr,
 		unsigned long pfn, unsigned long size, pgprot_t prot)
 {
-	if (addr != (pfn << PAGE_SHIFT))
+	if ((addr & PAGE_MASK) != (pfn << PAGE_SHIFT))
 		return -EINVAL;
 
 	vma->vm_flags |= VM_IO | VM_RESERVED | VM_PFNMAP;
-- 
1.7.9.5


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] nommu: remap_pfn_range: fix addr parameter check

[Andrew Morton]

On Thu, 13 Sep 2012 10:40:57 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> The addr parameter may not page aligned eg. when it's come from
> vfb_mmap():vma->vm_start in video driver.
> 
> This patch fix the check in remap_pfn_range() else some driver like v4l2 will
> fail in this function while calling mmap() on nommu arch like blackfin and st.
> 
> Reported-by: Bhupesh SHARMA <bhupesh.sharma@st.com>
> Reported-by: Scott Jiang <scott.jiang.linux@gmail.com>
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/nommu.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/nommu.c b/mm/nommu.c
> index d4b0c10..5d6068b 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1819,7 +1819,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
>  int remap_pfn_range(struct vm_area_struct *vma, unsigned long addr,
>  		unsigned long pfn, unsigned long size, pgprot_t prot)
>  {
> -	if (addr != (pfn << PAGE_SHIFT))
> +	if ((addr & PAGE_MASK) != (pfn << PAGE_SHIFT))
>  		return -EINVAL;
>  
>  	vma->vm_flags |= VM_IO | VM_RESERVED | VM_PFNMAP;

hm, what is the right thing to do here?

Yes, the MMU version of remap_pfn_range() does permit non-page-aligned
`addr' (at least, if the userspace maaping is a non-COW one).  But I
suspect that was an implementation accident - it is a nonsensical thing
to do, isn't it?  The MMU cannot map a bunch of kernel pages onto a
non-page-aligned userspace address.

So I'm thinking that we should declare ((addr & ~PAGE_MASK) != 0) to be
a caller bug, and fix up this regrettably unidentified v4l driver?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] nommu: remap_pfn_range: fix addr parameter check

[Scott Jiang]

> Yes, the MMU version of remap_pfn_range() does permit non-page-aligned
> `addr' (at least, if the userspace maaping is a non-COW one).  But I
> suspect that was an implementation accident - it is a nonsensical thing
> to do, isn't it?  The MMU cannot map a bunch of kernel pages onto a
> non-page-aligned userspace address.
>
> So I'm thinking that we should declare ((addr & ~PAGE_MASK) != 0) to be
> a caller bug, and fix up this regrettably unidentified v4l driver?

I agree. This should be fixed in videobuf.

Hi sharma, what's your kernel version? It seems videobuf2 already
fixed this bug in 3.5.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] nommu: remap_pfn_range: fix addr parameter check

[Bhupesh SHARMA]

> -----Original Message-----
> From: Scott Jiang [mailto:scott.jiang.linux@gmail.com]
> Sent: Friday, September 14, 2012 2:53 PM
> To: Andrew Morton
> Cc: Bob Liu; linux-mm@kvack.org; Bhupesh SHARMA;
> laurent.pinchart@ideasonboard.com; uclinux-dist-
> devel@blackfin.uclinux.org; linux-media@vger.kernel.org;
> dhowells@redhat.com; geert@linux-m68k.org; gerg@uclinux.org;
> stable@kernel.org; gregkh@linuxfoundation.org; Hugh Dickins
> Subject: Re: [PATCH] nommu: remap_pfn_range: fix addr parameter check
> 
> > Yes, the MMU version of remap_pfn_range() does permit non-page-
> aligned
> > `addr' (at least, if the userspace maaping is a non-COW one).  But I
> > suspect that was an implementation accident - it is a nonsensical
> > thing to do, isn't it?  The MMU cannot map a bunch of kernel pages
> > onto a non-page-aligned userspace address.
> >
> > So I'm thinking that we should declare ((addr & ~PAGE_MASK) != 0) to
> > be a caller bug, and fix up this regrettably unidentified v4l driver?
> 
> I agree. This should be fixed in videobuf.
> 
> Hi sharma, what's your kernel version? It seems videobuf2 already fixed this
> bug in 3.5.

Hi Scott,

I was using 3.3 linux kernel. I will again check if videobuf2 in 3.5 has already fixed this issue.

Regards,
Bhupesh


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] nommu: remap_pfn_range: fix addr parameter check

[Bhupesh SHARMA]

> -----Original Message-----
> From: Bhupesh SHARMA
> Sent: Friday, September 14, 2012 3:45 PM
> To: 'Scott Jiang'; Andrew Morton
> Cc: Bob Liu; linux-mm@kvack.org; laurent.pinchart@ideasonboard.com;
> uclinux-dist-devel@blackfin.uclinux.org; linux-media@vger.kernel.org;
> dhowells@redhat.com; geert@linux-m68k.org; gerg@uclinux.org;
> stable@kernel.org; gregkh@linuxfoundation.org; Hugh Dickins
> Subject: RE: [PATCH] nommu: remap_pfn_range: fix addr parameter check
> 
> > -----Original Message-----
> > From: Scott Jiang [mailto:scott.jiang.linux@gmail.com]
> > Sent: Friday, September 14, 2012 2:53 PM
> > To: Andrew Morton
> > Cc: Bob Liu; linux-mm@kvack.org; Bhupesh SHARMA;
> > laurent.pinchart@ideasonboard.com; uclinux-dist-
> > devel@blackfin.uclinux.org; linux-media@vger.kernel.org;
> > dhowells@redhat.com; geert@linux-m68k.org; gerg@uclinux.org;
> > stable@kernel.org; gregkh@linuxfoundation.org; Hugh Dickins
> > Subject: Re: [PATCH] nommu: remap_pfn_range: fix addr parameter check
> >
> > > Yes, the MMU version of remap_pfn_range() does permit non-page-
> > aligned
> > > `addr' (at least, if the userspace maaping is a non-COW one).  But I
> > > suspect that was an implementation accident - it is a nonsensical
> > > thing to do, isn't it?  The MMU cannot map a bunch of kernel pages
> > > onto a non-page-aligned userspace address.
> > >
> > > So I'm thinking that we should declare ((addr & ~PAGE_MASK) != 0) to
> > > be a caller bug, and fix up this regrettably unidentified v4l driver?
> >
> > I agree. This should be fixed in videobuf.
> >
> > Hi sharma, what's your kernel version? It seems videobuf2 already
> > fixed this bug in 3.5.
> 

[snip..]

> 
> I was using 3.3 linux kernel. I will again check if videobuf2 in 3.5 has already
> fixed this issue.

[snip..]

Ok I just checked the vb2_dma_contig allocator and it has no major changes from my version,
http://lxr.linux.no/linux+v3.5.3/drivers/media/video/videobuf2-dma-contig.c#L37

So, I am not sure if this issue has been fixed in the videobuf2 (or if any patch is in the pipeline
which fixes the issue).

BTW I paste my original mail on the subject below to help everyone understand the complete setup
and the issue I faced.

Regards,
Bhupesh

---------------------
Hi,

I have been trying recently to make a usb-based-webcam device to work with Linux. The entire scheme is a bit complex:

UVC gadget <--User Pointer--> User-Space Daemon <-- MMAP --> V4L2 capture device.

The UVC gadget is internally a v4l2 based device supporting VB2_VMALLOC operations, whereas the V4L2 capture device supports VB2_DMA_CONTIG operations.

The application (user-space daemon), is responsible for getting memory allocated from the V4L2 capture device via REQBUF calls. The V4L2 capture side exposes a
 MMAP IO method, whereas the UVC gadget can get a USERPTR to the buffer filled with video data from the V4L2 capture device and then send the same on a USB bus.

This scheme works absolutely fine on an architecture having a MMU, but when I try the same on a NOMMU arch, I see MMAP calls from the user-space daemon failing.

I have implemented a .get_unmapped_area callback in my V4L2 capture driver using the blackfin video capture driver as a reference (see [1]).

I make a MMAP call from the user-space application in a sequence like this (pretty similar to the standard capture.c example, see[2]):

static void init_mmap (void)
{
	struct v4l2_requestbuffers req;

	CLEAR (req);

	req.count               = 4;
	req.type                = V4L2_BUF_TYPE_VIDEO_CAPTURE;
	req.memory              = V4L2_MEMORY_MMAP;

	if (-1 == xioctl (fd, VIDIOC_REQBUFS, &req)) 
	{
		if (EINVAL == errno) 
		{
			fprintf (stderr, "%s does not support memory mapping\n", dev_name);
			exit (EXIT_FAILURE);
		} 
		else 
		{
			errno_exit ("VIDIOC_REQBUFS");
		}
	}

	if (req.count < 2) 
	{
		fprintf (stderr, "Insufficient buffer memory on %s\n",dev_name);
		exit (EXIT_FAILURE);
	}

	buffers = (buffer*) calloc (req.count, sizeof (*buffers));

	if (!buffers) 
	{
		fprintf (stderr, "Out of memory\n");
		exit (EXIT_FAILURE);
	}

	for (n_buffers = 0; n_buffers < req.count; ++n_buffers) 
	{
		struct v4l2_buffer buf;

		CLEAR (buf);

		buf.type        = V4L2_BUF_TYPE_VIDEO_CAPTURE;
		buf.memory      = V4L2_MEMORY_MMAP;
		buf.index       = n_buffers;

		if (-1 == xioctl (fd, VIDIOC_QUERYBUF, &buf))
			errno_exit ("VIDIOC_QUERYBUF");

		buffers[n_buffers].length = buf.length;
		buffers[n_buffers].start =
				mmap (NULL /* start anywhere */,
					buf.length,
					PROT_READ | PROT_WRITE /* required */,
					MAP_SHARED /* recommended */,
					fd, buf.m.offset);

		if (MAP_FAILED == buffers[n_buffers].start)
			errno_exit ("mmap");
	}
}

Now, I see that the requested videobuffers are correctly allocated via 'vb2_dma_contig_alloc'
call (see [3] for reference). But the MMAP call fails in 'vb2_dma_contig_alloc' function in mm/nommu.c (see [4] for reference) when it tries to make the following check:
	
	if (addr != (pfn << PAGE_SHIFT))
		return -EINVAL;

I address Scott also, as I see that he has worked on the Blackfin v4l2 capture driver using DMA contiguous method and may have seen this issue (on a NOMMU system)
with a v4l2 application performing a MMAP operation.

Any comments on what I could be doing wrong here?

References:

[1] Blackfin capture driver, http://lxr.linux.no/linux+v3.5.3/drivers/media/video/blackfin/bfin_capture.c#L243
[2] capture.c, http://linuxtv.org/downloads/v4l-dvb-apis/capture-example.html
[3] vb2_dma_contig_alloc, http://lxr.linux.no/linux+v3.5.3/drivers/media/video/videobuf2-dma-contig.c#L37
[4] remap_pfn_range, http://lxr.linux.no/linux+v3.5.3/mm/nommu.c#L1819

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] nommu: remap_pfn_range: fix addr parameter check

[Scott Jiang]

>> I was using 3.3 linux kernel. I will again check if videobuf2 in 3.5 has already
>> fixed this issue.
>
> [snip..]
>
> Ok I just checked the vb2_dma_contig allocator and it has no major changes from my version,
> http://lxr.linux.no/linux+v3.5.3/drivers/media/video/videobuf2-dma-contig.c#L37
>
> So, I am not sure if this issue has been fixed in the videobuf2 (or if any patch is in the pipeline
> which fixes the issue).
>
I run my test on our blackfin platform, and all addresses in
remap_pfn_range is aligned for 3.5 branch.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>