| Message ID | 1566713247-23873-1-git-send-email-jrdr.linux@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | IB/mlx5: Convert to use vm_map_pages_zero() | expand |
On Sun, Aug 25, 2019 at 11:37:27AM +0530, Souptick Joarder wrote: > First, length passed to mmap is checked explicitly against > PAGE_SIZE. > > Second, if vma->vm_pgoff is passed as non zero, it would return > error. It appears like driver is expecting vma->vm_pgoff to > be passed as 0 always. ? pg_off is not zero Jason
On Mon, Aug 26, 2019 at 1:13 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Sun, Aug 25, 2019 at 11:37:27AM +0530, Souptick Joarder wrote: > > First, length passed to mmap is checked explicitly against > > PAGE_SIZE. > > > > Second, if vma->vm_pgoff is passed as non zero, it would return > > error. It appears like driver is expecting vma->vm_pgoff to > > be passed as 0 always. > > ? pg_off is not zero Sorry, I mean, driver has a check against non zero to return error -EOPNOTSUPP which means in true scenario driver is expecting vma->vm_pgoff should be passed as 0. - if (get_index(vma->vm_pgoff) != MLX5_IB_CLOCK_INFO_V1) - return -EOPNOTSUPP;
On Mon, Aug 26, 2019 at 01:32:09AM +0530, Souptick Joarder wrote: > On Mon, Aug 26, 2019 at 1:13 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Sun, Aug 25, 2019 at 11:37:27AM +0530, Souptick Joarder wrote: > > > First, length passed to mmap is checked explicitly against > > > PAGE_SIZE. > > > > > > Second, if vma->vm_pgoff is passed as non zero, it would return > > > error. It appears like driver is expecting vma->vm_pgoff to > > > be passed as 0 always. > > > > ? pg_off is not zero > > Sorry, I mean, driver has a check against non zero to return error -EOPNOTSUPP > which means in true scenario driver is expecting vma->vm_pgoff should be passed > as 0. get_index is masking vm_pgoff, it is not 0 Jason
On Mon, Aug 26, 2019 at 5:50 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Mon, Aug 26, 2019 at 01:32:09AM +0530, Souptick Joarder wrote: > > On Mon, Aug 26, 2019 at 1:13 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > On Sun, Aug 25, 2019 at 11:37:27AM +0530, Souptick Joarder wrote: > > > > First, length passed to mmap is checked explicitly against > > > > PAGE_SIZE. > > > > > > > > Second, if vma->vm_pgoff is passed as non zero, it would return > > > > error. It appears like driver is expecting vma->vm_pgoff to > > > > be passed as 0 always. > > > > > > ? pg_off is not zero > > > > Sorry, I mean, driver has a check against non zero to return error -EOPNOTSUPP > > which means in true scenario driver is expecting vma->vm_pgoff should be passed > > as 0. > > get_index is masking vm_pgoff, it is not 0 Sorry, I missed this part. Further looking into code, in mlx5_ib_mmap(), vma_vm_pgoff is used to get command and inside mlx5_ib_mmap_clock_info_page() entire *dev->mdev->clock_info* is mapped. Consider that, the below modification will only take care of vma length error check inside vm_map_pages_zero() and an extra check for vma length is not needed. diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index 0569bca..c3e3bfe 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -2071,8 +2071,9 @@ static int mlx5_ib_mmap_clock_info_page(struct mlx5_ib_dev *dev, struct vm_area_struct *vma, struct mlx5_ib_ucontext *context) { - if ((vma->vm_end - vma->vm_start != PAGE_SIZE) || - !(vma->vm_flags & VM_SHARED)) + struct page *pages; + + if (!(vma->vm_flags & VM_SHARED)) return -EINVAL; if (get_index(vma->vm_pgoff) != MLX5_IB_CLOCK_INFO_V1) @@ -2084,9 +2085,9 @@ static int mlx5_ib_mmap_clock_info_page(struct mlx5_ib_dev *dev, if (!dev->mdev->clock_info) return -EOPNOTSUPP; + pages = virt_to_page(dev->mdev->clock_info); - return vm_insert_page(vma, vma->vm_start, - virt_to_page(dev->mdev->clock_info)); + return vm_map_pages_zero(vma, &pages, 1); } If this is fine, I can post it as v2. Otherwise I will drop this patch ?
On Tue, Aug 27, 2019 at 01:48:57AM +0530, Souptick Joarder wrote: > On Mon, Aug 26, 2019 at 5:50 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Mon, Aug 26, 2019 at 01:32:09AM +0530, Souptick Joarder wrote: > > > On Mon, Aug 26, 2019 at 1:13 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > > > On Sun, Aug 25, 2019 at 11:37:27AM +0530, Souptick Joarder wrote: > > > > > First, length passed to mmap is checked explicitly against > > > > > PAGE_SIZE. > > > > > > > > > > Second, if vma->vm_pgoff is passed as non zero, it would return > > > > > error. It appears like driver is expecting vma->vm_pgoff to > > > > > be passed as 0 always. > > > > > > > > ? pg_off is not zero > > > > > > Sorry, I mean, driver has a check against non zero to return error -EOPNOTSUPP > > > which means in true scenario driver is expecting vma->vm_pgoff should be passed > > > as 0. > > > > get_index is masking vm_pgoff, it is not 0 > > Sorry, I missed this part. Further looking into code, > in mlx5_ib_mmap(), vma_vm_pgoff is used to get command and > inside mlx5_ib_mmap_clock_info_page() entire *dev->mdev->clock_info* > is mapped. > > Consider that, the below modification will only take care of vma length > error check inside vm_map_pages_zero() and an extra check for vma > length is not needed. What is the point of vm_map_pages_zero() Is there some reason we should prefer it for mapping a single page? Jason
On Tue, Aug 27, 2019 at 9:19 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Tue, Aug 27, 2019 at 01:48:57AM +0530, Souptick Joarder wrote: > > On Mon, Aug 26, 2019 at 5:50 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > On Mon, Aug 26, 2019 at 01:32:09AM +0530, Souptick Joarder wrote: > > > > On Mon, Aug 26, 2019 at 1:13 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > > > > > On Sun, Aug 25, 2019 at 11:37:27AM +0530, Souptick Joarder wrote: > > > > > > First, length passed to mmap is checked explicitly against > > > > > > PAGE_SIZE. > > > > > > > > > > > > Second, if vma->vm_pgoff is passed as non zero, it would return > > > > > > error. It appears like driver is expecting vma->vm_pgoff to > > > > > > be passed as 0 always. > > > > > > > > > > ? pg_off is not zero > > > > > > > > Sorry, I mean, driver has a check against non zero to return error -EOPNOTSUPP > > > > which means in true scenario driver is expecting vma->vm_pgoff should be passed > > > > as 0. > > > > > > get_index is masking vm_pgoff, it is not 0 > > > > Sorry, I missed this part. Further looking into code, > > in mlx5_ib_mmap(), vma_vm_pgoff is used to get command and > > inside mlx5_ib_mmap_clock_info_page() entire *dev->mdev->clock_info* > > is mapped. > > > > Consider that, the below modification will only take care of vma length > > error check inside vm_map_pages_zero() and an extra check for vma > > length is not needed. > > What is the point of vm_map_pages_zero() Is there some reason we should > prefer it for mapping a single page? vm_map_pages_zero() can be used to map single/ multiple pages both. There were drivers previously which either check length and pg_off explicitly or didn't check for incorrect value of length /pg_off passed to it at all. Calling vm_map_pages_zero() in those places were more appropriate as it has internal check for both. Now considering this patch, avoiding an extra check for length explicitly is the only part which can be avoided if converted to use vm_map_pages_zero() because pg_off is used in different context ( to identify command). So yes, improvement wise convert to use vm_map_pages_zero() is not making much difference here.
On Wed, Aug 28, 2019 at 08:57:19AM +0530, Souptick Joarder wrote: > On Tue, Aug 27, 2019 at 9:19 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Tue, Aug 27, 2019 at 01:48:57AM +0530, Souptick Joarder wrote: > > > On Mon, Aug 26, 2019 at 5:50 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > > > On Mon, Aug 26, 2019 at 01:32:09AM +0530, Souptick Joarder wrote: > > > > > On Mon, Aug 26, 2019 at 1:13 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > > > > > > > On Sun, Aug 25, 2019 at 11:37:27AM +0530, Souptick Joarder wrote: > > > > > > > First, length passed to mmap is checked explicitly against > > > > > > > PAGE_SIZE. > > > > > > > > > > > > > > Second, if vma->vm_pgoff is passed as non zero, it would return > > > > > > > error. It appears like driver is expecting vma->vm_pgoff to > > > > > > > be passed as 0 always. > > > > > > > > > > > > ? pg_off is not zero > > > > > > > > > > Sorry, I mean, driver has a check against non zero to return error -EOPNOTSUPP > > > > > which means in true scenario driver is expecting vma->vm_pgoff should be passed > > > > > as 0. > > > > > > > > get_index is masking vm_pgoff, it is not 0 > > > > > > Sorry, I missed this part. Further looking into code, > > > in mlx5_ib_mmap(), vma_vm_pgoff is used to get command and > > > inside mlx5_ib_mmap_clock_info_page() entire *dev->mdev->clock_info* > > > is mapped. > > > > > > Consider that, the below modification will only take care of vma length > > > error check inside vm_map_pages_zero() and an extra check for vma > > > length is not needed. > > > > What is the point of vm_map_pages_zero() Is there some reason we should > > prefer it for mapping a single page? > > vm_map_pages_zero() can be used to map single/ multiple pages both. > There were drivers previously which either check length and pg_off explicitly > or didn't check for incorrect value of length /pg_off passed to it at > all. Calling > vm_map_pages_zero() in those places were more appropriate as it has > internal check for both. > > Now considering this patch, avoiding an extra check for length > explicitly is the only > part which can be avoided if converted to use vm_map_pages_zero() > because pg_off > is used in different context ( to identify command). So yes, > improvement wise convert > to use vm_map_pages_zero() is not making much difference here. So let's drop it, please. Thanks
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index 0569bca..366211d 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -2071,12 +2071,10 @@ static int mlx5_ib_mmap_clock_info_page(struct mlx5_ib_dev *dev, struct vm_area_struct *vma, struct mlx5_ib_ucontext *context) { - if ((vma->vm_end - vma->vm_start != PAGE_SIZE) || - !(vma->vm_flags & VM_SHARED)) - return -EINVAL; + struct page *pages; - if (get_index(vma->vm_pgoff) != MLX5_IB_CLOCK_INFO_V1) - return -EOPNOTSUPP; + if (!(vma->vm_flags & VM_SHARED)) + return -EINVAL; if (vma->vm_flags & (VM_WRITE | VM_EXEC)) return -EPERM; @@ -2084,9 +2082,9 @@ static int mlx5_ib_mmap_clock_info_page(struct mlx5_ib_dev *dev, if (!dev->mdev->clock_info) return -EOPNOTSUPP; + pages = virt_to_page(dev->mdev->clock_info); - return vm_insert_page(vma, vma->vm_start, - virt_to_page(dev->mdev->clock_info)); + return vm_map_pages_zero(vma, &pages, 1); } static int uar_mmap(struct mlx5_ib_dev *dev, enum mlx5_ib_mmap_cmd cmd,
First, length passed to mmap is checked explicitly against PAGE_SIZE. Second, if vma->vm_pgoff is passed as non zero, it would return error. It appears like driver is expecting vma->vm_pgoff to be passed as 0 always. otherwise throw error (not sure if done with a particular purpose). Rather driver could set vma->vm_pgoff to 0 irrespective of the value passed to it. vm_map_pages_zero() has condition to validate incorrect length passed to driver and second it can also set vma->vm_pgoff to 0 before mapping the page to vma. Hence convert to use vm_map_pages_zero(). Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> --- drivers/infiniband/hw/mlx5/main.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)