[PATCH v2] media: atomisp: Fixed error handling path

[PATCH v2] media: atomisp: Fixed error handling path

[Souptick Joarder]

Inside alloc_user_pages() based on flag value either pin_user_pages()
or get_user_pages_fast() will be called. However, these API might fail.

But free_user_pages() called in error handling path doesn't bother
about return value and will try to unpin bo->pgnr pages, which is
incorrect.

Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
pages will be unpinned based on bo->mem_type. This will also take care
of non error handling path.

Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
allocation")
Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
---
v2:
    Added review tag.

 drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
index f13af23..0168f98 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
@@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object *bo,
 	kfree(bo->page_obj);
 }
 
-static void free_user_pages(struct hmm_buffer_object *bo)
+static void free_user_pages(struct hmm_buffer_object *bo,
+			    unsigned int page_nr)
 {
 	int i;
 
 	hmm_mem_stat.usr_size -= bo->pgnr;
 
 	if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
-		unpin_user_pages(bo->pages, bo->pgnr);
+		unpin_user_pages(bo->pages, page_nr);
 	} else {
-		for (i = 0; i < bo->pgnr; i++)
+		for (i = 0; i < page_nr; i++)
 			put_page(bo->pages[i]);
 	}
 	kfree(bo->pages);
@@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 		dev_err(atomisp_dev,
 			"get_user_pages err: bo->pgnr = %d, pgnr actually pinned = %d.\n",
 			bo->pgnr, page_nr);
+		if (page_nr < 0)
+			page_nr = 0;
 		goto out_of_mem;
 	}
 
@@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 
 out_of_mem:
 
-	free_user_pages(bo);
+	free_user_pages(bo, page_nr);
 
 	return -ENOMEM;
 }
@@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
 	if (bo->type == HMM_BO_PRIVATE)
 		free_private_pages(bo, &dynamic_pool, &reserved_pool);
 	else if (bo->type == HMM_BO_USER)
-		free_user_pages(bo);
+		free_user_pages(bo, bo->pgnr);
 	else
 		dev_err(atomisp_dev, "invalid buffer type.\n");
 	mutex_unlock(&bo->mutex);
-- 
1.9.1

Re: [PATCH v2] media: atomisp: Fix error handling path

[Dan Carpenter]

On Wed, Nov 04, 2020 at 08:15:58AM +0100, Markus Elfring wrote:
> > Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> > allocation")
> 
> Please delete a line break for this tag.

Markus, the thing is that we all saw the line break and we just thought
it didn't matter at all...

regards,
dan carpenter

Re: [v2] media: atomisp: Fix error handling path

[Dan Carpenter]

On Wed, Nov 04, 2020 at 11:30:29AM +0100, Markus Elfring wrote:
> >>> Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> >>> allocation")
> >>
> >> Please delete a line break for this tag.
> >
> > Markus, the thing is that we all saw the line break and we just thought
> > it didn't matter at all...
> 
> Do you disagree to the known documentation then?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=4ef8451b332662d004df269d4cdeb7d9f31419b5#n123 

The documentation is correct but no one wants you to constantly be
nagging developers about minor stuff...

One thing that I do is I start to write an email and then if I realize
it's not worth complaining about and I save it to my postponed messages
folder.  Then I never send it and I forget about it completely.  I have
currently have 740 messages in my postponed messages folder.  :P

That's a lot of whining and complaining which I never sent and the world
is the more beautiful for it.

regards,
dan carpenter

Re: [PATCH v2] media: atomisp: Fixed error handling path

[Souptick Joarder]

On Wed, Nov 4, 2020 at 7:32 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> Inside alloc_user_pages() based on flag value either pin_user_pages()
> or get_user_pages_fast() will be called. However, these API might fail.
>
> But free_user_pages() called in error handling path doesn't bother
> about return value and will try to unpin bo->pgnr pages, which is
> incorrect.
>
> Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
> pages will be unpinned based on bo->mem_type. This will also take care
> of non error handling path.
>
> Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> allocation")
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2:
>     Added review tag.

Any further comment ? If no, can we get this patch in queue for 5.11 ?
>
>  drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> index f13af23..0168f98 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> @@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object *bo,
>         kfree(bo->page_obj);
>  }
>
> -static void free_user_pages(struct hmm_buffer_object *bo)
> +static void free_user_pages(struct hmm_buffer_object *bo,
> +                           unsigned int page_nr)
>  {
>         int i;
>
>         hmm_mem_stat.usr_size -= bo->pgnr;
>
>         if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
> -               unpin_user_pages(bo->pages, bo->pgnr);
> +               unpin_user_pages(bo->pages, page_nr);
>         } else {
> -               for (i = 0; i < bo->pgnr; i++)
> +               for (i = 0; i < page_nr; i++)
>                         put_page(bo->pages[i]);
>         }
>         kfree(bo->pages);
> @@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
>                 dev_err(atomisp_dev,
>                         "get_user_pages err: bo->pgnr = %d, pgnr actually pinned = %d.\n",
>                         bo->pgnr, page_nr);
> +               if (page_nr < 0)
> +                       page_nr = 0;
>                 goto out_of_mem;
>         }
>
> @@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
>
>  out_of_mem:
>
> -       free_user_pages(bo);
> +       free_user_pages(bo, page_nr);
>
>         return -ENOMEM;
>  }
> @@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
>         if (bo->type == HMM_BO_PRIVATE)
>                 free_private_pages(bo, &dynamic_pool, &reserved_pool);
>         else if (bo->type == HMM_BO_USER)
> -               free_user_pages(bo);
> +               free_user_pages(bo, bo->pgnr);
>         else
>                 dev_err(atomisp_dev, "invalid buffer type.\n");
>         mutex_unlock(&bo->mutex);
> --
> 1.9.1
>

Re: [PATCH v2] media: atomisp: Fixed error handling path

[Souptick Joarder]

On Thu, Nov 19, 2020 at 1:06 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> On Wed, Nov 4, 2020 at 7:32 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> >
> > Inside alloc_user_pages() based on flag value either pin_user_pages()
> > or get_user_pages_fast() will be called. However, these API might fail.
> >
> > But free_user_pages() called in error handling path doesn't bother
> > about return value and will try to unpin bo->pgnr pages, which is
> > incorrect.
> >
> > Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
> > pages will be unpinned based on bo->mem_type. This will also take care
> > of non error handling path.
> >
> > Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> > allocation")
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > v2:
> >     Added review tag.
>
> Any further comment ? If no, can we get this patch in queue for 5.11 ?

Can we get this patch in the queue for 5.11 ?

> >
> >  drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > index f13af23..0168f98 100644
> > --- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > @@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object *bo,
> >         kfree(bo->page_obj);
> >  }
> >
> > -static void free_user_pages(struct hmm_buffer_object *bo)
> > +static void free_user_pages(struct hmm_buffer_object *bo,
> > +                           unsigned int page_nr)
> >  {
> >         int i;
> >
> >         hmm_mem_stat.usr_size -= bo->pgnr;
> >
> >         if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
> > -               unpin_user_pages(bo->pages, bo->pgnr);
> > +               unpin_user_pages(bo->pages, page_nr);
> >         } else {
> > -               for (i = 0; i < bo->pgnr; i++)
> > +               for (i = 0; i < page_nr; i++)
> >                         put_page(bo->pages[i]);
> >         }
> >         kfree(bo->pages);
> > @@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
> >                 dev_err(atomisp_dev,
> >                         "get_user_pages err: bo->pgnr = %d, pgnr actually pinned = %d.\n",
> >                         bo->pgnr, page_nr);
> > +               if (page_nr < 0)
> > +                       page_nr = 0;
> >                 goto out_of_mem;
> >         }
> >
> > @@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
> >
> >  out_of_mem:
> >
> > -       free_user_pages(bo);
> > +       free_user_pages(bo, page_nr);
> >
> >         return -ENOMEM;
> >  }
> > @@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
> >         if (bo->type == HMM_BO_PRIVATE)
> >                 free_private_pages(bo, &dynamic_pool, &reserved_pool);
> >         else if (bo->type == HMM_BO_USER)
> > -               free_user_pages(bo);
> > +               free_user_pages(bo, bo->pgnr);
> >         else
> >                 dev_err(atomisp_dev, "invalid buffer type.\n");
> >         mutex_unlock(&bo->mutex);
> > --
> > 1.9.1
> >

Re: [PATCH v2] media: atomisp: Fixed error handling path

[Souptick Joarder]

On Wed, Dec 9, 2020 at 1:18 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> On Thu, Nov 19, 2020 at 1:06 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> >
> > On Wed, Nov 4, 2020 at 7:32 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> > >
> > > Inside alloc_user_pages() based on flag value either pin_user_pages()
> > > or get_user_pages_fast() will be called. However, these API might fail.
> > >
> > > But free_user_pages() called in error handling path doesn't bother
> > > about return value and will try to unpin bo->pgnr pages, which is
> > > incorrect.
> > >
> > > Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
> > > pages will be unpinned based on bo->mem_type. This will also take care
> > > of non error handling path.
> > >
> > > Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> > > allocation")
> > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Cc: John Hubbard <jhubbard@nvidia.com>
> > > Cc: Ira Weiny <ira.weiny@intel.com>
> > > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > > v2:
> > >     Added review tag.
> >
> > Any further comment ? If no, can we get this patch in queue for 5.11 ?
>
> Can we get this patch in the queue for 5.11 ?

Any further comment on this patch ?

>
> > >
> > >  drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 ++++++++-----
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > > index f13af23..0168f98 100644
> > > --- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > > @@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object *bo,
> > >         kfree(bo->page_obj);
> > >  }
> > >
> > > -static void free_user_pages(struct hmm_buffer_object *bo)
> > > +static void free_user_pages(struct hmm_buffer_object *bo,
> > > +                           unsigned int page_nr)
> > >  {
> > >         int i;
> > >
> > >         hmm_mem_stat.usr_size -= bo->pgnr;
> > >
> > >         if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
> > > -               unpin_user_pages(bo->pages, bo->pgnr);
> > > +               unpin_user_pages(bo->pages, page_nr);
> > >         } else {
> > > -               for (i = 0; i < bo->pgnr; i++)
> > > +               for (i = 0; i < page_nr; i++)
> > >                         put_page(bo->pages[i]);
> > >         }
> > >         kfree(bo->pages);
> > > @@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
> > >                 dev_err(atomisp_dev,
> > >                         "get_user_pages err: bo->pgnr = %d, pgnr actually pinned = %d.\n",
> > >                         bo->pgnr, page_nr);
> > > +               if (page_nr < 0)
> > > +                       page_nr = 0;
> > >                 goto out_of_mem;
> > >         }
> > >
> > > @@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
> > >
> > >  out_of_mem:
> > >
> > > -       free_user_pages(bo);
> > > +       free_user_pages(bo, page_nr);
> > >
> > >         return -ENOMEM;
> > >  }
> > > @@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
> > >         if (bo->type == HMM_BO_PRIVATE)
> > >                 free_private_pages(bo, &dynamic_pool, &reserved_pool);
> > >         else if (bo->type == HMM_BO_USER)
> > > -               free_user_pages(bo);
> > > +               free_user_pages(bo, bo->pgnr);
> > >         else
> > >                 dev_err(atomisp_dev, "invalid buffer type.\n");
> > >         mutex_unlock(&bo->mutex);
> > > --
> > > 1.9.1
> > >