# \[PATCH 0/2\] video: fbdev: fix error handling, convert to pin\_user\_pages\*()

- Subsystems: FRAMEBUFFER LAYER, THE REST
- Maintainers: Helge Deller, Linus Torvalds
- Series: 2 patches touching 1 file, +18/-6
- Status: LANDED
- Landed in mainline.
- List: linux-fbdev
- 5 messages, 2 authors
- Page: https://ratatoskr.run/linux-fbdev/2020/05/5974564/t

## \[PATCH 0/2\] video: fbdev: fix error handling, convert to pin\_user\_pages\*()

- From: John Hubbard \<jhubbard@nvidia.com\>
- Date: 2020-05-22 04:15:11 
- Message: https://ratatoskr.run/linux-fbdev/2020/05/5974564.md

```text
Hi,

Note that I have only compile-tested this series, although that does
also include cross-compiling for a few other arches. I'm hoping that
this posting will lead to some run-time testing.

Also: the proposed fix does not have a "Fixes:" tag, nor does it
Cc stable. That's because the issue has been there since the dawn of
git history for the kernel. If it's gone unnoticed this long, then
there is clearly no need for the relatively fast track of putting it
into stable, IMHO. But please correct me if that's wrong.

Cc: Bartlomiej Zolnierkiewicz <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Daniel Vetter <redacted>
Cc: Gustavo A. R. Silva <redacted>
Cc: Jani Nikula <redacted>
Cc: <redacted>
Cc: <redacted>

John Hubbard (2):
  video: fbdev: fix error handling for get_user_pages_fast()
  video: fbdev: convert get_user_pages() --> pin_user_pages()

 drivers/video/fbdev/pvr2fb.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)


base-commit: 051143e1602d90ea71887d92363edd539d411de5
-- 
2.26.2
```

## \[PATCH 1/2\] video: fbdev: fix error handling for get\_user\_pages\_fast()

- From: John Hubbard \<jhubbard@nvidia.com\>
- Date: 2020-05-22 04:15:13 
- Message: https://ratatoskr.run/linux-fbdev/2020/05/5974565.md

```text
Dealing with the return value of get_user_pages*() variants has a few
classic pitfalls, and this driver found one of them: the return value
might be zero, positive, or -errno. And if positive, it might be fewer
pages than were requested. And if fewer pages than requested, then
the caller should return (via put_page()) the pages that *were*
pinned.

This driver was doing that *except* that it had a problem with the
-errno case, which was being stored in an unsigned int, and which
would case an interesting mess if it ever happened: nr_pages would be
interpreted as a spectacularly huge unsigned value, rather than a
small negative value. Also, it was unnecessarily overriding a
potentially informative -errno, with -EINVAL, in some cases.

Instead: clamp the nr_pages to zero or positive, so that the error
handling works. And return the -errno value from get_user_pages*(),
unchanged, if we get one. And explain this with comments, seeing as
how it is error-prone.

Cc: Bartlomiej Zolnierkiewicz <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Daniel Vetter <redacted>
Cc: Gustavo A. R. Silva <redacted>
Cc: Jani Nikula <redacted>
Cc: <redacted>
Cc: <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/video/fbdev/pvr2fb.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c
index f18d457175d9..ceb6ef590597 100644
--- a/drivers/video/fbdev/pvr2fb.c
+++ b/drivers/video/fbdev/pvr2fb.c
@@ -654,8 +654,22 @@ static ssize_t pvr2fb_write(struct fb_info *info, const char *buf,
 
 	ret = get_user_pages_fast((unsigned long)buf, nr_pages, FOLL_WRITE, pages);
 	if (ret < nr_pages) {
-		nr_pages = ret;
-		ret = -EINVAL;
+		if (ret < 0) {
+			/*
+			 *  Clamp the unsigned nr_pages to zero so that the
+			 *  error handling works. And leave ret at whatever
+			 *  -errno value was returned from GUP.
+			 */
+			nr_pages = 0;
+		} else {
+			nr_pages = ret;
+			/*
+			 * Use -EINVAL to represent a mildly desperate guess at
+			 * why we got fewer pages (maybe even zero pages) than
+			 * requested.
+			 */
+			ret = -EINVAL;
+		}
 		goto out_unmap;
 	}
 
-- 
2.26.2
```

## \[PATCH 2/2\] video: fbdev: convert get\_user\_pages() --\> pin\_user\_pages()

- From: John Hubbard \<jhubbard@nvidia.com\>
- Date: 2020-05-22 04:15:13 
- Message: https://ratatoskr.run/linux-fbdev/2020/05/5974566.md

```text
This code was using get_user_pages*(), in a "Case 2" scenario
(DMA/RDMA), using the categorization from [1]. That means that it's
time to convert the get_user_pages*() + put_page() calls to
pin_user_pages*() + unpin_user_pages() calls.

There is some helpful background in [2]: basically, this is a small
part of fixing a long-standing disconnect between pinning pages, and
file systems' use of those pages.

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
    https://lwn.net/Articles/807108/

Cc: Bartlomiej Zolnierkiewicz <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Daniel Vetter <redacted>
Cc: Gustavo A. R. Silva <redacted>
Cc: Jani Nikula <redacted>
Cc: <redacted>
Cc: <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/video/fbdev/pvr2fb.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c
index ceb6ef590597..2d9f69b93392 100644
--- a/drivers/video/fbdev/pvr2fb.c
+++ b/drivers/video/fbdev/pvr2fb.c
@@ -652,7 +652,7 @@ static ssize_t pvr2fb_write(struct fb_info *info, const char *buf,
 	if (!pages)
 		return -ENOMEM;
 
-	ret = get_user_pages_fast((unsigned long)buf, nr_pages, FOLL_WRITE, pages);
+	ret = pin_user_pages_fast((unsigned long)buf, nr_pages, FOLL_WRITE, pages);
 	if (ret < nr_pages) {
 		if (ret < 0) {
 			/*
@@ -712,9 +712,7 @@ static ssize_t pvr2fb_write(struct fb_info *info, const char *buf,
 	ret = count;
 
 out_unmap:
-	for (i = 0; i < nr_pages; i++)
-		put_page(pages[i]);
-
+	unpin_user_pages(pages, nr_pages);
 	kfree(pages);
 
 	return ret;
-- 
2.26.2
```

## Re: \[PATCH 0/2\] video: fbdev: fix error handling, convert to pin\_user\_pages\*()

- From: Sam Ravnborg \<hidden\>
- Date: 2020-05-31 20:58:26 
- Message: https://ratatoskr.run/linux-fbdev/2020/05/5984822.md

```text
Hi John.
On Thu, May 21, 2020 at 09:15:04PM -0700, John Hubbard wrote:
> Hi,
> 
> Note that I have only compile-tested this series, although that does
> also include cross-compiling for a few other arches. I'm hoping that
> this posting will lead to some run-time testing.
> 
> Also: the proposed fix does not have a "Fixes:" tag, nor does it
> Cc stable. That's because the issue has been there since the dawn of
> git history for the kernel. If it's gone unnoticed this long, then
> there is clearly no need for the relatively fast track of putting it
> into stable, IMHO. But please correct me if that's wrong.
> 
> Cc: Bartlomiej Zolnierkiewicz <redacted>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Daniel Vetter <redacted>
> Cc: Gustavo A. R. Silva <redacted>
> Cc: Jani Nikula <redacted>
> Cc: <redacted>
> Cc: <redacted>

Thanks, patches are now applied to drm-misc-next.
They will hit -next soon, but you will have to wait
until next (not the upcoming) merge window before they hit
mainline linux.

	Sam

> 
> John Hubbard (2):
>   video: fbdev: fix error handling for get_user_pages_fast()
>   video: fbdev: convert get_user_pages() --> pin_user_pages()
> 
>  drivers/video/fbdev/pvr2fb.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> 
> base-commit: 051143e1602d90ea71887d92363edd539d411de5
> -- 
> 2.26.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
```

## Re: \[PATCH 0/2\] video: fbdev: fix error handling, convert to pin\_user\_pages\*()

- From: John Hubbard \<jhubbard@nvidia.com\>
- Date: 2020-05-31 21:06:56 
- Message: https://ratatoskr.run/linux-fbdev/2020/05/5984827.md

```text
On 2020-05-31 13:58, Sam Ravnborg wrote:
...
> Thanks, patches are now applied to drm-misc-next.
> They will hit -next soon, but you will have to wait
> until next (not the upcoming) merge window before they hit
> mainline linux.
> 
> 	Sam
> 

Great! That will work out just fine.


thanks,
-- 
John Hubbard
NVIDIA
```