[PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[Souptick Joarder]

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(-)

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,
-- 
1.9.1

Re: [PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[Souptick Joarder]

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;

Re: [PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[Souptick Joarder]

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 ?

Re: [PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[Jason Gunthorpe]

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

Re: [PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[Souptick Joarder]

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.

Re: [PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[Leon Romanovsky]

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