[linux-next PATCH] rapidio: Fix error handling path

[linux-next PATCH] rapidio: Fix error handling path

[Souptick Joarder]

There is an error when pin_user_pages_fast() returns -ERRNO and
inside error handling path driver end up calling unpin_user_pages()
with -ERRNO which is not correct.

This patch will fix the problem.

Fixes: e8de370188d09 ("rapidio: add mport char device driver")
Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Matthew Wilcox <willy@infradead.org>
---
 drivers/rapidio/devices/rio_mport_cdev.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
index a303429..163b6c72 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -871,15 +871,16 @@ static int do_dma_request(struct mport_dma_req *req,
 				rmcd_error("pin_user_pages_fast err=%ld",
 					   pinned);
 				nr_pages = 0;
-			} else
+			} else {
 				rmcd_error("pinned %ld out of %ld pages",
 					   pinned, nr_pages);
+				/*
+				 * Set nr_pages up to mean "how many pages to unpin, in
+				 * the error handler:
+				 */
+				nr_pages = pinned;
+			}
 			ret = -EFAULT;
-			/*
-			 * Set nr_pages up to mean "how many pages to unpin, in
-			 * the error handler:
-			 */
-			nr_pages = pinned;
 			goto err_pg;
 		}
 
-- 
1.9.1

Re: [linux-next PATCH] rapidio: Fix error handling path

[John Hubbard]

On 9/15/20 8:42 PM, Souptick Joarder wrote:
> There is an error when pin_user_pages_fast() returns -ERRNO and
> inside error handling path driver end up calling unpin_user_pages()
> with -ERRNO which is not correct.
> > This patch will fix the problem.

How about:

rio_dma_transfer() attempts to clamp the return value of
pin_user_pages_fast() to be >= 0. However, the attempt fails because
nr_pages is overridden a few lines later, and restored to the
undesirable -ERRNO value.

The return value is ultimately stored in nr_pages, which in turn is
passed to unpin_user_pages(), which expects nr_pages >= 0, else,
disaster.

Fix this by fixing the nesting of the assignment to nr_pages: nr_pages
should be clamped to zero if pin_user_pages_fast() returns -ERRNO, or
set to the return value of pin_user_pages_fast(), otherwise.

> 
> Fixes: e8de370188d09 ("rapidio: add mport char device driver")
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> ---
>   drivers/rapidio/devices/rio_mport_cdev.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
> index a303429..163b6c72 100644
> --- a/drivers/rapidio/devices/rio_mport_cdev.c
> +++ b/drivers/rapidio/devices/rio_mport_cdev.c
> @@ -871,15 +871,16 @@ static int do_dma_request(struct mport_dma_req *req,
>   				rmcd_error("pin_user_pages_fast err=%ld",
>   					   pinned);
>   				nr_pages = 0;
> -			} else
> +			} else {
>   				rmcd_error("pinned %ld out of %ld pages",
>   					   pinned, nr_pages);
> +				/*
> +				 * Set nr_pages up to mean "how many pages to unpin, in
> +				 * the error handler:
> +				 */
> +				nr_pages = pinned;
> +			}
>   			ret = -EFAULT;
> -			/*
> -			 * Set nr_pages up to mean "how many pages to unpin, in
> -			 * the error handler:
> -			 */
> -			nr_pages = pinned;
>   			goto err_pg;
>   		}
>   
> 

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard
NVIDIA

Re: [linux-next PATCH] rapidio: Fix error handling path

[Dan Carpenter]

On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> There is an error when pin_user_pages_fast() returns -ERRNO and
> inside error handling path driver end up calling unpin_user_pages()
> with -ERRNO which is not correct.
> 
> This patch will fix the problem.

There are a few ways we could prevent bug in the future.

1) This could have been caught with existing static analysis tools
   which warn about when a value is set but not used.

2) I've created a Smatch check which warngs about:

	drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'

   I'll test it out tonight and see how well it works.  I don't
   immediately see any other bugs allthough Smatch doesn't like the code
   in siw_umem_release().  It uses "min_t(int" which suggests that
   negative pages are okay.

	   int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);

3) We could add a check in unpin_user_pages().

	if (WARN_ON(IS_ERR_VALUE(npages)))
		return;

It's not possible to pin more than "ULONG_MAX - 4095" because otherwise
returning error pointers wouldn't work.  So this check can't break
anything and it could prevent a crash.

regards,
dan carpenter

Re: [linux-next PATCH] rapidio: Fix error handling path

[Dan Carpenter]

On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> > There is an error when pin_user_pages_fast() returns -ERRNO and
> > inside error handling path driver end up calling unpin_user_pages()
> > with -ERRNO which is not correct.
> > 
> > This patch will fix the problem.
> 
> There are a few ways we could prevent bug in the future.
> 
> 1) This could have been caught with existing static analysis tools
>    which warn about when a value is set but not used.
> 
> 2) I've created a Smatch check which warngs about:
> 
> 	drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
> 
>    I'll test it out tonight and see how well it works.  I don't
>    immediately see any other bugs allthough Smatch doesn't like the code
>    in siw_umem_release().  It uses "min_t(int" which suggests that
>    negative pages are okay.
> 
> 	   int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> 
> 3) We could add a check in unpin_user_pages().
> 
> 	if (WARN_ON(IS_ERR_VALUE(npages)))
> 		return;
> 
> It's not possible to pin more than "ULONG_MAX - 4095" because otherwise
> returning error pointers wouldn't work.  So this check can't break
> anything and it could prevent a crash.

Actually pin_user_pages_fast() returns an int.  It's not possible to
pin more than INT_MAX.

regards,
dan carpenter

Re: [linux-next PATCH] rapidio: Fix error handling path

[Ira Weiny]

On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> > There is an error when pin_user_pages_fast() returns -ERRNO and
> > inside error handling path driver end up calling unpin_user_pages()
> > with -ERRNO which is not correct.
> > 
> > This patch will fix the problem.
> 
> There are a few ways we could prevent bug in the future.
> 
> 1) This could have been caught with existing static analysis tools
>    which warn about when a value is set but not used.
> 
> 2) I've created a Smatch check which warngs about:
> 
> 	drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
> 
>    I'll test it out tonight and see how well it works.  I don't
>    immediately see any other bugs allthough Smatch doesn't like the code
>    in siw_umem_release().  It uses "min_t(int" which suggests that
>    negative pages are okay.
> 
> 	   int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> 
> 3) We could add a check in unpin_user_pages().
> 
> 	if (WARN_ON(IS_ERR_VALUE(npages)))
> 		return;

Does IS_ERR_VALUE() work on an unsigned variable?  The issue with adding a
check in unpin_user_pages() is that npages is unsigned long.

Ira

> 
> It's not possible to pin more than "ULONG_MAX - 4095" because otherwise
> returning error pointers wouldn't work.  So this check can't break
> anything and it could prevent a crash.
> 
> regards,
> dan carpenter
> 

Re: [linux-next PATCH] rapidio: Fix error handling path

[Ira Weiny]

On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> There is an error when pin_user_pages_fast() returns -ERRNO and
> inside error handling path driver end up calling unpin_user_pages()
> with -ERRNO which is not correct.
> 
> This patch will fix the problem.
> 
> Fixes: e8de370188d09 ("rapidio: add mport char device driver")
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> ---
>  drivers/rapidio/devices/rio_mport_cdev.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
> index a303429..163b6c72 100644
> --- a/drivers/rapidio/devices/rio_mport_cdev.c
> +++ b/drivers/rapidio/devices/rio_mport_cdev.c
> @@ -871,15 +871,16 @@ static int do_dma_request(struct mport_dma_req *req,
>  				rmcd_error("pin_user_pages_fast err=%ld",
>  					   pinned);
>  				nr_pages = 0;
> -			} else
> +			} else {
>  				rmcd_error("pinned %ld out of %ld pages",
>  					   pinned, nr_pages);
> +				/*
> +				 * Set nr_pages up to mean "how many pages to unpin, in
> +				 * the error handler:
> +				 */
> +				nr_pages = pinned;
> +			}
>  			ret = -EFAULT;
> -			/*
> -			 * Set nr_pages up to mean "how many pages to unpin, in
> -			 * the error handler:
> -			 */
> -			nr_pages = pinned;
>  			goto err_pg;
>  		}
>  
> -- 
> 1.9.1
> 

Re: [linux-next PATCH] rapidio: Fix error handling path

[Dan Carpenter]

On Wed, Sep 16, 2020 at 08:16:09AM -0700, Ira Weiny wrote:
> On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> > On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> > > There is an error when pin_user_pages_fast() returns -ERRNO and
> > > inside error handling path driver end up calling unpin_user_pages()
> > > with -ERRNO which is not correct.
> > > 
> > > This patch will fix the problem.
> > 
> > There are a few ways we could prevent bug in the future.
> > 
> > 1) This could have been caught with existing static analysis tools
> >    which warn about when a value is set but not used.
> > 
> > 2) I've created a Smatch check which warngs about:
> > 
> > 	drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
> > 
> >    I'll test it out tonight and see how well it works.  I don't
> >    immediately see any other bugs allthough Smatch doesn't like the code
> >    in siw_umem_release().  It uses "min_t(int" which suggests that
> >    negative pages are okay.
> > 
> > 	   int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> > 
> > 3) We could add a check in unpin_user_pages().
> > 
> > 	if (WARN_ON(IS_ERR_VALUE(npages)))
> > 		return;
> 
> Does IS_ERR_VALUE() work on an unsigned variable?  The issue with adding a
> check in unpin_user_pages() is that npages is unsigned long.

Yeah, it does.  It only works on long and unsigned long.

(Or long long types if they have the same number of bytes as a long).

regards,
dan carpenter

[PATCH] mm/gup: protect unpin_user_pages() against npages==-ERRNO

[John Hubbard]

As suggested by Dan Carpenter, fortify unpin_user_pages() just a bit,
against a typical caller mistake: check if the npages arg is really a
-ERRNO value, which would blow up the unpinning loop: WARN and return.

If this new WARN_ON() fires, then the system *might* be leaking pages
(by leaving them pinned), but probably not. More likely, gup/pup
returned a hard -ERRNO error to the caller, who erroneously passed it
here.

Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---

Hi Dan,

Is is OK to use your signed-off-by here? Since you came up with this.

thanks,
John Hubbard
NVIDIA

 mm/gup.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index e5739a1974d5..41d082707016 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -328,6 +328,13 @@ void unpin_user_pages(struct page **pages, unsigned long npages)
 {
 	unsigned long index;
 
+	/*
+	 * If this WARN_ON() fires, then the system *might* be leaking pages (by
+	 * leaving them pinned), but probably not. More likely, gup/pup returned
+	 * a hard -ERRNO error to the caller, who erroneously passed it here.
+	 */
+	if (WARN_ON(IS_ERR_VALUE(npages)))
+		return;
 	/*
 	 * TODO: this can be optimized for huge pages: if a series of pages is
 	 * physically contiguous and part of the same compound page, then a
-- 
2.28.0

Re: [PATCH] mm/gup: protect unpin_user_pages() against npages==-ERRNO

[Dan Carpenter]

On Wed, Sep 16, 2020 at 11:57:06PM -0700, John Hubbard wrote:
> As suggested by Dan Carpenter, fortify unpin_user_pages() just a bit,
> against a typical caller mistake: check if the npages arg is really a
> -ERRNO value, which would blow up the unpinning loop: WARN and return.
> 
> If this new WARN_ON() fires, then the system *might* be leaking pages
> (by leaving them pinned), but probably not. More likely, gup/pup
> returned a hard -ERRNO error to the caller, who erroneously passed it
> here.
> 
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Souptick Joarder <jrdr.linux@gmail.com>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
> 
> Hi Dan,
> 
> Is is OK to use your signed-off-by here? Since you came up with this.
> 

Yeah.  That's fine.

regards,
dan carpenter

Re: [linux-next PATCH] rapidio: Fix error handling path

[Dan Carpenter]

On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> > There is an error when pin_user_pages_fast() returns -ERRNO and
> > inside error handling path driver end up calling unpin_user_pages()
> > with -ERRNO which is not correct.
> > 
> > This patch will fix the problem.
> 
> There are a few ways we could prevent bug in the future.
> 
> 1) This could have been caught with existing static analysis tools
>    which warn about when a value is set but not used.
> 
> 2) I've created a Smatch check which warngs about:
> 
> 	drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
> 
>    I'll test it out tonight and see how well it works.  I don't
>    immediately see any other bugs allthough Smatch doesn't like the code
>    in siw_umem_release().  It uses "min_t(int" which suggests that
>    negative pages are okay.
> 
> 	   int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
>

I only found one bug but I'm going to add unpin_user_pages_dirty_lock()
to the mix a retest.  There were a few other false positives.  In
reviewing the code, I noticed that orangefs_bufmap_map() is also buggy.

I sort of feel like returning partial successes is not working.  We
could easily make a wrapper which either pins everything or it returns
an error code.

drivers/misc/mic/scif/scif_rma.c:1399 __scif_pin_pages() warn: unpinning negative pages 'pinned_pages->nr_pages'

drivers/misc/mic/scif/scif_rma.c
  1355                          vmalloc_addr = true;
  1356  
  1357                  for (i = 0; i < nr_pages; i++) {
  1358                          if (vmalloc_addr)
  1359                                  pinned_pages->pages[i] =
  1360                                          vmalloc_to_page(addr + (i * PAGE_SIZE));
  1361                          else
  1362                                  pinned_pages->pages[i] =
  1363                                          virt_to_page(addr + (i * PAGE_SIZE));
  1364                  }
  1365                  pinned_pages->nr_pages = nr_pages;
  1366                  pinned_pages->map_flags = SCIF_MAP_KERNEL;
  1367          } else {
  1368                  /*
  1369                   * SCIF supports registration caching. If a registration has
  1370                   * been requested with read only permissions, then we try
  1371                   * to pin the pages with RW permissions so that a subsequent
  1372                   * transfer with RW permission can hit the cache instead of
  1373                   * invalidating it. If the upgrade fails with RW then we
  1374                   * revert back to R permission and retry
  1375                   */
  1376                  if (prot == SCIF_PROT_READ)
  1377                          try_upgrade = true;
  1378                  prot |= SCIF_PROT_WRITE;
  1379  retry:
  1380                  mm = current->mm;
  1381                  if (ulimit) {
  1382                          err = __scif_check_inc_pinned_vm(mm, nr_pages);
  1383                          if (err) {
  1384                                  pinned_pages->nr_pages = 0;
  1385                                  goto error_unmap;
  1386                          }
  1387                  }
  1388  
  1389                  pinned_pages->nr_pages = pin_user_pages_fast(
  1390                                  (u64)addr,
  1391                                  nr_pages,
  1392                                  (prot & SCIF_PROT_WRITE) ? FOLL_WRITE : 0,
  1393                                  pinned_pages->pages);
  1394                  if (nr_pages != pinned_pages->nr_pages) {
  1395                          if (try_upgrade) {
  1396                                  if (ulimit)
  1397                                          __scif_dec_pinned_vm_lock(mm, nr_pages);
  1398                                  /* Roll back any pinned pages */
  1399                                  unpin_user_pages(pinned_pages->pages,
  1400                                                   pinned_pages->nr_pages);
                                                         ^^^^^^^^^^^^^^^^^^^^^^
Negative.

  1401                                  prot &= ~SCIF_PROT_WRITE;
  1402                                  try_upgrade = false;
  1403                                  goto retry;
  1404                          }
  1405                  }
  1406                  pinned_pages->map_flags = 0;
  1407          }
  1408  
  1409          if (pinned_pages->nr_pages < nr_pages) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
These are both signed so it negative ->nr_pages are less than nr_pages.

  1410                  err = -EFAULT;
  1411                  pinned_pages->nr_pages = nr_pages;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This sets it to "everything was pinned".

  1412                  goto dec_pinned;
  1413          }
  1414  
  1415          *out_prot = prot;
  1416          atomic_set(&pinned_pages->ref_count, 1);
  1417          *pages = pinned_pages;
  1418          return err;
  1419  dec_pinned:
  1420          if (ulimit)
  1421                  __scif_dec_pinned_vm_lock(mm, nr_pages);
  1422          /* Something went wrong! Rollback */
  1423  error_unmap:
  1424          pinned_pages->nr_pages = nr_pages;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This assumes everything was pinned successfully.

  1425          scif_destroy_pinned_pages(pinned_pages);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We absolutely don't want to pass negative ->nr_pages to this function
either.

  1426          *pages = NULL;
  1427          dev_dbg(scif_info.mdev.this_device,
  1428                  "%s %d err %d len 0x%lx\n", __func__, __LINE__, err, len);
  1429          return err;
  1430  }

regards,
dan carpenter

Re: [linux-next PATCH] rapidio: Fix error handling path

[Ira Weiny]

On Thu, Sep 17, 2020 at 03:39:51PM +0300, Dan Carpenter wrote:
> On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> > On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> > > There is an error when pin_user_pages_fast() returns -ERRNO and
> > > inside error handling path driver end up calling unpin_user_pages()
> > > with -ERRNO which is not correct.
> > > 
> > > This patch will fix the problem.
> > 
> > There are a few ways we could prevent bug in the future.
> > 
> > 1) This could have been caught with existing static analysis tools
> >    which warn about when a value is set but not used.
> > 
> > 2) I've created a Smatch check which warngs about:
> > 
> > 	drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
> > 
> >    I'll test it out tonight and see how well it works.  I don't
> >    immediately see any other bugs allthough Smatch doesn't like the code
> >    in siw_umem_release().  It uses "min_t(int" which suggests that
> >    negative pages are okay.
> > 
> > 	   int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> >
> 
> I only found one bug but I'm going to add unpin_user_pages_dirty_lock()
> to the mix a retest.  There were a few other false positives.  In
> reviewing the code, I noticed that orangefs_bufmap_map() is also buggy.
> 
> I sort of feel like returning partial successes is not working.  We
> could easily make a wrapper which either pins everything or it returns
> an error code.

I guess the question is are there drivers which will keep working (or limp
along?) on partial pins?  A quick search of a driver I thought did this does
not apparently any more...  So it sounds good to me from 30,000 feet!  :-D

Ira

> 
> drivers/misc/mic/scif/scif_rma.c:1399 __scif_pin_pages() warn: unpinning negative pages 'pinned_pages->nr_pages'
> 
> drivers/misc/mic/scif/scif_rma.c
>   1355                          vmalloc_addr = true;
>   1356  
>   1357                  for (i = 0; i < nr_pages; i++) {
>   1358                          if (vmalloc_addr)
>   1359                                  pinned_pages->pages[i] =
>   1360                                          vmalloc_to_page(addr + (i * PAGE_SIZE));
>   1361                          else
>   1362                                  pinned_pages->pages[i] =
>   1363                                          virt_to_page(addr + (i * PAGE_SIZE));
>   1364                  }
>   1365                  pinned_pages->nr_pages = nr_pages;
>   1366                  pinned_pages->map_flags = SCIF_MAP_KERNEL;
>   1367          } else {
>   1368                  /*
>   1369                   * SCIF supports registration caching. If a registration has
>   1370                   * been requested with read only permissions, then we try
>   1371                   * to pin the pages with RW permissions so that a subsequent
>   1372                   * transfer with RW permission can hit the cache instead of
>   1373                   * invalidating it. If the upgrade fails with RW then we
>   1374                   * revert back to R permission and retry
>   1375                   */
>   1376                  if (prot == SCIF_PROT_READ)
>   1377                          try_upgrade = true;
>   1378                  prot |= SCIF_PROT_WRITE;
>   1379  retry:
>   1380                  mm = current->mm;
>   1381                  if (ulimit) {
>   1382                          err = __scif_check_inc_pinned_vm(mm, nr_pages);
>   1383                          if (err) {
>   1384                                  pinned_pages->nr_pages = 0;
>   1385                                  goto error_unmap;
>   1386                          }
>   1387                  }
>   1388  
>   1389                  pinned_pages->nr_pages = pin_user_pages_fast(
>   1390                                  (u64)addr,
>   1391                                  nr_pages,
>   1392                                  (prot & SCIF_PROT_WRITE) ? FOLL_WRITE : 0,
>   1393                                  pinned_pages->pages);
>   1394                  if (nr_pages != pinned_pages->nr_pages) {
>   1395                          if (try_upgrade) {
>   1396                                  if (ulimit)
>   1397                                          __scif_dec_pinned_vm_lock(mm, nr_pages);
>   1398                                  /* Roll back any pinned pages */
>   1399                                  unpin_user_pages(pinned_pages->pages,
>   1400                                                   pinned_pages->nr_pages);
>                                                          ^^^^^^^^^^^^^^^^^^^^^^
> Negative.
> 
>   1401                                  prot &= ~SCIF_PROT_WRITE;
>   1402                                  try_upgrade = false;
>   1403                                  goto retry;
>   1404                          }
>   1405                  }
>   1406                  pinned_pages->map_flags = 0;
>   1407          }
>   1408  
>   1409          if (pinned_pages->nr_pages < nr_pages) {
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> These are both signed so it negative ->nr_pages are less than nr_pages.
> 
>   1410                  err = -EFAULT;
>   1411                  pinned_pages->nr_pages = nr_pages;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This sets it to "everything was pinned".
> 
>   1412                  goto dec_pinned;
>   1413          }
>   1414  
>   1415          *out_prot = prot;
>   1416          atomic_set(&pinned_pages->ref_count, 1);
>   1417          *pages = pinned_pages;
>   1418          return err;
>   1419  dec_pinned:
>   1420          if (ulimit)
>   1421                  __scif_dec_pinned_vm_lock(mm, nr_pages);
>   1422          /* Something went wrong! Rollback */
>   1423  error_unmap:
>   1424          pinned_pages->nr_pages = nr_pages;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This assumes everything was pinned successfully.
> 
>   1425          scif_destroy_pinned_pages(pinned_pages);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We absolutely don't want to pass negative ->nr_pages to this function
> either.
> 
>   1426          *pages = NULL;
>   1427          dev_dbg(scif_info.mdev.this_device,
>   1428                  "%s %d err %d len 0x%lx\n", __func__, __LINE__, err, len);
>   1429          return err;
>   1430  }
> 
> regards,
> dan carpenter
> 

Re: [linux-next PATCH] rapidio: Fix error handling path

[John Hubbard]

On 9/17/20 10:34 AM, Ira Weiny wrote:
> On Thu, Sep 17, 2020 at 03:39:51PM +0300, Dan Carpenter wrote:
>> On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
>>> On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
>>>> There is an error when pin_user_pages_fast() returns -ERRNO and
>>>> inside error handling path driver end up calling unpin_user_pages()
>>>> with -ERRNO which is not correct.
>>>>
>>>> This patch will fix the problem.
>>>
>>> There are a few ways we could prevent bug in the future.
>>>
>>> 1) This could have been caught with existing static analysis tools
>>>     which warn about when a value is set but not used.
>>>
>>> 2) I've created a Smatch check which warngs about:
>>>
>>> 	drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
>>>
>>>     I'll test it out tonight and see how well it works.  I don't
>>>     immediately see any other bugs allthough Smatch doesn't like the code
>>>     in siw_umem_release().  It uses "min_t(int" which suggests that
>>>     negative pages are okay.
>>>
>>> 	   int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
>>>
>>
>> I only found one bug but I'm going to add unpin_user_pages_dirty_lock()
>> to the mix a retest.  There were a few other false positives.  In
>> reviewing the code, I noticed that orangefs_bufmap_map() is also buggy.
>>
>> I sort of feel like returning partial successes is not working.  We
>> could easily make a wrapper which either pins everything or it returns
>> an error code.

Yes we could. And I have the same feeling about this API. It's generated a
remarkable amount of bug fixes, several of which ended up being partial or
wrong in themselves. And mostly this is due to the complicated tristate
return code: instead of 0 or -ERRNO, it also can return "N pages that is
less than what you requested", and there are no standard helpers in the kernel
to make that easier to deal with.

> 
> I guess the question is are there drivers which will keep working (or limp
> along?) on partial pins?  A quick search of a driver I thought did this does
> not apparently any more...  So it sounds good to me from 30,000 feet!  :-D

It sounds good to me too--and from just a *few hundred feet* (having touched most
of the call sites at some point)! haha :)

I think the wrapper should be short-term, though, just until all the callers
are converted to the simpler API. Then change the core gup/pup calls to the simpler
API. There are more than enough gup/pup API entry points as it is, that's for sure.


thanks,
-- 
John Hubbard
NVIDIA

Re: [linux-next PATCH] rapidio: Fix error handling path

[Souptick Joarder]

On Thu, Sep 17, 2020 at 11:17 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 9/17/20 10:34 AM, Ira Weiny wrote:
> > On Thu, Sep 17, 2020 at 03:39:51PM +0300, Dan Carpenter wrote:
> >> On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> >>> On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> >>>> There is an error when pin_user_pages_fast() returns -ERRNO and
> >>>> inside error handling path driver end up calling unpin_user_pages()
> >>>> with -ERRNO which is not correct.
> >>>>
> >>>> This patch will fix the problem.
> >>>
> >>> There are a few ways we could prevent bug in the future.
> >>>
> >>> 1) This could have been caught with existing static analysis tools
> >>>     which warn about when a value is set but not used.
> >>>
> >>> 2) I've created a Smatch check which warngs about:
> >>>
> >>>     drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
> >>>
> >>>     I'll test it out tonight and see how well it works.  I don't
> >>>     immediately see any other bugs allthough Smatch doesn't like the code
> >>>     in siw_umem_release().  It uses "min_t(int" which suggests that
> >>>     negative pages are okay.
> >>>
> >>>        int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> >>>
> >>
> >> I only found one bug but I'm going to add unpin_user_pages_dirty_lock()
> >> to the mix a retest.  There were a few other false positives.  In
> >> reviewing the code, I noticed that orangefs_bufmap_map() is also buggy.
> >>
> >> I sort of feel like returning partial successes is not working.  We
> >> could easily make a wrapper which either pins everything or it returns
> >> an error code.
>
> Yes we could. And I have the same feeling about this API. It's generated a
> remarkable amount of bug fixes, several of which ended up being partial or
> wrong in themselves. And mostly this is due to the complicated tristate
> return code: instead of 0 or -ERRNO, it also can return "N pages that is
> less than what you requested", and there are no standard helpers in the kernel
> to make that easier to deal with

There was some discussion on removing return value 0 from one of the
gup variants [1].
I think it might be partially relevant to the current discussion.

[1] https://patchwork.kernel.org/patch/11529795/

>
> >
> > I guess the question is are there drivers which will keep working (or limp
> > along?) on partial pins?  A quick search of a driver I thought did this does
> > not apparently any more...  So it sounds good to me from 30,000 feet!  :-D
>
> It sounds good to me too--and from just a *few hundred feet* (having touched most
> of the call sites at some point)! haha :)
>
> I think the wrapper should be short-term, though, just until all the callers
> are converted to the simpler API. Then change the core gup/pup calls to the simpler
> API. There are more than enough gup/pup API entry points as it is, that's for sure.
>
>
> thanks,
> --
> John Hubbard
> NVIDIA

Re: [linux-next PATCH] rapidio: Fix error handling path

[Souptick Joarder]

Hi Dan,

On Thu, Sep 17, 2020 at 6:10 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> > On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> > > There is an error when pin_user_pages_fast() returns -ERRNO and
> > > inside error handling path driver end up calling unpin_user_pages()
> > > with -ERRNO which is not correct.
> > >
> > > This patch will fix the problem.
> >
> > There are a few ways we could prevent bug in the future.
> >
> > 1) This could have been caught with existing static analysis tools
> >    which warn about when a value is set but not used.
> >
> > 2) I've created a Smatch check which warngs about:
> >
> >       drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
> >
> >    I'll test it out tonight and see how well it works.  I don't
> >    immediately see any other bugs allthough Smatch doesn't like the code
> >    in siw_umem_release().  It uses "min_t(int" which suggests that
> >    negative pages are okay.
> >
> >          int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> >
>
> I only found one bug but I'm going to add unpin_user_pages_dirty_lock()
> to the mix a retest.  There were a few other false positives.  In
> reviewing the code, I noticed that orangefs_bufmap_map() is also buggy.
>
> I sort of feel like returning partial successes is not working.  We
> could easily make a wrapper which either pins everything or it returns
> an error code.
>
> drivers/misc/mic/scif/scif_rma.c:1399 __scif_pin_pages() warn: unpinning negative pages 'pinned_pages->nr_pages'
>
> drivers/misc/mic/scif/scif_rma.c
>   1355                          vmalloc_addr = true;
>   1356
>   1357                  for (i = 0; i < nr_pages; i++) {
>   1358                          if (vmalloc_addr)
>   1359                                  pinned_pages->pages[i] =
>   1360                                          vmalloc_to_page(addr + (i * PAGE_SIZE));
>   1361                          else
>   1362                                  pinned_pages->pages[i] =
>   1363                                          virt_to_page(addr + (i * PAGE_SIZE));
>   1364                  }
>   1365                  pinned_pages->nr_pages = nr_pages;
>   1366                  pinned_pages->map_flags = SCIF_MAP_KERNEL;
>   1367          } else {
>   1368                  /*
>   1369                   * SCIF supports registration caching. If a registration has
>   1370                   * been requested with read only permissions, then we try
>   1371                   * to pin the pages with RW permissions so that a subsequent
>   1372                   * transfer with RW permission can hit the cache instead of
>   1373                   * invalidating it. If the upgrade fails with RW then we
>   1374                   * revert back to R permission and retry
>   1375                   */
>   1376                  if (prot == SCIF_PROT_READ)
>   1377                          try_upgrade = true;
>   1378                  prot |= SCIF_PROT_WRITE;
>   1379  retry:
>   1380                  mm = current->mm;
>   1381                  if (ulimit) {
>   1382                          err = __scif_check_inc_pinned_vm(mm, nr_pages);
>   1383                          if (err) {
>   1384                                  pinned_pages->nr_pages = 0;
>   1385                                  goto error_unmap;
>   1386                          }
>   1387                  }
>   1388
>   1389                  pinned_pages->nr_pages = pin_user_pages_fast(
>   1390                                  (u64)addr,
>   1391                                  nr_pages,
>   1392                                  (prot & SCIF_PROT_WRITE) ? FOLL_WRITE : 0,
>   1393                                  pinned_pages->pages);
>   1394                  if (nr_pages != pinned_pages->nr_pages) {
>   1395                          if (try_upgrade) {
>   1396                                  if (ulimit)
>   1397                                          __scif_dec_pinned_vm_lock(mm, nr_pages);
>   1398                                  /* Roll back any pinned pages */
>   1399                                  unpin_user_pages(pinned_pages->pages,
>   1400                                                   pinned_pages->nr_pages);
>                                                          ^^^^^^^^^^^^^^^^^^^^^^
> Negative.
>
>   1401                                  prot &= ~SCIF_PROT_WRITE;
>   1402                                  try_upgrade = false;
>   1403                                  goto retry;
>   1404                          }
>   1405                  }
>   1406                  pinned_pages->map_flags = 0;
>   1407          }
>   1408
>   1409          if (pinned_pages->nr_pages < nr_pages) {
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> These are both signed so it negative ->nr_pages are less than nr_pages.
>
>   1410                  err = -EFAULT;
>   1411                  pinned_pages->nr_pages = nr_pages;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This sets it to "everything was pinned".
>
>   1412                  goto dec_pinned;
>   1413          }
>   1414
>   1415          *out_prot = prot;
>   1416          atomic_set(&pinned_pages->ref_count, 1);
>   1417          *pages = pinned_pages;
>   1418          return err;
>   1419  dec_pinned:
>   1420          if (ulimit)
>   1421                  __scif_dec_pinned_vm_lock(mm, nr_pages);
>   1422          /* Something went wrong! Rollback */
>   1423  error_unmap:
>   1424          pinned_pages->nr_pages = nr_pages;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This assumes everything was pinned successfully.
>
>   1425          scif_destroy_pinned_pages(pinned_pages);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We absolutely don't want to pass negative ->nr_pages to this function
> either.
>
>   1426          *pages = NULL;
>   1427          dev_dbg(scif_info.mdev.this_device,
>   1428                  "%s %d err %d len 0x%lx\n", __func__, __LINE__, err, len);
>   1429          return err;
>   1430  }
>
> regards,
> dan carpenter


I have drafted a patch for this bug. Shall I post it ?

Re: [linux-next PATCH] rapidio: Fix error handling path

[Dan Carpenter]

On Fri, Sep 18, 2020 at 07:55:05AM +0530, Souptick Joarder wrote:
> Hi Dan,
> 
> On Thu, Sep 17, 2020 at 6:10 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> > > On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> > > > There is an error when pin_user_pages_fast() returns -ERRNO and
> > > > inside error handling path driver end up calling unpin_user_pages()
> > > > with -ERRNO which is not correct.
> > > >
> > > > This patch will fix the problem.
> > >
> > > There are a few ways we could prevent bug in the future.
> > >
> > > 1) This could have been caught with existing static analysis tools
> > >    which warn about when a value is set but not used.
> > >
> > > 2) I've created a Smatch check which warngs about:
> > >
> > >       drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
> > >
> > >    I'll test it out tonight and see how well it works.  I don't
> > >    immediately see any other bugs allthough Smatch doesn't like the code
> > >    in siw_umem_release().  It uses "min_t(int" which suggests that
> > >    negative pages are okay.
> > >
> > >          int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> > >
> >
> > I only found one bug but I'm going to add unpin_user_pages_dirty_lock()
> > to the mix a retest.  There were a few other false positives.  In
> > reviewing the code, I noticed that orangefs_bufmap_map() is also buggy.
> >
> > I sort of feel like returning partial successes is not working.  We
> > could easily make a wrapper which either pins everything or it returns
> > an error code.
> >
> > drivers/misc/mic/scif/scif_rma.c:1399 __scif_pin_pages() warn: unpinning negative pages 'pinned_pages->nr_pages'
> >
> > drivers/misc/mic/scif/scif_rma.c
> >   1355                          vmalloc_addr = true;
> >   1356
> >   1357                  for (i = 0; i < nr_pages; i++) {
> >   1358                          if (vmalloc_addr)
> >   1359                                  pinned_pages->pages[i] =
> >   1360                                          vmalloc_to_page(addr + (i * PAGE_SIZE));
> >   1361                          else
> >   1362                                  pinned_pages->pages[i] =
> >   1363                                          virt_to_page(addr + (i * PAGE_SIZE));
> >   1364                  }
> >   1365                  pinned_pages->nr_pages = nr_pages;
> >   1366                  pinned_pages->map_flags = SCIF_MAP_KERNEL;
> >   1367          } else {
> >   1368                  /*
> >   1369                   * SCIF supports registration caching. If a registration has
> >   1370                   * been requested with read only permissions, then we try
> >   1371                   * to pin the pages with RW permissions so that a subsequent
> >   1372                   * transfer with RW permission can hit the cache instead of
> >   1373                   * invalidating it. If the upgrade fails with RW then we
> >   1374                   * revert back to R permission and retry
> >   1375                   */
> >   1376                  if (prot == SCIF_PROT_READ)
> >   1377                          try_upgrade = true;
> >   1378                  prot |= SCIF_PROT_WRITE;
> >   1379  retry:
> >   1380                  mm = current->mm;
> >   1381                  if (ulimit) {
> >   1382                          err = __scif_check_inc_pinned_vm(mm, nr_pages);
> >   1383                          if (err) {
> >   1384                                  pinned_pages->nr_pages = 0;
> >   1385                                  goto error_unmap;
> >   1386                          }
> >   1387                  }
> >   1388
> >   1389                  pinned_pages->nr_pages = pin_user_pages_fast(
> >   1390                                  (u64)addr,
> >   1391                                  nr_pages,
> >   1392                                  (prot & SCIF_PROT_WRITE) ? FOLL_WRITE : 0,
> >   1393                                  pinned_pages->pages);
> >   1394                  if (nr_pages != pinned_pages->nr_pages) {
> >   1395                          if (try_upgrade) {
> >   1396                                  if (ulimit)
> >   1397                                          __scif_dec_pinned_vm_lock(mm, nr_pages);
> >   1398                                  /* Roll back any pinned pages */
> >   1399                                  unpin_user_pages(pinned_pages->pages,
> >   1400                                                   pinned_pages->nr_pages);
> >                                                          ^^^^^^^^^^^^^^^^^^^^^^
> > Negative.
> >
> >   1401                                  prot &= ~SCIF_PROT_WRITE;
> >   1402                                  try_upgrade = false;
> >   1403                                  goto retry;
> >   1404                          }
> >   1405                  }
> >   1406                  pinned_pages->map_flags = 0;
> >   1407          }
> >   1408
> >   1409          if (pinned_pages->nr_pages < nr_pages) {
> >                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > These are both signed so it negative ->nr_pages are less than nr_pages.
> >
> >   1410                  err = -EFAULT;
> >   1411                  pinned_pages->nr_pages = nr_pages;
> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This sets it to "everything was pinned".
> >
> >   1412                  goto dec_pinned;
> >   1413          }
> >   1414
> >   1415          *out_prot = prot;
> >   1416          atomic_set(&pinned_pages->ref_count, 1);
> >   1417          *pages = pinned_pages;
> >   1418          return err;
> >   1419  dec_pinned:
> >   1420          if (ulimit)
> >   1421                  __scif_dec_pinned_vm_lock(mm, nr_pages);
> >   1422          /* Something went wrong! Rollback */
> >   1423  error_unmap:
> >   1424          pinned_pages->nr_pages = nr_pages;
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This assumes everything was pinned successfully.
> >
> >   1425          scif_destroy_pinned_pages(pinned_pages);
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > We absolutely don't want to pass negative ->nr_pages to this function
> > either.
> >
> >   1426          *pages = NULL;
> >   1427          dev_dbg(scif_info.mdev.this_device,
> >   1428                  "%s %d err %d len 0x%lx\n", __func__, __LINE__, err, len);
> >   1429          return err;
> >   1430  }
> >
> > regards,
> > dan carpenter
> 
> 
> I have drafted a patch for this bug. Shall I post it ?

Absolutely, please.  :)

regards,
dan carpenter

Re: [linux-next PATCH] rapidio: Fix error handling path

[John Hubbard]

On 9/17/20 7:21 PM, Souptick Joarder wrote:
> On Thu, Sep 17, 2020 at 11:17 PM John Hubbard <jhubbard@nvidia.com> wrote:
...
>>>> I sort of feel like returning partial successes is not working.  We
>>>> could easily make a wrapper which either pins everything or it returns
>>>> an error code.
>>
>> Yes we could. And I have the same feeling about this API. It's generated a
>> remarkable amount of bug fixes, several of which ended up being partial or
>> wrong in themselves. And mostly this is due to the complicated tristate
>> return code: instead of 0 or -ERRNO, it also can return "N pages that is
>> less than what you requested", and there are no standard helpers in the kernel
>> to make that easier to deal with
> 
> There was some discussion on removing return value 0 from one of the
> gup variants [1].
> I think it might be partially relevant to the current discussion.
> 
> [1] https://patchwork.kernel.org/patch/11529795/
> 

Yes, although as I mentioned above, I'm thinking of a 0 or -ERRNO return value,
and not even return nr_pages at all.

But in any case, as a practical matter, I'm not sure if it's a good idea to
actually change all the callsites, or not. If we just fix the remaining buggy
callers, maybe that's better than the churn associated with another API change.

On the other-other hand, there does seem to be more churn coming anyway, with
talk of actually doing a [get|pin]_user_bvec(), for example. So maybe it's better
to head off the coming mess.

This is something that should be discussed on linux-mm.

>>
>>>
>>> I guess the question is are there drivers which will keep working (or limp
>>> along?) on partial pins?  A quick search of a driver I thought did this does
>>> not apparently any more...  So it sounds good to me from 30,000 feet!  :-D
>>
>> It sounds good to me too--and from just a *few hundred feet* (having touched most
>> of the call sites at some point)! haha :)
>>
>> I think the wrapper should be short-term, though, just until all the callers
>> are converted to the simpler API. Then change the core gup/pup calls to the simpler
>> API. There are more than enough gup/pup API entry points as it is, that's for sure.
>>
>>
>> thanks,
>> --
>> John Hubbard
>> NVIDIA

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm/gup: protect unpin_user_pages() against npages==-ERRNO

[Souptick Joarder]

On Thu, Sep 17, 2020 at 1:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Sep 16, 2020 at 11:57:06PM -0700, John Hubbard wrote:
> > As suggested by Dan Carpenter, fortify unpin_user_pages() just a bit,
> > against a typical caller mistake: check if the npages arg is really a
> > -ERRNO value, which would blow up the unpinning loop: WARN and return.
> >
> > If this new WARN_ON() fires, then the system *might* be leaking pages
> > (by leaving them pinned), but probably not. More likely, gup/pup
> > returned a hard -ERRNO error to the caller, who erroneously passed it
> > here.
> >
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Cc: Souptick Joarder <jrdr.linux@gmail.com>
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> > ---
> >
> > Hi Dan,
> >
> > Is is OK to use your signed-off-by here? Since you came up with this.
> >
>
> Yeah.  That's fine.

Do we need a similar check inside unpin_user_pages_dirty_lock(),
when make_dirty set to false ?

Re: [PATCH] mm/gup: protect unpin_user_pages() against npages==-ERRNO

[John Hubbard]

On 9/19/20 8:03 PM, Souptick Joarder wrote:
> On Thu, Sep 17, 2020 at 1:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> On Wed, Sep 16, 2020 at 11:57:06PM -0700, John Hubbard wrote:
>>> As suggested by Dan Carpenter, fortify unpin_user_pages() just a bit,
>>> against a typical caller mistake: check if the npages arg is really a
>>> -ERRNO value, which would blow up the unpinning loop: WARN and return.
>>>
>>> If this new WARN_ON() fires, then the system *might* be leaking pages
>>> (by leaving them pinned), but probably not. More likely, gup/pup
>>> returned a hard -ERRNO error to the caller, who erroneously passed it
>>> here.
...
> 
> Do we need a similar check inside unpin_user_pages_dirty_lock(),
> when make_dirty set to false ?


Maybe not. This call is rarely if ever used for error handling, but
rather, for finishing up a successful use of the pages.

There is a balance between protecting against buggy callers and just
fixing any buggy callers. There is also a limit to how much code one can
write in hopes of avoiding bugs in...code that one writes. :)  Which is
why static analysis, unit and regression tests, code reviews are
important too.

Here, I submit that that we're about to cross the line and go too far.
But if you have any examples of buggy callers for
unpin_user_pages_dirty_lock(), that might shift the line.

Or maybe others feel that we haven't gone far enough yet after all?


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm/gup: protect unpin_user_pages() against npages==-ERRNO

[Dan Carpenter]

On Sat, Sep 19, 2020 at 09:13:17PM -0700, John Hubbard wrote:
> On 9/19/20 8:03 PM, Souptick Joarder wrote:
> > On Thu, Sep 17, 2020 at 1:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > On Wed, Sep 16, 2020 at 11:57:06PM -0700, John Hubbard wrote:
> > > > As suggested by Dan Carpenter, fortify unpin_user_pages() just a bit,
> > > > against a typical caller mistake: check if the npages arg is really a
> > > > -ERRNO value, which would blow up the unpinning loop: WARN and return.
> > > > 
> > > > If this new WARN_ON() fires, then the system *might* be leaking pages
> > > > (by leaving them pinned), but probably not. More likely, gup/pup
> > > > returned a hard -ERRNO error to the caller, who erroneously passed it
> > > > here.
> ...
> > 
> > Do we need a similar check inside unpin_user_pages_dirty_lock(),
> > when make_dirty set to false ?
> 
> 
> Maybe not. This call is rarely if ever used for error handling, but
> rather, for finishing up a successful use of the pages.
> 
> There is a balance between protecting against buggy callers and just
> fixing any buggy callers. There is also a limit to how much code one can
> write in hopes of avoiding bugs in...code that one writes. :)  Which is
> why static analysis, unit and regression tests, code reviews are
> important too.
> 
> Here, I submit that that we're about to cross the line and go too far.
> But if you have any examples of buggy callers for
> unpin_user_pages_dirty_lock(), that might shift the line.

I checked for buggy uses of unpin_user_pages_dirty_lock() using Smatch
and didn't find anything.  (Which doesn't mean that there aren't any).

regards,
dan carpenter