dri-devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()
@ 2020-05-22  4:15 John Hubbard
  2020-05-22  4:15 ` [PATCH 1/2] video: fbdev: fix error handling for get_user_pages_fast() John Hubbard
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: John Hubbard @ 2020-05-22  4:15 UTC (permalink / raw)
  To: LKML
  Cc: linux-fbdev, Arnd Bergmann, Gustavo A . R . Silva, Jani Nikula,
	Daniel Vetter, Bartlomiej Zolnierkiewicz, dri-devel,
	John Hubbard

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 <b.zolnierkie@samsung.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] video: fbdev: fix error handling for get_user_pages_fast()
  2020-05-22  4:15 [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*() John Hubbard
@ 2020-05-22  4:15 ` John Hubbard
  2020-05-22  4:15 ` [PATCH 2/2] video: fbdev: convert get_user_pages() --> pin_user_pages() John Hubbard
  2020-05-31 20:58 ` [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*() Sam Ravnborg
  2 siblings, 0 replies; 7+ messages in thread
From: John Hubbard @ 2020-05-22  4:15 UTC (permalink / raw)
  To: LKML
  Cc: linux-fbdev, Arnd Bergmann, Gustavo A . R . Silva, Jani Nikula,
	Daniel Vetter, Bartlomiej Zolnierkiewicz, dri-devel,
	John Hubbard

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 <b.zolnierkie@samsung.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] video: fbdev: convert get_user_pages() --> pin_user_pages()
  2020-05-22  4:15 [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*() John Hubbard
  2020-05-22  4:15 ` [PATCH 1/2] video: fbdev: fix error handling for get_user_pages_fast() John Hubbard
@ 2020-05-22  4:15 ` John Hubbard
  2020-05-31 20:58 ` [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*() Sam Ravnborg
  2 siblings, 0 replies; 7+ messages in thread
From: John Hubbard @ 2020-05-22  4:15 UTC (permalink / raw)
  To: LKML
  Cc: linux-fbdev, Arnd Bergmann, Gustavo A . R . Silva, Jani Nikula,
	Daniel Vetter, Bartlomiej Zolnierkiewicz, dri-devel,
	John Hubbard

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 <b.zolnierkie@samsung.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fbdev@vger.kernel.org
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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()
  2020-05-22  4:15 [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*() John Hubbard
  2020-05-22  4:15 ` [PATCH 1/2] video: fbdev: fix error handling for get_user_pages_fast() John Hubbard
  2020-05-22  4:15 ` [PATCH 2/2] video: fbdev: convert get_user_pages() --> pin_user_pages() John Hubbard
@ 2020-05-31 20:58 ` Sam Ravnborg
  2020-05-31 21:06   ` John Hubbard
  2 siblings, 1 reply; 7+ messages in thread
From: Sam Ravnborg @ 2020-05-31 20:58 UTC (permalink / raw)
  To: John Hubbard
  Cc: linux-fbdev, Arnd Bergmann, Gustavo A . R . Silva, Jani Nikula,
	Daniel Vetter, Bartlomiej Zolnierkiewicz, LKML, dri-devel

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 <b.zolnierkie@samsung.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-fbdev@vger.kernel.org

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()
  2020-05-31 20:58 ` [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*() Sam Ravnborg
@ 2020-05-31 21:06   ` John Hubbard
  2020-05-31 21:11     ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: John Hubbard @ 2020-05-31 21:06 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: linux-fbdev, Arnd Bergmann, Gustavo A . R . Silva, Jani Nikula,
	Daniel Vetter, Bartlomiej Zolnierkiewicz, LKML, dri-devel

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()
  2020-05-31 21:06   ` John Hubbard
@ 2020-05-31 21:11     ` Andy Shevchenko
  2020-05-31 22:00       ` John Hubbard
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2020-05-31 21:11 UTC (permalink / raw)
  To: John Hubbard
  Cc: linux-fbdev, Arnd Bergmann, Gustavo A . R . Silva, Jani Nikula,
	Daniel Vetter, Bartlomiej Zolnierkiewicz, LKML, dri-devel,
	Sam Ravnborg

[-- Attachment #1.1: Type: text/plain, Size: 509 bytes --]

On Monday, June 1, 2020, John Hubbard <jhubbard@nvidia.com> wrote:

> 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.



JFYI, we have history.git starting from v0.01.


>
>
> thanks,
> --
> John Hubbard
> NVIDIA
>


-- 
With Best Regards,
Andy Shevchenko

[-- Attachment #1.2: Type: text/html, Size: 1033 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()
  2020-05-31 21:11     ` Andy Shevchenko
@ 2020-05-31 22:00       ` John Hubbard
  0 siblings, 0 replies; 7+ messages in thread
From: John Hubbard @ 2020-05-31 22:00 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-fbdev, Arnd Bergmann, Gustavo A . R . Silva, Jani Nikula,
	Daniel Vetter, Bartlomiej Zolnierkiewicz, LKML, dri-devel,
	Paul Mundt, Sam Ravnborg

On 2020-05-31 14:11, Andy Shevchenko wrote:
>     ...
> JFYI, we have history.git starting from v0.01.
> 
OK, thanks for that note. According to that history.git [1],
then: drivers/video/pvr2fb.c had get_user_pages_fast() support added to
pvr2fb_write() back in 2004, but only for CONFIG_SH_DMA, as part of

     commit 434502754f2 ("[PATCH] SH Merge")

...and that commit created the minor bug that patch 0001 here
addresses. (+Cc Paul just for the sake of completeness.)


[1] git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git


thanks,
-- 
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-22  4:15 [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*() John Hubbard
2020-05-22  4:15 ` [PATCH 1/2] video: fbdev: fix error handling for get_user_pages_fast() John Hubbard
2020-05-22  4:15 ` [PATCH 2/2] video: fbdev: convert get_user_pages() --> pin_user_pages() John Hubbard
2020-05-31 20:58 ` [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*() Sam Ravnborg
2020-05-31 21:06   ` John Hubbard
2020-05-31 21:11     ` Andy Shevchenko
2020-05-31 22:00       ` John Hubbard

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git