| Message ID | 1347504057-5612-1-git-send-email-lliubbo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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
> 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 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
> -----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 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
> -----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 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
>> 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 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
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;
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(-)