Hi Marek, On Tue, Jan 22, 2019 at 8:37 PM Marek Szyprowski <m.szyprow...@samsung.com> wrote: > > Hi Souptick, > > On 2019-01-11 16:11, Souptick Joarder wrote: > > Convert to use vm_insert_range_buggy to map range of kernel memory > > to user vma. > > > > This driver has ignored vm_pgoff. We could later "fix" these drivers > > to behave according to the normal vm_pgoff offsetting simply by > > removing the _buggy suffix on the function name and if that causes > > regressions, it gives us an easy way to revert. > > Just a generic note about videobuf2: videobuf2-dma-sg is ignoring vm_pgoff by > design. vm_pgoff is used as a 'cookie' to select a buffer to mmap and > videobuf2-core already checks that. If userspace provides an offset, which > doesn't match any of the registered 'cookies' (reported to userspace via > separate v4l2 ioctl), an error is returned.
Ok, it means once the buf is selected, videobuf2-dma-sg should always mapped buf->pages[i] from index 0 ( irrespective of vm_pgoff value). So although we are replacing the code with vm_insert_range_buggy(), *_buggy* suffix will mislead others and should not be used. And if we replace this code with vm_insert_range(), this will introduce bug for *non zero* value of vm_pgoff. Please correct me if my understanding is wrong. So what your opinion about this patch ? Shall I drop this patch from current series ? or, There is any better way to handle this scenario ? > > > There is an existing bug inside gem_mmap_obj(), where user passed > > length is not checked against buf->num_pages. For any value of > > length > buf->num_pages it will end up overrun buf->pages[i], > > which could lead to a potential bug. It is not gem_mmap_obj(), it should be vb2_dma_sg_mmap(). Sorry about it. What about this issue ? Does it looks like a valid issue ? > > > > This has been addressed by passing buf->num_pages as input to > > vm_insert_range_buggy() and inside this API error condition is > > checked which will avoid overrun the page boundary. > > > > Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com> > > --- > > drivers/media/common/videobuf2/videobuf2-dma-sg.c | 22 > > ++++++---------------- > > 1 file changed, 6 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > index 015e737..ef046b4 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > @@ -328,28 +328,18 @@ static unsigned int vb2_dma_sg_num_users(void > > *buf_priv) > > static int vb2_dma_sg_mmap(void *buf_priv, struct vm_area_struct *vma) > > { > > struct vb2_dma_sg_buf *buf = buf_priv; > > - unsigned long uaddr = vma->vm_start; > > - unsigned long usize = vma->vm_end - vma->vm_start; > > - int i = 0; > > + int err; > > > > if (!buf) { > > printk(KERN_ERR "No memory to map\n"); > > return -EINVAL; > > } > > > > - do { > > - int ret; > > - > > - ret = vm_insert_page(vma, uaddr, buf->pages[i++]); > > - if (ret) { > > - printk(KERN_ERR "Remapping memory, error: %d\n", ret); > > - return ret; > > - } > > - > > - uaddr += PAGE_SIZE; > > - usize -= PAGE_SIZE; > > - } while (usize > 0); > > - > > + err = vm_insert_range_buggy(vma, buf->pages, buf->num_pages); > > + if (err) { > > + printk(KERN_ERR "Remapping memory, error: %d\n", err); > > + return err; > > + } > > > > /* > > * Use common vm_area operations to track buffer refcount. > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >