* [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-02-15 2:48 Souptick Joarder 2019-07-28 18:06 ` Marek Marczykowski-Górecki 0 siblings, 1 reply; 23+ messages in thread From: Souptick Joarder @ 2019-02-15 2:48 UTC (permalink / raw) To: akpm, willy, mhocko, boris.ostrovsky, jgross, linux, robin.murphy Cc: xen-devel, linux-kernel, linux-mm Convert to use vm_map_pages() to map range of kernel memory to user vma. map->count is passed to vm_map_pages() and internal API verify map->count against count ( count = vma_pages(vma)) for page array boundary overrun condition. Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- drivers/xen/gntdev.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 5efc5ee..5d64262 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) int index = vma->vm_pgoff; int count = vma_pages(vma); struct gntdev_grant_map *map; - int i, err = -EINVAL; + int err = -EINVAL; if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) return -EINVAL; @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) goto out_put_map; if (!use_ptemod) { - for (i = 0; i < count; i++) { - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, - map->pages[i]); - if (err) - goto out_put_map; - } + err = vm_map_pages(vma, map->pages, map->count); + if (err) + goto out_put_map; } else { #ifdef CONFIG_X86 /* -- 1.9.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() 2019-02-15 2:48 [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() Souptick Joarder @ 2019-07-28 18:06 ` Marek Marczykowski-Górecki 0 siblings, 0 replies; 23+ messages in thread From: Marek Marczykowski-Górecki @ 2019-07-28 18:06 UTC (permalink / raw) To: Souptick Joarder Cc: akpm, willy, mhocko, boris.ostrovsky, jgross, linux, robin.murphy, xen-devel, linux-kernel, linux-mm [-- Attachment #1: Type: text/plain, Size: 2088 bytes --] On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > Convert to use vm_map_pages() to map range of kernel > memory to user vma. > > map->count is passed to vm_map_pages() and internal API > verify map->count against count ( count = vma_pages(vma)) > for page array boundary overrun condition. This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages will: - use map->pages starting at vma->vm_pgoff instead of 0 - verify map->count against vma_pages()+vma->vm_pgoff instead of just vma_pages(). In practice, this breaks using a single gntdev FD for mapping multiple grants. It looks like vm_map_pages() is not a good fit for this code and IMO it should be reverted. > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > --- > drivers/xen/gntdev.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > index 5efc5ee..5d64262 100644 > --- a/drivers/xen/gntdev.c > +++ b/drivers/xen/gntdev.c > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > int index = vma->vm_pgoff; > int count = vma_pages(vma); > struct gntdev_grant_map *map; > - int i, err = -EINVAL; > + int err = -EINVAL; > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > return -EINVAL; > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > goto out_put_map; > > if (!use_ptemod) { > - for (i = 0; i < count; i++) { > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > - map->pages[i]); > - if (err) > - goto out_put_map; > - } > + err = vm_map_pages(vma, map->pages, map->count); > + if (err) > + goto out_put_map; > } else { > #ifdef CONFIG_X86 > /* -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-28 18:06 ` Marek Marczykowski-Górecki 0 siblings, 0 replies; 23+ messages in thread From: Marek Marczykowski-Górecki @ 2019-07-28 18:06 UTC (permalink / raw) To: Souptick Joarder Cc: jgross, mhocko, linux, willy, linux-kernel, linux-mm, xen-devel, akpm, robin.murphy, boris.ostrovsky [-- Attachment #1.1: Type: text/plain, Size: 2088 bytes --] On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > Convert to use vm_map_pages() to map range of kernel > memory to user vma. > > map->count is passed to vm_map_pages() and internal API > verify map->count against count ( count = vma_pages(vma)) > for page array boundary overrun condition. This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages will: - use map->pages starting at vma->vm_pgoff instead of 0 - verify map->count against vma_pages()+vma->vm_pgoff instead of just vma_pages(). In practice, this breaks using a single gntdev FD for mapping multiple grants. It looks like vm_map_pages() is not a good fit for this code and IMO it should be reverted. > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > --- > drivers/xen/gntdev.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > index 5efc5ee..5d64262 100644 > --- a/drivers/xen/gntdev.c > +++ b/drivers/xen/gntdev.c > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > int index = vma->vm_pgoff; > int count = vma_pages(vma); > struct gntdev_grant_map *map; > - int i, err = -EINVAL; > + int err = -EINVAL; > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > return -EINVAL; > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > goto out_put_map; > > if (!use_ptemod) { > - for (i = 0; i < count; i++) { > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > - map->pages[i]); > - if (err) > - goto out_put_map; > - } > + err = vm_map_pages(vma, map->pages, map->count); > + if (err) > + goto out_put_map; > } else { > #ifdef CONFIG_X86 > /* -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() 2019-07-28 18:06 ` Marek Marczykowski-Górecki (?) @ 2019-07-29 8:05 ` Souptick Joarder -1 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-29 8:05 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, Boris Ostrovsky, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > Convert to use vm_map_pages() to map range of kernel > > memory to user vma. > > > > map->count is passed to vm_map_pages() and internal API > > verify map->count against count ( count = vma_pages(vma)) > > for page array boundary overrun condition. > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > will: > - use map->pages starting at vma->vm_pgoff instead of 0 The actual code ignores vma->vm_pgoff > 0 scenario and mapped the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped if vma->vm_pgoff > 0 (in original code) ? are you referring to set vma->vm_pgoff = 0 irrespective of value passed from user space ? If yes, using vm_map_pages_zero() is an alternate option. > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > vma_pages(). In original code -> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 559d4b7f807d..469dfbd6cf90 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) int index = vma->vm_pgoff; int count = vma_pages(vma); Count is user passed value. struct gntdev_grant_map *map; - int i, err = -EINVAL; + int err = -EINVAL; if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) return -EINVAL; @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) goto out_put_map; if (!use_ptemod) { - for (i = 0; i < count; i++) { - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, - map->pages[i]); and when count > i , we end up with trying to map memory outside boundary of map->pages[i], which was not correct. - if (err) - goto out_put_map; - } + err = vm_map_pages(vma, map->pages, map->count); + if (err) + goto out_put_map; With this commit, inside __vm_map_pages(), we have addressed this scenario. +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, + unsigned long num, unsigned long offset) +{ + unsigned long count = vma_pages(vma); + unsigned long uaddr = vma->vm_start; + int ret, i; + + /* Fail if the user requested offset is beyond the end of the object */ + if (offset > num) + return -ENXIO; + + /* Fail if the user requested size exceeds available object size */ + if (count > num - offset) + return -ENXIO; By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). So we will never cross the boundary of map->pages[i]. > > In practice, this breaks using a single gntdev FD for mapping multiple > grants. How ? > > It looks like vm_map_pages() is not a good fit for this code and IMO it > should be reverted. Did you hit any issue around this code in real time ? > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > --- > > drivers/xen/gntdev.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > index 5efc5ee..5d64262 100644 > > --- a/drivers/xen/gntdev.c > > +++ b/drivers/xen/gntdev.c > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > int index = vma->vm_pgoff; > > int count = vma_pages(vma); > > struct gntdev_grant_map *map; > > - int i, err = -EINVAL; > > + int err = -EINVAL; > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > return -EINVAL; > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > goto out_put_map; > > > > if (!use_ptemod) { > > - for (i = 0; i < count; i++) { > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > - map->pages[i]); > > - if (err) > > - goto out_put_map; > > - } > > + err = vm_map_pages(vma, map->pages, map->count); > > + if (err) > > + goto out_put_map; > > } else { > > #ifdef CONFIG_X86 > > /* > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-29 8:05 ` Souptick Joarder 0 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-29 8:05 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Juergen Gross, Michal Hocko, Russell King - ARM Linux, Matthew Wilcox, linux-kernel, Linux-MM, xen-devel, Andrew Morton, robin.murphy, Boris Ostrovsky On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > Convert to use vm_map_pages() to map range of kernel > > memory to user vma. > > > > map->count is passed to vm_map_pages() and internal API > > verify map->count against count ( count = vma_pages(vma)) > > for page array boundary overrun condition. > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > will: > - use map->pages starting at vma->vm_pgoff instead of 0 The actual code ignores vma->vm_pgoff > 0 scenario and mapped the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped if vma->vm_pgoff > 0 (in original code) ? are you referring to set vma->vm_pgoff = 0 irrespective of value passed from user space ? If yes, using vm_map_pages_zero() is an alternate option. > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > vma_pages(). In original code -> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 559d4b7f807d..469dfbd6cf90 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) int index = vma->vm_pgoff; int count = vma_pages(vma); Count is user passed value. struct gntdev_grant_map *map; - int i, err = -EINVAL; + int err = -EINVAL; if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) return -EINVAL; @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) goto out_put_map; if (!use_ptemod) { - for (i = 0; i < count; i++) { - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, - map->pages[i]); and when count > i , we end up with trying to map memory outside boundary of map->pages[i], which was not correct. - if (err) - goto out_put_map; - } + err = vm_map_pages(vma, map->pages, map->count); + if (err) + goto out_put_map; With this commit, inside __vm_map_pages(), we have addressed this scenario. +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, + unsigned long num, unsigned long offset) +{ + unsigned long count = vma_pages(vma); + unsigned long uaddr = vma->vm_start; + int ret, i; + + /* Fail if the user requested offset is beyond the end of the object */ + if (offset > num) + return -ENXIO; + + /* Fail if the user requested size exceeds available object size */ + if (count > num - offset) + return -ENXIO; By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). So we will never cross the boundary of map->pages[i]. > > In practice, this breaks using a single gntdev FD for mapping multiple > grants. How ? > > It looks like vm_map_pages() is not a good fit for this code and IMO it > should be reverted. Did you hit any issue around this code in real time ? > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > --- > > drivers/xen/gntdev.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > index 5efc5ee..5d64262 100644 > > --- a/drivers/xen/gntdev.c > > +++ b/drivers/xen/gntdev.c > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > int index = vma->vm_pgoff; > > int count = vma_pages(vma); > > struct gntdev_grant_map *map; > > - int i, err = -EINVAL; > > + int err = -EINVAL; > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > return -EINVAL; > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > goto out_put_map; > > > > if (!use_ptemod) { > > - for (i = 0; i < count; i++) { > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > - map->pages[i]); > > - if (err) > > - goto out_put_map; > > - } > > + err = vm_map_pages(vma, map->pages, map->count); > > + if (err) > > + goto out_put_map; > > } else { > > #ifdef CONFIG_X86 > > /* > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-29 8:05 ` Souptick Joarder 0 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-29 8:05 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, Boris Ostrovsky, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > Convert to use vm_map_pages() to map range of kernel > > memory to user vma. > > > > map->count is passed to vm_map_pages() and internal API > > verify map->count against count ( count = vma_pages(vma)) > > for page array boundary overrun condition. > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > will: > - use map->pages starting at vma->vm_pgoff instead of 0 The actual code ignores vma->vm_pgoff > 0 scenario and mapped the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped if vma->vm_pgoff > 0 (in original code) ? are you referring to set vma->vm_pgoff = 0 irrespective of value passed from user space ? If yes, using vm_map_pages_zero() is an alternate option. > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > vma_pages(). In original code -> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 559d4b7f807d..469dfbd6cf90 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) int index = vma->vm_pgoff; int count = vma_pages(vma); Count is user passed value. struct gntdev_grant_map *map; - int i, err = -EINVAL; + int err = -EINVAL; if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) return -EINVAL; @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) goto out_put_map; if (!use_ptemod) { - for (i = 0; i < count; i++) { - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, - map->pages[i]); and when count > i , we end up with trying to map memory outside boundary of map->pages[i], which was not correct. - if (err) - goto out_put_map; - } + err = vm_map_pages(vma, map->pages, map->count); + if (err) + goto out_put_map; With this commit, inside __vm_map_pages(), we have addressed this scenario. +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, + unsigned long num, unsigned long offset) +{ + unsigned long count = vma_pages(vma); + unsigned long uaddr = vma->vm_start; + int ret, i; + + /* Fail if the user requested offset is beyond the end of the object */ + if (offset > num) + return -ENXIO; + + /* Fail if the user requested size exceeds available object size */ + if (count > num - offset) + return -ENXIO; By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). So we will never cross the boundary of map->pages[i]. > > In practice, this breaks using a single gntdev FD for mapping multiple > grants. How ? > > It looks like vm_map_pages() is not a good fit for this code and IMO it > should be reverted. Did you hit any issue around this code in real time ? > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > --- > > drivers/xen/gntdev.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > index 5efc5ee..5d64262 100644 > > --- a/drivers/xen/gntdev.c > > +++ b/drivers/xen/gntdev.c > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > int index = vma->vm_pgoff; > > int count = vma_pages(vma); > > struct gntdev_grant_map *map; > > - int i, err = -EINVAL; > > + int err = -EINVAL; > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > return -EINVAL; > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > goto out_put_map; > > > > if (!use_ptemod) { > > - for (i = 0; i < count; i++) { > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > - map->pages[i]); > > - if (err) > > - goto out_put_map; > > - } > > + err = vm_map_pages(vma, map->pages, map->count); > > + if (err) > > + goto out_put_map; > > } else { > > #ifdef CONFIG_X86 > > /* > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() 2019-07-29 8:05 ` Souptick Joarder (?) @ 2019-07-29 8:32 ` Souptick Joarder -1 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-29 8:32 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, Boris Ostrovsky, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: > > > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > Convert to use vm_map_pages() to map range of kernel > > > memory to user vma. > > > > > > map->count is passed to vm_map_pages() and internal API > > > verify map->count against count ( count = vma_pages(vma)) > > > for page array boundary overrun condition. > > > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > will: > > - use map->pages starting at vma->vm_pgoff instead of 0 > > The actual code ignores vma->vm_pgoff > 0 scenario and mapped > the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > if vma->vm_pgoff > 0 (in original code) ? > > are you referring to set vma->vm_pgoff = 0 irrespective of value passed > from user space ? If yes, using vm_map_pages_zero() is an alternate > option. > > > > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > > vma_pages(). > > In original code -> > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > index 559d4b7f807d..469dfbd6cf90 100644 > --- a/drivers/xen/gntdev.c > +++ b/drivers/xen/gntdev.c > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct > vm_area_struct *vma) > int index = vma->vm_pgoff; > int count = vma_pages(vma); > > Count is user passed value. > > struct gntdev_grant_map *map; > - int i, err = -EINVAL; > + int err = -EINVAL; > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > return -EINVAL; > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, > struct vm_area_struct *vma) > goto out_put_map; > if (!use_ptemod) { > - for (i = 0; i < count; i++) { > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > - map->pages[i]); > > and when count > i , we end up with trying to map memory outside > boundary of map->pages[i], which was not correct. typo. s/count > i / count > map->count > > - if (err) > - goto out_put_map; > - } > + err = vm_map_pages(vma, map->pages, map->count); > + if (err) > + goto out_put_map; > > With this commit, inside __vm_map_pages(), we have addressed this scenario. > > +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, > + unsigned long num, unsigned long offset) > +{ > + unsigned long count = vma_pages(vma); > + unsigned long uaddr = vma->vm_start; > + int ret, i; > + > + /* Fail if the user requested offset is beyond the end of the object */ > + if (offset > num) > + return -ENXIO; > + > + /* Fail if the user requested size exceeds available object size */ > + if (count > num - offset) > + return -ENXIO; > > By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). > So we will never cross the boundary of map->pages[i]. > > > > > > In practice, this breaks using a single gntdev FD for mapping multiple > > grants. > > How ? > > > > > It looks like vm_map_pages() is not a good fit for this code and IMO it > > should be reverted. > > Did you hit any issue around this code in real time ? > > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > --- > > > drivers/xen/gntdev.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > index 5efc5ee..5d64262 100644 > > > --- a/drivers/xen/gntdev.c > > > +++ b/drivers/xen/gntdev.c > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > int index = vma->vm_pgoff; > > > int count = vma_pages(vma); > > > struct gntdev_grant_map *map; > > > - int i, err = -EINVAL; > > > + int err = -EINVAL; > > > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > return -EINVAL; > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > goto out_put_map; > > > > > > if (!use_ptemod) { > > > - for (i = 0; i < count; i++) { > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > - map->pages[i]); > > > - if (err) > > > - goto out_put_map; > > > - } > > > + err = vm_map_pages(vma, map->pages, map->count); > > > + if (err) > > > + goto out_put_map; > > > } else { > > > #ifdef CONFIG_X86 > > > /* > > > > -- > > Best Regards, > > Marek Marczykowski-Górecki > > Invisible Things Lab > > A: Because it messes up the order in which people normally read text. > > Q: Why is top-posting such a bad thing? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-29 8:32 ` Souptick Joarder 0 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-29 8:32 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Juergen Gross, Michal Hocko, Russell King - ARM Linux, Matthew Wilcox, linux-kernel, Linux-MM, xen-devel, Andrew Morton, robin.murphy, Boris Ostrovsky On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: > > > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > Convert to use vm_map_pages() to map range of kernel > > > memory to user vma. > > > > > > map->count is passed to vm_map_pages() and internal API > > > verify map->count against count ( count = vma_pages(vma)) > > > for page array boundary overrun condition. > > > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > will: > > - use map->pages starting at vma->vm_pgoff instead of 0 > > The actual code ignores vma->vm_pgoff > 0 scenario and mapped > the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > if vma->vm_pgoff > 0 (in original code) ? > > are you referring to set vma->vm_pgoff = 0 irrespective of value passed > from user space ? If yes, using vm_map_pages_zero() is an alternate > option. > > > > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > > vma_pages(). > > In original code -> > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > index 559d4b7f807d..469dfbd6cf90 100644 > --- a/drivers/xen/gntdev.c > +++ b/drivers/xen/gntdev.c > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct > vm_area_struct *vma) > int index = vma->vm_pgoff; > int count = vma_pages(vma); > > Count is user passed value. > > struct gntdev_grant_map *map; > - int i, err = -EINVAL; > + int err = -EINVAL; > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > return -EINVAL; > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, > struct vm_area_struct *vma) > goto out_put_map; > if (!use_ptemod) { > - for (i = 0; i < count; i++) { > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > - map->pages[i]); > > and when count > i , we end up with trying to map memory outside > boundary of map->pages[i], which was not correct. typo. s/count > i / count > map->count > > - if (err) > - goto out_put_map; > - } > + err = vm_map_pages(vma, map->pages, map->count); > + if (err) > + goto out_put_map; > > With this commit, inside __vm_map_pages(), we have addressed this scenario. > > +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, > + unsigned long num, unsigned long offset) > +{ > + unsigned long count = vma_pages(vma); > + unsigned long uaddr = vma->vm_start; > + int ret, i; > + > + /* Fail if the user requested offset is beyond the end of the object */ > + if (offset > num) > + return -ENXIO; > + > + /* Fail if the user requested size exceeds available object size */ > + if (count > num - offset) > + return -ENXIO; > > By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). > So we will never cross the boundary of map->pages[i]. > > > > > > In practice, this breaks using a single gntdev FD for mapping multiple > > grants. > > How ? > > > > > It looks like vm_map_pages() is not a good fit for this code and IMO it > > should be reverted. > > Did you hit any issue around this code in real time ? > > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > --- > > > drivers/xen/gntdev.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > index 5efc5ee..5d64262 100644 > > > --- a/drivers/xen/gntdev.c > > > +++ b/drivers/xen/gntdev.c > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > int index = vma->vm_pgoff; > > > int count = vma_pages(vma); > > > struct gntdev_grant_map *map; > > > - int i, err = -EINVAL; > > > + int err = -EINVAL; > > > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > return -EINVAL; > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > goto out_put_map; > > > > > > if (!use_ptemod) { > > > - for (i = 0; i < count; i++) { > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > - map->pages[i]); > > > - if (err) > > > - goto out_put_map; > > > - } > > > + err = vm_map_pages(vma, map->pages, map->count); > > > + if (err) > > > + goto out_put_map; > > > } else { > > > #ifdef CONFIG_X86 > > > /* > > > > -- > > Best Regards, > > Marek Marczykowski-Górecki > > Invisible Things Lab > > A: Because it messes up the order in which people normally read text. > > Q: Why is top-posting such a bad thing? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-29 8:32 ` Souptick Joarder 0 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-29 8:32 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, Boris Ostrovsky, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: > > > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > Convert to use vm_map_pages() to map range of kernel > > > memory to user vma. > > > > > > map->count is passed to vm_map_pages() and internal API > > > verify map->count against count ( count = vma_pages(vma)) > > > for page array boundary overrun condition. > > > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > will: > > - use map->pages starting at vma->vm_pgoff instead of 0 > > The actual code ignores vma->vm_pgoff > 0 scenario and mapped > the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > if vma->vm_pgoff > 0 (in original code) ? > > are you referring to set vma->vm_pgoff = 0 irrespective of value passed > from user space ? If yes, using vm_map_pages_zero() is an alternate > option. > > > > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > > vma_pages(). > > In original code -> > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > index 559d4b7f807d..469dfbd6cf90 100644 > --- a/drivers/xen/gntdev.c > +++ b/drivers/xen/gntdev.c > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct > vm_area_struct *vma) > int index = vma->vm_pgoff; > int count = vma_pages(vma); > > Count is user passed value. > > struct gntdev_grant_map *map; > - int i, err = -EINVAL; > + int err = -EINVAL; > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > return -EINVAL; > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, > struct vm_area_struct *vma) > goto out_put_map; > if (!use_ptemod) { > - for (i = 0; i < count; i++) { > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > - map->pages[i]); > > and when count > i , we end up with trying to map memory outside > boundary of map->pages[i], which was not correct. typo. s/count > i / count > map->count > > - if (err) > - goto out_put_map; > - } > + err = vm_map_pages(vma, map->pages, map->count); > + if (err) > + goto out_put_map; > > With this commit, inside __vm_map_pages(), we have addressed this scenario. > > +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, > + unsigned long num, unsigned long offset) > +{ > + unsigned long count = vma_pages(vma); > + unsigned long uaddr = vma->vm_start; > + int ret, i; > + > + /* Fail if the user requested offset is beyond the end of the object */ > + if (offset > num) > + return -ENXIO; > + > + /* Fail if the user requested size exceeds available object size */ > + if (count > num - offset) > + return -ENXIO; > > By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). > So we will never cross the boundary of map->pages[i]. > > > > > > In practice, this breaks using a single gntdev FD for mapping multiple > > grants. > > How ? > > > > > It looks like vm_map_pages() is not a good fit for this code and IMO it > > should be reverted. > > Did you hit any issue around this code in real time ? > > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > --- > > > drivers/xen/gntdev.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > index 5efc5ee..5d64262 100644 > > > --- a/drivers/xen/gntdev.c > > > +++ b/drivers/xen/gntdev.c > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > int index = vma->vm_pgoff; > > > int count = vma_pages(vma); > > > struct gntdev_grant_map *map; > > > - int i, err = -EINVAL; > > > + int err = -EINVAL; > > > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > return -EINVAL; > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > goto out_put_map; > > > > > > if (!use_ptemod) { > > > - for (i = 0; i < count; i++) { > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > - map->pages[i]); > > > - if (err) > > > - goto out_put_map; > > > - } > > > + err = vm_map_pages(vma, map->pages, map->count); > > > + if (err) > > > + goto out_put_map; > > > } else { > > > #ifdef CONFIG_X86 > > > /* > > > > -- > > Best Regards, > > Marek Marczykowski-Górecki > > Invisible Things Lab > > A: Because it messes up the order in which people normally read text. > > Q: Why is top-posting such a bad thing? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() 2019-07-29 8:32 ` Souptick Joarder @ 2019-07-29 13:36 ` Marek Marczykowski-Górecki -1 siblings, 0 replies; 23+ messages in thread From: Marek Marczykowski-Górecki @ 2019-07-29 13:36 UTC (permalink / raw) To: Souptick Joarder Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, Boris Ostrovsky, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM [-- Attachment #1: Type: text/plain, Size: 6813 bytes --] On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> wrote: > > > > > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > > Convert to use vm_map_pages() to map range of kernel > > > > memory to user vma. > > > > > > > > map->count is passed to vm_map_pages() and internal API > > > > verify map->count against count ( count = vma_pages(vma)) > > > > for page array boundary overrun condition. > > > > > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > > will: > > > - use map->pages starting at vma->vm_pgoff instead of 0 > > > > The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > if vma->vm_pgoff > 0 (in original code) ? vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's basically (ab)using this parameter for "which grant reference to map". > > are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > from user space ? If yes, using vm_map_pages_zero() is an alternate > > option. Yes, that should work. > > > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > > > vma_pages(). > > > > In original code -> > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > index 559d4b7f807d..469dfbd6cf90 100644 > > --- a/drivers/xen/gntdev.c > > +++ b/drivers/xen/gntdev.c > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct > > vm_area_struct *vma) > > int index = vma->vm_pgoff; > > int count = vma_pages(vma); > > > > Count is user passed value. > > > > struct gntdev_grant_map *map; > > - int i, err = -EINVAL; > > + int err = -EINVAL; > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > return -EINVAL; > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, > > struct vm_area_struct *vma) > > goto out_put_map; > > if (!use_ptemod) { > > - for (i = 0; i < count; i++) { > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > - map->pages[i]); > > > > and when count > i , we end up with trying to map memory outside > > boundary of map->pages[i], which was not correct. > > typo. > s/count > i / count > map->count gntdev_find_map_index verifies it. Specifically, it looks for a map matching both index and count. > > > > - if (err) > > - goto out_put_map; > > - } > > + err = vm_map_pages(vma, map->pages, map->count); > > + if (err) > > + goto out_put_map; > > > > With this commit, inside __vm_map_pages(), we have addressed this scenario. > > > > +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, > > + unsigned long num, unsigned long offset) > > +{ > > + unsigned long count = vma_pages(vma); > > + unsigned long uaddr = vma->vm_start; > > + int ret, i; > > + > > + /* Fail if the user requested offset is beyond the end of the object */ > > + if (offset > num) > > + return -ENXIO; > > + > > + /* Fail if the user requested size exceeds available object size */ > > + if (count > num - offset) > > + return -ENXIO; > > > > By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). > > So we will never cross the boundary of map->pages[i]. > > > > > > > > > > In practice, this breaks using a single gntdev FD for mapping multiple > > > grants. > > > > How ? gntdev uses vma->vm_pgoff to select which grant entry should be mapped. map struct returned by gntdev_find_map_index() describes just the pages to be mapped. Specifically map->pages[0] should be mapped at vma->vm_start, not vma->vm_start+vma->vm_pgoff*PAGE_SIZE. When trying to map grant with index (aka vma->vm_pgoff) > 1, __vm_map_pages() will refuse to map it because it will expect map->count to be at least vma_pages(vma)+vma->vm_pgoff, while it is exactly vma_pages(vma). > > > It looks like vm_map_pages() is not a good fit for this code and IMO it > > > should be reverted. > > > > Did you hit any issue around this code in real time ? Yes, relevant strace output: [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0) = 0x777f1211b000 [pid 857] ioctl(7, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, 0x7ffd3407b710) = 0 [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0x1000) = -1 ENXIO (No such device or address) details here: https://github.com/QubesOS/qubes-issues/issues/5199 > > > > > > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > --- > > > > drivers/xen/gntdev.c | 11 ++++------- > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > > index 5efc5ee..5d64262 100644 > > > > --- a/drivers/xen/gntdev.c > > > > +++ b/drivers/xen/gntdev.c > > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > int index = vma->vm_pgoff; > > > > int count = vma_pages(vma); > > > > struct gntdev_grant_map *map; > > > > - int i, err = -EINVAL; > > > > + int err = -EINVAL; > > > > > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > > return -EINVAL; > > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > goto out_put_map; > > > > > > > > if (!use_ptemod) { > > > > - for (i = 0; i < count; i++) { > > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > > - map->pages[i]); > > > > - if (err) > > > > - goto out_put_map; > > > > - } > > > > + err = vm_map_pages(vma, map->pages, map->count); > > > > + if (err) > > > > + goto out_put_map; > > > > } else { > > > > #ifdef CONFIG_X86 > > > > /* > > > > > > -- > > > Best Regards, > > > Marek Marczykowski-Górecki > > > Invisible Things Lab > > > A: Because it messes up the order in which people normally read text. > > > Q: Why is top-posting such a bad thing? -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-29 13:36 ` Marek Marczykowski-Górecki 0 siblings, 0 replies; 23+ messages in thread From: Marek Marczykowski-Górecki @ 2019-07-29 13:36 UTC (permalink / raw) To: Souptick Joarder Cc: Juergen Gross, Michal Hocko, Russell King - ARM Linux, Matthew Wilcox, linux-kernel, Linux-MM, xen-devel, Andrew Morton, robin.murphy, Boris Ostrovsky [-- Attachment #1.1: Type: text/plain, Size: 6813 bytes --] On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> wrote: > > > > > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > > Convert to use vm_map_pages() to map range of kernel > > > > memory to user vma. > > > > > > > > map->count is passed to vm_map_pages() and internal API > > > > verify map->count against count ( count = vma_pages(vma)) > > > > for page array boundary overrun condition. > > > > > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > > will: > > > - use map->pages starting at vma->vm_pgoff instead of 0 > > > > The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > if vma->vm_pgoff > 0 (in original code) ? vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's basically (ab)using this parameter for "which grant reference to map". > > are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > from user space ? If yes, using vm_map_pages_zero() is an alternate > > option. Yes, that should work. > > > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > > > vma_pages(). > > > > In original code -> > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > index 559d4b7f807d..469dfbd6cf90 100644 > > --- a/drivers/xen/gntdev.c > > +++ b/drivers/xen/gntdev.c > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct > > vm_area_struct *vma) > > int index = vma->vm_pgoff; > > int count = vma_pages(vma); > > > > Count is user passed value. > > > > struct gntdev_grant_map *map; > > - int i, err = -EINVAL; > > + int err = -EINVAL; > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > return -EINVAL; > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, > > struct vm_area_struct *vma) > > goto out_put_map; > > if (!use_ptemod) { > > - for (i = 0; i < count; i++) { > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > - map->pages[i]); > > > > and when count > i , we end up with trying to map memory outside > > boundary of map->pages[i], which was not correct. > > typo. > s/count > i / count > map->count gntdev_find_map_index verifies it. Specifically, it looks for a map matching both index and count. > > > > - if (err) > > - goto out_put_map; > > - } > > + err = vm_map_pages(vma, map->pages, map->count); > > + if (err) > > + goto out_put_map; > > > > With this commit, inside __vm_map_pages(), we have addressed this scenario. > > > > +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, > > + unsigned long num, unsigned long offset) > > +{ > > + unsigned long count = vma_pages(vma); > > + unsigned long uaddr = vma->vm_start; > > + int ret, i; > > + > > + /* Fail if the user requested offset is beyond the end of the object */ > > + if (offset > num) > > + return -ENXIO; > > + > > + /* Fail if the user requested size exceeds available object size */ > > + if (count > num - offset) > > + return -ENXIO; > > > > By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). > > So we will never cross the boundary of map->pages[i]. > > > > > > > > > > In practice, this breaks using a single gntdev FD for mapping multiple > > > grants. > > > > How ? gntdev uses vma->vm_pgoff to select which grant entry should be mapped. map struct returned by gntdev_find_map_index() describes just the pages to be mapped. Specifically map->pages[0] should be mapped at vma->vm_start, not vma->vm_start+vma->vm_pgoff*PAGE_SIZE. When trying to map grant with index (aka vma->vm_pgoff) > 1, __vm_map_pages() will refuse to map it because it will expect map->count to be at least vma_pages(vma)+vma->vm_pgoff, while it is exactly vma_pages(vma). > > > It looks like vm_map_pages() is not a good fit for this code and IMO it > > > should be reverted. > > > > Did you hit any issue around this code in real time ? Yes, relevant strace output: [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0) = 0x777f1211b000 [pid 857] ioctl(7, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, 0x7ffd3407b710) = 0 [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0x1000) = -1 ENXIO (No such device or address) details here: https://github.com/QubesOS/qubes-issues/issues/5199 > > > > > > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > --- > > > > drivers/xen/gntdev.c | 11 ++++------- > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > > index 5efc5ee..5d64262 100644 > > > > --- a/drivers/xen/gntdev.c > > > > +++ b/drivers/xen/gntdev.c > > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > int index = vma->vm_pgoff; > > > > int count = vma_pages(vma); > > > > struct gntdev_grant_map *map; > > > > - int i, err = -EINVAL; > > > > + int err = -EINVAL; > > > > > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > > return -EINVAL; > > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > goto out_put_map; > > > > > > > > if (!use_ptemod) { > > > > - for (i = 0; i < count; i++) { > > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > > - map->pages[i]); > > > > - if (err) > > > > - goto out_put_map; > > > > - } > > > > + err = vm_map_pages(vma, map->pages, map->count); > > > > + if (err) > > > > + goto out_put_map; > > > > } else { > > > > #ifdef CONFIG_X86 > > > > /* > > > > > > -- > > > Best Regards, > > > Marek Marczykowski-Górecki > > > Invisible Things Lab > > > A: Because it messes up the order in which people normally read text. > > > Q: Why is top-posting such a bad thing? -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() 2019-07-29 13:36 ` Marek Marczykowski-Górecki (?) @ 2019-07-30 6:03 ` Souptick Joarder -1 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-30 6:03 UTC (permalink / raw) To: Marek Marczykowski-Górecki, Boris Ostrovsky Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > > On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> wrote: > > > > > > > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > > > Convert to use vm_map_pages() to map range of kernel > > > > > memory to user vma. > > > > > > > > > > map->count is passed to vm_map_pages() and internal API > > > > > verify map->count against count ( count = vma_pages(vma)) > > > > > for page array boundary overrun condition. > > > > > > > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > > > will: > > > > - use map->pages starting at vma->vm_pgoff instead of 0 > > > > > > The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > > the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > > if vma->vm_pgoff > 0 (in original code) ? > > vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > basically (ab)using this parameter for "which grant reference to map". > > > > are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > > from user space ? If yes, using vm_map_pages_zero() is an alternate > > > option. > > Yes, that should work. I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively the patch can be reverted as you suggested. Let me know you opinion and wait for feedback from others. Boris, would you like to give any feedback ? > > > > > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > > > > vma_pages(). > > > > > > In original code -> > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > index 559d4b7f807d..469dfbd6cf90 100644 > > > --- a/drivers/xen/gntdev.c > > > +++ b/drivers/xen/gntdev.c > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct > > > vm_area_struct *vma) > > > int index = vma->vm_pgoff; > > > int count = vma_pages(vma); > > > > > > Count is user passed value. > > > > > > struct gntdev_grant_map *map; > > > - int i, err = -EINVAL; > > > + int err = -EINVAL; > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > return -EINVAL; > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, > > > struct vm_area_struct *vma) > > > goto out_put_map; > > > if (!use_ptemod) { > > > - for (i = 0; i < count; i++) { > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > - map->pages[i]); > > > > > > and when count > i , we end up with trying to map memory outside > > > boundary of map->pages[i], which was not correct. > > > > typo. > > s/count > i / count > map->count > > gntdev_find_map_index verifies it. Specifically, it looks for a map matching > both index and count. > > > > > > > - if (err) > > > - goto out_put_map; > > > - } > > > + err = vm_map_pages(vma, map->pages, map->count); > > > + if (err) > > > + goto out_put_map; > > > > > > With this commit, inside __vm_map_pages(), we have addressed this scenario. > > > > > > +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, > > > + unsigned long num, unsigned long offset) > > > +{ > > > + unsigned long count = vma_pages(vma); > > > + unsigned long uaddr = vma->vm_start; > > > + int ret, i; > > > + > > > + /* Fail if the user requested offset is beyond the end of the object */ > > > + if (offset > num) > > > + return -ENXIO; > > > + > > > + /* Fail if the user requested size exceeds available object size */ > > > + if (count > num - offset) > > > + return -ENXIO; > > > > > > By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). > > > So we will never cross the boundary of map->pages[i]. > > > > > > > > > > > > > > In practice, this breaks using a single gntdev FD for mapping multiple > > > > grants. > > > > > > How ? > > gntdev uses vma->vm_pgoff to select which grant entry should be mapped. > map struct returned by gntdev_find_map_index() describes just the pages > to be mapped. Specifically map->pages[0] should be mapped at > vma->vm_start, not vma->vm_start+vma->vm_pgoff*PAGE_SIZE. > > When trying to map grant with index (aka vma->vm_pgoff) > 1, > __vm_map_pages() will refuse to map it because it will expect map->count > to be at least vma_pages(vma)+vma->vm_pgoff, while it is exactly > vma_pages(vma). > > > > > It looks like vm_map_pages() is not a good fit for this code and IMO it > > > > should be reverted. > > > > > > Did you hit any issue around this code in real time ? > > Yes, relevant strace output: > [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 > [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0) = 0x777f1211b000 > [pid 857] ioctl(7, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, 0x7ffd3407b710) = 0 > [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 > [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0x1000) = -1 ENXIO (No such device or address) > > details here: > https://github.com/QubesOS/qubes-issues/issues/5199 > > > > > > > > > > > > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > > --- > > > > > drivers/xen/gntdev.c | 11 ++++------- > > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > > > index 5efc5ee..5d64262 100644 > > > > > --- a/drivers/xen/gntdev.c > > > > > +++ b/drivers/xen/gntdev.c > > > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > > int index = vma->vm_pgoff; > > > > > int count = vma_pages(vma); > > > > > struct gntdev_grant_map *map; > > > > > - int i, err = -EINVAL; > > > > > + int err = -EINVAL; > > > > > > > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > > > return -EINVAL; > > > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > > goto out_put_map; > > > > > > > > > > if (!use_ptemod) { > > > > > - for (i = 0; i < count; i++) { > > > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > > > - map->pages[i]); > > > > > - if (err) > > > > > - goto out_put_map; > > > > > - } > > > > > + err = vm_map_pages(vma, map->pages, map->count); > > > > > + if (err) > > > > > + goto out_put_map; > > > > > } else { > > > > > #ifdef CONFIG_X86 > > > > > /* > > > > > > > > -- > > > > Best Regards, > > > > Marek Marczykowski-Górecki > > > > Invisible Things Lab > > > > A: Because it messes up the order in which people normally read text. > > > > Q: Why is top-posting such a bad thing? > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-30 6:03 ` Souptick Joarder 0 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-30 6:03 UTC (permalink / raw) To: Marek Marczykowski-Górecki, Boris Ostrovsky Cc: Juergen Gross, Michal Hocko, Russell King - ARM Linux, Matthew Wilcox, linux-kernel, Linux-MM, xen-devel, Andrew Morton, robin.murphy On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > > On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> wrote: > > > > > > > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > > > Convert to use vm_map_pages() to map range of kernel > > > > > memory to user vma. > > > > > > > > > > map->count is passed to vm_map_pages() and internal API > > > > > verify map->count against count ( count = vma_pages(vma)) > > > > > for page array boundary overrun condition. > > > > > > > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > > > will: > > > > - use map->pages starting at vma->vm_pgoff instead of 0 > > > > > > The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > > the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > > if vma->vm_pgoff > 0 (in original code) ? > > vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > basically (ab)using this parameter for "which grant reference to map". > > > > are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > > from user space ? If yes, using vm_map_pages_zero() is an alternate > > > option. > > Yes, that should work. I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively the patch can be reverted as you suggested. Let me know you opinion and wait for feedback from others. Boris, would you like to give any feedback ? > > > > > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > > > > vma_pages(). > > > > > > In original code -> > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > index 559d4b7f807d..469dfbd6cf90 100644 > > > --- a/drivers/xen/gntdev.c > > > +++ b/drivers/xen/gntdev.c > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct > > > vm_area_struct *vma) > > > int index = vma->vm_pgoff; > > > int count = vma_pages(vma); > > > > > > Count is user passed value. > > > > > > struct gntdev_grant_map *map; > > > - int i, err = -EINVAL; > > > + int err = -EINVAL; > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > return -EINVAL; > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, > > > struct vm_area_struct *vma) > > > goto out_put_map; > > > if (!use_ptemod) { > > > - for (i = 0; i < count; i++) { > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > - map->pages[i]); > > > > > > and when count > i , we end up with trying to map memory outside > > > boundary of map->pages[i], which was not correct. > > > > typo. > > s/count > i / count > map->count > > gntdev_find_map_index verifies it. Specifically, it looks for a map matching > both index and count. > > > > > > > - if (err) > > > - goto out_put_map; > > > - } > > > + err = vm_map_pages(vma, map->pages, map->count); > > > + if (err) > > > + goto out_put_map; > > > > > > With this commit, inside __vm_map_pages(), we have addressed this scenario. > > > > > > +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, > > > + unsigned long num, unsigned long offset) > > > +{ > > > + unsigned long count = vma_pages(vma); > > > + unsigned long uaddr = vma->vm_start; > > > + int ret, i; > > > + > > > + /* Fail if the user requested offset is beyond the end of the object */ > > > + if (offset > num) > > > + return -ENXIO; > > > + > > > + /* Fail if the user requested size exceeds available object size */ > > > + if (count > num - offset) > > > + return -ENXIO; > > > > > > By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). > > > So we will never cross the boundary of map->pages[i]. > > > > > > > > > > > > > > In practice, this breaks using a single gntdev FD for mapping multiple > > > > grants. > > > > > > How ? > > gntdev uses vma->vm_pgoff to select which grant entry should be mapped. > map struct returned by gntdev_find_map_index() describes just the pages > to be mapped. Specifically map->pages[0] should be mapped at > vma->vm_start, not vma->vm_start+vma->vm_pgoff*PAGE_SIZE. > > When trying to map grant with index (aka vma->vm_pgoff) > 1, > __vm_map_pages() will refuse to map it because it will expect map->count > to be at least vma_pages(vma)+vma->vm_pgoff, while it is exactly > vma_pages(vma). > > > > > It looks like vm_map_pages() is not a good fit for this code and IMO it > > > > should be reverted. > > > > > > Did you hit any issue around this code in real time ? > > Yes, relevant strace output: > [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 > [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0) = 0x777f1211b000 > [pid 857] ioctl(7, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, 0x7ffd3407b710) = 0 > [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 > [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0x1000) = -1 ENXIO (No such device or address) > > details here: > https://github.com/QubesOS/qubes-issues/issues/5199 > > > > > > > > > > > > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > > --- > > > > > drivers/xen/gntdev.c | 11 ++++------- > > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > > > index 5efc5ee..5d64262 100644 > > > > > --- a/drivers/xen/gntdev.c > > > > > +++ b/drivers/xen/gntdev.c > > > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > > int index = vma->vm_pgoff; > > > > > int count = vma_pages(vma); > > > > > struct gntdev_grant_map *map; > > > > > - int i, err = -EINVAL; > > > > > + int err = -EINVAL; > > > > > > > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > > > return -EINVAL; > > > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > > goto out_put_map; > > > > > > > > > > if (!use_ptemod) { > > > > > - for (i = 0; i < count; i++) { > > > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > > > - map->pages[i]); > > > > > - if (err) > > > > > - goto out_put_map; > > > > > - } > > > > > + err = vm_map_pages(vma, map->pages, map->count); > > > > > + if (err) > > > > > + goto out_put_map; > > > > > } else { > > > > > #ifdef CONFIG_X86 > > > > > /* > > > > > > > > -- > > > > Best Regards, > > > > Marek Marczykowski-Górecki > > > > Invisible Things Lab > > > > A: Because it messes up the order in which people normally read text. > > > > Q: Why is top-posting such a bad thing? > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-30 6:03 ` Souptick Joarder 0 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-30 6:03 UTC (permalink / raw) To: Marek Marczykowski-Górecki, Boris Ostrovsky Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > > On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > > > > On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> wrote: > > > > > > > > On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > > > Convert to use vm_map_pages() to map range of kernel > > > > > memory to user vma. > > > > > > > > > > map->count is passed to vm_map_pages() and internal API > > > > > verify map->count against count ( count = vma_pages(vma)) > > > > > for page array boundary overrun condition. > > > > > > > > This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > > > will: > > > > - use map->pages starting at vma->vm_pgoff instead of 0 > > > > > > The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > > the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > > if vma->vm_pgoff > 0 (in original code) ? > > vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > basically (ab)using this parameter for "which grant reference to map". > > > > are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > > from user space ? If yes, using vm_map_pages_zero() is an alternate > > > option. > > Yes, that should work. I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively the patch can be reverted as you suggested. Let me know you opinion and wait for feedback from others. Boris, would you like to give any feedback ? > > > > > - verify map->count against vma_pages()+vma->vm_pgoff instead of just > > > > vma_pages(). > > > > > > In original code -> > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > index 559d4b7f807d..469dfbd6cf90 100644 > > > --- a/drivers/xen/gntdev.c > > > +++ b/drivers/xen/gntdev.c > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct > > > vm_area_struct *vma) > > > int index = vma->vm_pgoff; > > > int count = vma_pages(vma); > > > > > > Count is user passed value. > > > > > > struct gntdev_grant_map *map; > > > - int i, err = -EINVAL; > > > + int err = -EINVAL; > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > return -EINVAL; > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, > > > struct vm_area_struct *vma) > > > goto out_put_map; > > > if (!use_ptemod) { > > > - for (i = 0; i < count; i++) { > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > - map->pages[i]); > > > > > > and when count > i , we end up with trying to map memory outside > > > boundary of map->pages[i], which was not correct. > > > > typo. > > s/count > i / count > map->count > > gntdev_find_map_index verifies it. Specifically, it looks for a map matching > both index and count. > > > > > > > - if (err) > > > - goto out_put_map; > > > - } > > > + err = vm_map_pages(vma, map->pages, map->count); > > > + if (err) > > > + goto out_put_map; > > > > > > With this commit, inside __vm_map_pages(), we have addressed this scenario. > > > > > > +static int __vm_map_pages(struct vm_area_struct *vma, struct page **pages, > > > + unsigned long num, unsigned long offset) > > > +{ > > > + unsigned long count = vma_pages(vma); > > > + unsigned long uaddr = vma->vm_start; > > > + int ret, i; > > > + > > > + /* Fail if the user requested offset is beyond the end of the object */ > > > + if (offset > num) > > > + return -ENXIO; > > > + > > > + /* Fail if the user requested size exceeds available object size */ > > > + if (count > num - offset) > > > + return -ENXIO; > > > > > > By checking count > num -offset. (considering vma->vm_pgoff != 0 as well). > > > So we will never cross the boundary of map->pages[i]. > > > > > > > > > > > > > > In practice, this breaks using a single gntdev FD for mapping multiple > > > > grants. > > > > > > How ? > > gntdev uses vma->vm_pgoff to select which grant entry should be mapped. > map struct returned by gntdev_find_map_index() describes just the pages > to be mapped. Specifically map->pages[0] should be mapped at > vma->vm_start, not vma->vm_start+vma->vm_pgoff*PAGE_SIZE. > > When trying to map grant with index (aka vma->vm_pgoff) > 1, > __vm_map_pages() will refuse to map it because it will expect map->count > to be at least vma_pages(vma)+vma->vm_pgoff, while it is exactly > vma_pages(vma). > > > > > It looks like vm_map_pages() is not a good fit for this code and IMO it > > > > should be reverted. > > > > > > Did you hit any issue around this code in real time ? > > Yes, relevant strace output: > [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 > [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0) = 0x777f1211b000 > [pid 857] ioctl(7, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, 0x7ffd3407b710) = 0 > [pid 857] ioctl(7, IOCTL_GNTDEV_MAP_GRANT_REF, 0x7ffd3407b6d0) = 0 > [pid 857] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 7, 0x1000) = -1 ENXIO (No such device or address) > > details here: > https://github.com/QubesOS/qubes-issues/issues/5199 > > > > > > > > > > > > > > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > > --- > > > > > drivers/xen/gntdev.c | 11 ++++------- > > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > > > > > index 5efc5ee..5d64262 100644 > > > > > --- a/drivers/xen/gntdev.c > > > > > +++ b/drivers/xen/gntdev.c > > > > > @@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > > int index = vma->vm_pgoff; > > > > > int count = vma_pages(vma); > > > > > struct gntdev_grant_map *map; > > > > > - int i, err = -EINVAL; > > > > > + int err = -EINVAL; > > > > > > > > > > if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED)) > > > > > return -EINVAL; > > > > > @@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma) > > > > > goto out_put_map; > > > > > > > > > > if (!use_ptemod) { > > > > > - for (i = 0; i < count; i++) { > > > > > - err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE, > > > > > - map->pages[i]); > > > > > - if (err) > > > > > - goto out_put_map; > > > > > - } > > > > > + err = vm_map_pages(vma, map->pages, map->count); > > > > > + if (err) > > > > > + goto out_put_map; > > > > > } else { > > > > > #ifdef CONFIG_X86 > > > > > /* > > > > > > > > -- > > > > Best Regards, > > > > Marek Marczykowski-Górecki > > > > Invisible Things Lab > > > > A: Because it messes up the order in which people normally read text. > > > > Q: Why is top-posting such a bad thing? > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() 2019-07-30 6:03 ` Souptick Joarder @ 2019-07-30 14:05 ` Boris Ostrovsky -1 siblings, 0 replies; 23+ messages in thread From: Boris Ostrovsky @ 2019-07-30 14:05 UTC (permalink / raw) To: Souptick Joarder, Marek Marczykowski-Górecki Cc: Andrew Morton, Matthew Wilcox, Michal Hocko, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM On 7/30/19 2:03 AM, Souptick Joarder wrote: > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: >> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: >>> On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki >>>> <marmarek@invisiblethingslab.com> wrote: >>>>> On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: >>>>>> Convert to use vm_map_pages() to map range of kernel >>>>>> memory to user vma. >>>>>> >>>>>> map->count is passed to vm_map_pages() and internal API >>>>>> verify map->count against count ( count = vma_pages(vma)) >>>>>> for page array boundary overrun condition. >>>>> This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages >>>>> will: >>>>> - use map->pages starting at vma->vm_pgoff instead of 0 >>>> The actual code ignores vma->vm_pgoff > 0 scenario and mapped >>>> the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped >>>> if vma->vm_pgoff > 0 (in original code) ? >> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's >> basically (ab)using this parameter for "which grant reference to map". >> >>>> are you referring to set vma->vm_pgoff = 0 irrespective of value passed >>>> from user space ? If yes, using vm_map_pages_zero() is an alternate >>>> option. >> Yes, that should work. > I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively > the patch can be reverted as you suggested. Let me know you opinion and wait > for feedback from others. > > Boris, would you like to give any feedback ? vm_map_pages_zero() looks good to me. Marek, does it work for you? -boris ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-30 14:05 ` Boris Ostrovsky 0 siblings, 0 replies; 23+ messages in thread From: Boris Ostrovsky @ 2019-07-30 14:05 UTC (permalink / raw) To: Souptick Joarder, Marek Marczykowski-Górecki Cc: Juergen Gross, Michal Hocko, Russell King - ARM Linux, Matthew Wilcox, linux-kernel, Linux-MM, xen-devel, Andrew Morton, robin.murphy On 7/30/19 2:03 AM, Souptick Joarder wrote: > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: >> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: >>> On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki >>>> <marmarek@invisiblethingslab.com> wrote: >>>>> On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: >>>>>> Convert to use vm_map_pages() to map range of kernel >>>>>> memory to user vma. >>>>>> >>>>>> map->count is passed to vm_map_pages() and internal API >>>>>> verify map->count against count ( count = vma_pages(vma)) >>>>>> for page array boundary overrun condition. >>>>> This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages >>>>> will: >>>>> - use map->pages starting at vma->vm_pgoff instead of 0 >>>> The actual code ignores vma->vm_pgoff > 0 scenario and mapped >>>> the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped >>>> if vma->vm_pgoff > 0 (in original code) ? >> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's >> basically (ab)using this parameter for "which grant reference to map". >> >>>> are you referring to set vma->vm_pgoff = 0 irrespective of value passed >>>> from user space ? If yes, using vm_map_pages_zero() is an alternate >>>> option. >> Yes, that should work. > I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively > the patch can be reverted as you suggested. Let me know you opinion and wait > for feedback from others. > > Boris, would you like to give any feedback ? vm_map_pages_zero() looks good to me. Marek, does it work for you? -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() 2019-07-30 14:05 ` Boris Ostrovsky @ 2019-07-30 14:22 ` Marek Marczykowski-Górecki -1 siblings, 0 replies; 23+ messages in thread From: Marek Marczykowski-Górecki @ 2019-07-30 14:22 UTC (permalink / raw) To: Boris Ostrovsky Cc: Souptick Joarder, Andrew Morton, Matthew Wilcox, Michal Hocko, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM [-- Attachment #1: Type: text/plain, Size: 2180 bytes --] On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> wrote: > >> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > >>> On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > >>>> <marmarek@invisiblethingslab.com> wrote: > >>>>> On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > >>>>>> Convert to use vm_map_pages() to map range of kernel > >>>>>> memory to user vma. > >>>>>> > >>>>>> map->count is passed to vm_map_pages() and internal API > >>>>>> verify map->count against count ( count = vma_pages(vma)) > >>>>>> for page array boundary overrun condition. > >>>>> This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > >>>>> will: > >>>>> - use map->pages starting at vma->vm_pgoff instead of 0 > >>>> The actual code ignores vma->vm_pgoff > 0 scenario and mapped > >>>> the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > >>>> if vma->vm_pgoff > 0 (in original code) ? > >> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > >> basically (ab)using this parameter for "which grant reference to map". > >> > >>>> are you referring to set vma->vm_pgoff = 0 irrespective of value passed > >>>> from user space ? If yes, using vm_map_pages_zero() is an alternate > >>>> option. > >> Yes, that should work. > > I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively > > the patch can be reverted as you suggested. Let me know you opinion and wait > > for feedback from others. > > > > Boris, would you like to give any feedback ? > > vm_map_pages_zero() looks good to me. Marek, does it work for you? Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the problem for me. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-30 14:22 ` Marek Marczykowski-Górecki 0 siblings, 0 replies; 23+ messages in thread From: Marek Marczykowski-Górecki @ 2019-07-30 14:22 UTC (permalink / raw) To: Boris Ostrovsky Cc: Juergen Gross, Michal Hocko, Russell King - ARM Linux, Matthew Wilcox, linux-kernel, Linux-MM, Souptick Joarder, xen-devel, Andrew Morton, robin.murphy [-- Attachment #1.1: Type: text/plain, Size: 2180 bytes --] On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> wrote: > >> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > >>> On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > >>>> <marmarek@invisiblethingslab.com> wrote: > >>>>> On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > >>>>>> Convert to use vm_map_pages() to map range of kernel > >>>>>> memory to user vma. > >>>>>> > >>>>>> map->count is passed to vm_map_pages() and internal API > >>>>>> verify map->count against count ( count = vma_pages(vma)) > >>>>>> for page array boundary overrun condition. > >>>>> This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > >>>>> will: > >>>>> - use map->pages starting at vma->vm_pgoff instead of 0 > >>>> The actual code ignores vma->vm_pgoff > 0 scenario and mapped > >>>> the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > >>>> if vma->vm_pgoff > 0 (in original code) ? > >> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > >> basically (ab)using this parameter for "which grant reference to map". > >> > >>>> are you referring to set vma->vm_pgoff = 0 irrespective of value passed > >>>> from user space ? If yes, using vm_map_pages_zero() is an alternate > >>>> option. > >> Yes, that should work. > > I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively > > the patch can be reverted as you suggested. Let me know you opinion and wait > > for feedback from others. > > > > Boris, would you like to give any feedback ? > > vm_map_pages_zero() looks good to me. Marek, does it work for you? Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the problem for me. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() 2019-07-30 14:22 ` Marek Marczykowski-Górecki (?) @ 2019-07-30 14:52 ` Souptick Joarder -1 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-30 14:52 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Boris Ostrovsky, Andrew Morton, Matthew Wilcox, Michal Hocko, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM, stable, Greg KH On Tue, Jul 30, 2019 at 7:52 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> wrote: > > >> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > > >>> On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > >>>> <marmarek@invisiblethingslab.com> wrote: > > >>>>> On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > >>>>>> Convert to use vm_map_pages() to map range of kernel > > >>>>>> memory to user vma. > > >>>>>> > > >>>>>> map->count is passed to vm_map_pages() and internal API > > >>>>>> verify map->count against count ( count = vma_pages(vma)) > > >>>>>> for page array boundary overrun condition. > > >>>>> This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > >>>>> will: > > >>>>> - use map->pages starting at vma->vm_pgoff instead of 0 > > >>>> The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > >>>> the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > >>>> if vma->vm_pgoff > 0 (in original code) ? > > >> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > > >> basically (ab)using this parameter for "which grant reference to map". > > >> > > >>>> are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > >>>> from user space ? If yes, using vm_map_pages_zero() is an alternate > > >>>> option. > > >> Yes, that should work. > > > I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively > > > the patch can be reverted as you suggested. Let me know you opinion and wait > > > for feedback from others. > > > > > > Boris, would you like to give any feedback ? > > > > vm_map_pages_zero() looks good to me. Marek, does it work for you? > > Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the > problem for me. Marek, I can send a patch for the same if you are ok. We need to cc stable as this changes are available in 5.2.4. > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-30 14:52 ` Souptick Joarder 0 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-30 14:52 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Juergen Gross, Michal Hocko, Greg KH, Russell King - ARM Linux, Matthew Wilcox, linux-kernel, Linux-MM, stable, xen-devel, Boris Ostrovsky, robin.murphy, Andrew Morton On Tue, Jul 30, 2019 at 7:52 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> wrote: > > >> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > > >>> On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > >>>> <marmarek@invisiblethingslab.com> wrote: > > >>>>> On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > >>>>>> Convert to use vm_map_pages() to map range of kernel > > >>>>>> memory to user vma. > > >>>>>> > > >>>>>> map->count is passed to vm_map_pages() and internal API > > >>>>>> verify map->count against count ( count = vma_pages(vma)) > > >>>>>> for page array boundary overrun condition. > > >>>>> This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > >>>>> will: > > >>>>> - use map->pages starting at vma->vm_pgoff instead of 0 > > >>>> The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > >>>> the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > >>>> if vma->vm_pgoff > 0 (in original code) ? > > >> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > > >> basically (ab)using this parameter for "which grant reference to map". > > >> > > >>>> are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > >>>> from user space ? If yes, using vm_map_pages_zero() is an alternate > > >>>> option. > > >> Yes, that should work. > > > I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively > > > the patch can be reverted as you suggested. Let me know you opinion and wait > > > for feedback from others. > > > > > > Boris, would you like to give any feedback ? > > > > vm_map_pages_zero() looks good to me. Marek, does it work for you? > > Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the > problem for me. Marek, I can send a patch for the same if you are ok. We need to cc stable as this changes are available in 5.2.4. > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-30 14:52 ` Souptick Joarder 0 siblings, 0 replies; 23+ messages in thread From: Souptick Joarder @ 2019-07-30 14:52 UTC (permalink / raw) To: Marek Marczykowski-Górecki Cc: Boris Ostrovsky, Andrew Morton, Matthew Wilcox, Michal Hocko, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM, stable, Greg KH On Tue, Jul 30, 2019 at 7:52 PM Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote: > > On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> wrote: > > >> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > > >>> On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > >>>> <marmarek@invisiblethingslab.com> wrote: > > >>>>> On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > >>>>>> Convert to use vm_map_pages() to map range of kernel > > >>>>>> memory to user vma. > > >>>>>> > > >>>>>> map->count is passed to vm_map_pages() and internal API > > >>>>>> verify map->count against count ( count = vma_pages(vma)) > > >>>>>> for page array boundary overrun condition. > > >>>>> This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > >>>>> will: > > >>>>> - use map->pages starting at vma->vm_pgoff instead of 0 > > >>>> The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > >>>> the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > >>>> if vma->vm_pgoff > 0 (in original code) ? > > >> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > > >> basically (ab)using this parameter for "which grant reference to map". > > >> > > >>>> are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > >>>> from user space ? If yes, using vm_map_pages_zero() is an alternate > > >>>> option. > > >> Yes, that should work. > > > I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively > > > the patch can be reverted as you suggested. Let me know you opinion and wait > > > for feedback from others. > > > > > > Boris, would you like to give any feedback ? > > > > vm_map_pages_zero() looks good to me. Marek, does it work for you? > > Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the > problem for me. Marek, I can send a patch for the same if you are ok. We need to cc stable as this changes are available in 5.2.4. > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() 2019-07-30 14:52 ` Souptick Joarder @ 2019-07-30 15:01 ` Marek Marczykowski-Górecki -1 siblings, 0 replies; 23+ messages in thread From: Marek Marczykowski-Górecki @ 2019-07-30 15:01 UTC (permalink / raw) To: Souptick Joarder Cc: Boris Ostrovsky, Andrew Morton, Matthew Wilcox, Michal Hocko, Juergen Gross, Russell King - ARM Linux, robin.murphy, xen-devel, linux-kernel, Linux-MM, stable, Greg KH [-- Attachment #1: Type: text/plain, Size: 2658 bytes --] On Tue, Jul 30, 2019 at 08:22:02PM +0530, Souptick Joarder wrote: > On Tue, Jul 30, 2019 at 7:52 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: > > > > On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > > > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > > > <marmarek@invisiblethingslab.com> wrote: > > > >> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > > > >>> On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > > >>>> <marmarek@invisiblethingslab.com> wrote: > > > >>>>> On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > >>>>>> Convert to use vm_map_pages() to map range of kernel > > > >>>>>> memory to user vma. > > > >>>>>> > > > >>>>>> map->count is passed to vm_map_pages() and internal API > > > >>>>>> verify map->count against count ( count = vma_pages(vma)) > > > >>>>>> for page array boundary overrun condition. > > > >>>>> This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > > >>>>> will: > > > >>>>> - use map->pages starting at vma->vm_pgoff instead of 0 > > > >>>> The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > > >>>> the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > > >>>> if vma->vm_pgoff > 0 (in original code) ? > > > >> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > > > >> basically (ab)using this parameter for "which grant reference to map". > > > >> > > > >>>> are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > > >>>> from user space ? If yes, using vm_map_pages_zero() is an alternate > > > >>>> option. > > > >> Yes, that should work. > > > > I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively > > > > the patch can be reverted as you suggested. Let me know you opinion and wait > > > > for feedback from others. > > > > > > > > Boris, would you like to give any feedback ? > > > > > > vm_map_pages_zero() looks good to me. Marek, does it work for you? > > > > Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the > > problem for me. > > Marek, I can send a patch for the same if you are ok. > We need to cc stable as this changes are available in 5.2.4. Sounds good, thanks! -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() @ 2019-07-30 15:01 ` Marek Marczykowski-Górecki 0 siblings, 0 replies; 23+ messages in thread From: Marek Marczykowski-Górecki @ 2019-07-30 15:01 UTC (permalink / raw) To: Souptick Joarder Cc: Juergen Gross, Michal Hocko, Greg KH, Russell King - ARM Linux, Matthew Wilcox, linux-kernel, Linux-MM, stable, xen-devel, Boris Ostrovsky, robin.murphy, Andrew Morton [-- Attachment #1.1: Type: text/plain, Size: 2658 bytes --] On Tue, Jul 30, 2019 at 08:22:02PM +0530, Souptick Joarder wrote: > On Tue, Jul 30, 2019 at 7:52 PM Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> wrote: > > > > On Tue, Jul 30, 2019 at 10:05:42AM -0400, Boris Ostrovsky wrote: > > > On 7/30/19 2:03 AM, Souptick Joarder wrote: > > > > On Mon, Jul 29, 2019 at 7:06 PM Marek Marczykowski-Górecki > > > > <marmarek@invisiblethingslab.com> wrote: > > > >> On Mon, Jul 29, 2019 at 02:02:54PM +0530, Souptick Joarder wrote: > > > >>> On Mon, Jul 29, 2019 at 1:35 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > > >>>> On Sun, Jul 28, 2019 at 11:36 PM Marek Marczykowski-Górecki > > > >>>> <marmarek@invisiblethingslab.com> wrote: > > > >>>>> On Fri, Feb 15, 2019 at 08:18:31AM +0530, Souptick Joarder wrote: > > > >>>>>> Convert to use vm_map_pages() to map range of kernel > > > >>>>>> memory to user vma. > > > >>>>>> > > > >>>>>> map->count is passed to vm_map_pages() and internal API > > > >>>>>> verify map->count against count ( count = vma_pages(vma)) > > > >>>>>> for page array boundary overrun condition. > > > >>>>> This commit breaks gntdev driver. If vma->vm_pgoff > 0, vm_map_pages > > > >>>>> will: > > > >>>>> - use map->pages starting at vma->vm_pgoff instead of 0 > > > >>>> The actual code ignores vma->vm_pgoff > 0 scenario and mapped > > > >>>> the entire map->pages[i]. Why the entire map->pages[i] needs to be mapped > > > >>>> if vma->vm_pgoff > 0 (in original code) ? > > > >> vma->vm_pgoff is used as index passed to gntdev_find_map_index. It's > > > >> basically (ab)using this parameter for "which grant reference to map". > > > >> > > > >>>> are you referring to set vma->vm_pgoff = 0 irrespective of value passed > > > >>>> from user space ? If yes, using vm_map_pages_zero() is an alternate > > > >>>> option. > > > >> Yes, that should work. > > > > I prefer to use vm_map_pages_zero() to resolve both the issues. Alternatively > > > > the patch can be reverted as you suggested. Let me know you opinion and wait > > > > for feedback from others. > > > > > > > > Boris, would you like to give any feedback ? > > > > > > vm_map_pages_zero() looks good to me. Marek, does it work for you? > > > > Yes, replacing vm_map_pages() with vm_map_pages_zero() fixes the > > problem for me. > > Marek, I can send a patch for the same if you are ok. > We need to cc stable as this changes are available in 5.2.4. Sounds good, thanks! -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-07-30 15:01 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-15 2:48 [PATCH v4 8/9] xen/gntdev.c: Convert to use vm_map_pages() Souptick Joarder 2019-07-28 18:06 ` [Xen-devel] " Marek Marczykowski-Górecki 2019-07-28 18:06 ` Marek Marczykowski-Górecki 2019-07-29 8:05 ` Souptick Joarder 2019-07-29 8:05 ` Souptick Joarder 2019-07-29 8:05 ` Souptick Joarder 2019-07-29 8:32 ` Souptick Joarder 2019-07-29 8:32 ` Souptick Joarder 2019-07-29 8:32 ` Souptick Joarder 2019-07-29 13:36 ` Marek Marczykowski-Górecki 2019-07-29 13:36 ` Marek Marczykowski-Górecki 2019-07-30 6:03 ` Souptick Joarder 2019-07-30 6:03 ` Souptick Joarder 2019-07-30 6:03 ` Souptick Joarder 2019-07-30 14:05 ` Boris Ostrovsky 2019-07-30 14:05 ` Boris Ostrovsky 2019-07-30 14:22 ` Marek Marczykowski-Górecki 2019-07-30 14:22 ` Marek Marczykowski-Górecki 2019-07-30 14:52 ` Souptick Joarder 2019-07-30 14:52 ` Souptick Joarder 2019-07-30 14:52 ` Souptick Joarder 2019-07-30 15:01 ` Marek Marczykowski-Górecki 2019-07-30 15:01 ` Marek Marczykowski-Górecki
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.