[PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()

[PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()

[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

[PATCH 1/2] video: fbdev: fix error handling for get_user_pages_fast()

[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

[PATCH 2/2] video: fbdev: convert get_user_pages() --> pin_user_pages()

[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

Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()

[Sam Ravnborg]

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

Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()

[John Hubbard]

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

Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()

[Andy Shevchenko]

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

Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()

[John Hubbard]

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

Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()

[Andy Shevchenko]

On Mon, Jun 1, 2020 at 1:00 AM John Hubbard <jhubbard@nvidia.com> wrote:
> 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

I mentioned this one, but I guess content should be the same.

https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
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*()

[John Hubbard]

On 2020-06-01 03:35, Andy Shevchenko wrote:
> On Mon, Jun 1, 2020 at 1:00 AM John Hubbard <jhubbard@nvidia.com> wrote:
>> 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
> 
> I mentioned this one, but I guess content should be the same.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/
> 

Actually, that history.git *starts* at Linux 2.6.12-rc2, while
tglx/history.git *ends* at Linux 2.6.12-rc2 (which is in April, 2005).
And the commit I was looking for is in 2004. So that's why I needed a
different stretch of history.



thanks,
-- 
John Hubbard
NVIDIA
_______________________________________________
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*()

[Andy Shevchenko]

On Mon, Jun 1, 2020 at 8:10 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 2020-06-01 03:35, Andy Shevchenko wrote:
> > On Mon, Jun 1, 2020 at 1:00 AM John Hubbard <jhubbard@nvidia.com> wrote:
> >> 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
> >
> > I mentioned this one, but I guess content should be the same.
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/
> >
>
> Actually, that history.git *starts* at Linux 2.6.12-rc2,

It's not true.

> while
> tglx/history.git *ends* at Linux 2.6.12-rc2 (which is in April, 2005).
> And the commit I was looking for is in 2004. So that's why I needed a
> different stretch of history.

Actually history/history.git contains all of them starting from v0.01.
But it ends, indeed, on 2.6.33.

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
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*()

[John Hubbard]

On 2020-06-01 10:25, Andy Shevchenko wrote:
> On Mon, Jun 1, 2020 at 8:10 PM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 2020-06-01 03:35, Andy Shevchenko wrote:
>>> On Mon, Jun 1, 2020 at 1:00 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>>> 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
>>>
>>> I mentioned this one, but I guess content should be the same.
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/
>>>
>>
>> Actually, that history.git *starts* at Linux 2.6.12-rc2,
> 
> It's not true.

OK I see, neither a straight "git log" nor git branches will suffice, you
have to use tags in order to get to the older versions.

> 
>> while
>> tglx/history.git *ends* at Linux 2.6.12-rc2 (which is in April, 2005).
>> And the commit I was looking for is in 2004. So that's why I needed a
>> different stretch of history.
> 
> Actually history/history.git contains all of them starting from v0.01.
> But it ends, indeed, on 2.6.33.
> 

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