While using mmap, the incorrect value of length and vm_pgoff are
ignored and this driver go ahead with mapping fbdev.buffer
to user vma.
Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
"fix" these drivers to behave according to the normal vm_pgoff
offsetting simply by removing the _zero suffix on the function name
and if that causes regressions, it gives us an easy way to revert.
Signed-off-by: Souptick Joarder <[email protected]>
---
drivers/auxdisplay/ht16k33.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 21393ec..9c0bb77 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -223,9 +223,9 @@ static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
struct ht16k33_priv *priv = info->par;
+ struct page *pages = virt_to_page(priv->fbdev.buffer);
- return vm_insert_page(vma, vma->vm_start,
- virt_to_page(priv->fbdev.buffer));
+ return vm_map_pages_zero(vma, &pages, 1);
}
static struct fb_ops ht16k33_fb_ops = {
--
1.9.1
On Mon, 20 May 2019 21:00:58 +0530
Souptick Joarder <[email protected]> wrote:
> While using mmap, the incorrect value of length and vm_pgoff are
> ignored and this driver go ahead with mapping fbdev.buffer
> to user vma.
>
> Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
> "fix" these drivers to behave according to the normal vm_pgoff
> offsetting simply by removing the _zero suffix on the function name
> and if that causes regressions, it gives us an easy way to revert.
>
> Signed-off-by: Souptick Joarder <[email protected]>
> ---
> drivers/auxdisplay/ht16k33.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index 21393ec..9c0bb77 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -223,9 +223,9 @@ static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
> static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> struct ht16k33_priv *priv = info->par;
> + struct page *pages = virt_to_page(priv->fbdev.buffer);
>
> - return vm_insert_page(vma, vma->vm_start,
> - virt_to_page(priv->fbdev.buffer));
> + return vm_map_pages_zero(vma, &pages, 1);
> }
>
> static struct fb_ops ht16k33_fb_ops = {
Acked-by: Robin van der Gracht <[email protected]>
On Tue, May 21, 2019 at 12:24 PM Robin van der Gracht <[email protected]> wrote:
>
> On Mon, 20 May 2019 21:00:58 +0530
> Souptick Joarder <[email protected]> wrote:
>
> > While using mmap, the incorrect value of length and vm_pgoff are
> > ignored and this driver go ahead with mapping fbdev.buffer
> > to user vma.
> >
> > Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
> > "fix" these drivers to behave according to the normal vm_pgoff
> > offsetting simply by removing the _zero suffix on the function name
> > and if that causes regressions, it gives us an easy way to revert.
> >
> > Signed-off-by: Souptick Joarder <[email protected]>
> > ---
> > drivers/auxdisplay/ht16k33.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> > index 21393ec..9c0bb77 100644
> > --- a/drivers/auxdisplay/ht16k33.c
> > +++ b/drivers/auxdisplay/ht16k33.c
> > @@ -223,9 +223,9 @@ static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
> > static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > {
> > struct ht16k33_priv *priv = info->par;
> > + struct page *pages = virt_to_page(priv->fbdev.buffer);
> >
> > - return vm_insert_page(vma, vma->vm_start,
> > - virt_to_page(priv->fbdev.buffer));
> > + return vm_map_pages_zero(vma, &pages, 1);
> > }
> >
> > static struct fb_ops ht16k33_fb_ops = {
>
> Acked-by: Robin van der Gracht <[email protected]>
Miguel, Ack from Robin is missing in linux-next-20190523 when applied.
On Thu, May 23, 2019 at 10:18 AM Souptick Joarder <[email protected]> wrote:
>
> Miguel, Ack from Robin is missing in linux-next-20190523 when applied.
Thanks for the warning! I wanted to review first but didn't get to it yet.
Taking a quick look now, by the way, why does vm_map_pages_zero() (and
__vm_map_pages() etc.) get a pointer to an array instead of a pointer
to the first element?
Cheers,
Miguel
On Mon, May 20, 2019 at 5:26 PM Souptick Joarder <[email protected]> wrote:
>
> While using mmap, the incorrect value of length and vm_pgoff are
> ignored and this driver go ahead with mapping fbdev.buffer
> to user vma.
Typos: values*, goes* (same for the other patch)
> Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
> "fix" these drivers to behave according to the normal vm_pgoff
> offsetting simply by removing the _zero suffix on the function name
> and if that causes regressions, it gives us an easy way to revert.
Would it be possible to add a "Link:" to where these new functions are
discussed/used (maybe a lore.kernel.org link?)?
Thanks for the patch!
Cheers,
Miguel
On Thu, May 23, 2019 at 2:58 PM Miguel Ojeda
<[email protected]> wrote:
>
> Taking a quick look now, by the way, why does vm_map_pages_zero() (and
> __vm_map_pages() etc.) get a pointer to an array instead of a pointer
> to the first element?
Also, in __vm_map_pages(), semantically w.r.t. to the comment,
shouldn't the first check test for equality too? (i.e. for vm_pgoff ==
num)? (even if such case fails in the second test anyway).
Cheers,
Miguel
On Fri, May 24, 2019 at 9:52 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Thu, May 23, 2019 at 2:58 PM Miguel Ojeda
> <[email protected]> wrote:
> >
> > Taking a quick look now, by the way, why does vm_map_pages_zero() (and
> > __vm_map_pages() etc.) get a pointer to an array instead of a pointer
> > to the first element?
For this particular driver, one page is getting mapped into vma. But
there are other
places where a entire page array ( with more than one pages) mapped into
vma. That's the reason to pass pointer to an array and do rest of the operations
inside __vm_map_pages().
https://lkml.org/lkml/2019/3/18/1265
>
> Also, in __vm_map_pages(), semantically w.r.t. to the comment,
> shouldn't the first check test for equality too? (i.e. for vm_pgoff ==
> num)? (even if such case fails in the second test anyway).
Sorry, didn't get it. Do you mean there should be a separate check for
*vm_pgoff == num* ?
> Cheers,
> Miguel
On Thu, May 23, 2019 at 6:53 PM Miguel Ojeda
<[email protected]> wrote:
>
> On Mon, May 20, 2019 at 5:26 PM Souptick Joarder <[email protected]> wrote:
> >
> > While using mmap, the incorrect value of length and vm_pgoff are
> > ignored and this driver go ahead with mapping fbdev.buffer
> > to user vma.
>
> Typos: values*, goes* (same for the other patch)
Ok, will add it into v2.
>
> > Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
> > "fix" these drivers to behave according to the normal vm_pgoff
> > offsetting simply by removing the _zero suffix on the function name
> > and if that causes regressions, it gives us an easy way to revert.
>
> Would it be possible to add a "Link:" to where these new functions are
> discussed/used (maybe a lore.kernel.org link?)?
This might be helpful.
https://lkml.org/lkml/2018/12/24/204
>
> Thanks for the patch!
>
> Cheers,
> Miguel
On Fri, May 24, 2019 at 8:07 AM Souptick Joarder <[email protected]> wrote:
>
> > > Taking a quick look now, by the way, why does vm_map_pages_zero() (and
> > > __vm_map_pages() etc.) get a pointer to an array instead of a pointer
> > > to the first element?
>
> For this particular driver, one page is getting mapped into vma. But
> there are other
> places where a entire page array ( with more than one pages) mapped into
> vma. That's the reason to pass pointer to an array and do rest of the operations
> inside __vm_map_pages().
Ah, "pointer to array of source kernel pages" made me think the actual
`struct page`s were the ones consecutive in memory, not the pointers.
Maybe "array of page pointers" is more clear.
> > Also, in __vm_map_pages(), semantically w.r.t. to the comment,
> > shouldn't the first check test for equality too? (i.e. for vm_pgoff ==
> > num)? (even if such case fails in the second test anyway).
>
> Sorry, didn't get it. Do you mean there should be a separate check for
> *vm_pgoff == num* ?
No, that the first check should be widened. I will send a patch.
Cheers,
Miguel