* [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; 11+ 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 related [flat|nested] 11+ 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; 11+ 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 related [flat|nested] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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
2020-06-01 10:35 ` Andy Shevchenko
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()
2020-05-31 22:00 ` John Hubbard
@ 2020-06-01 10:35 ` Andy Shevchenko
2020-06-01 17:10 ` John Hubbard
0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-06-01 10:35 UTC (permalink / raw)
To: John Hubbard
Cc: linux-fbdev, Arnd Bergmann, Gustavo A . R . Silva, Jani Nikula,
Daniel Vetter, Bartlomiej Zolnierkiewicz, LKML, dri-devel,
Paul Mundt, Sam Ravnborg
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()
2020-06-01 10:35 ` Andy Shevchenko
@ 2020-06-01 17:10 ` John Hubbard
2020-06-01 17:25 ` Andy Shevchenko
0 siblings, 1 reply; 11+ messages in thread
From: John Hubbard @ 2020-06-01 17:10 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-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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()
2020-06-01 17:10 ` John Hubbard
@ 2020-06-01 17:25 ` Andy Shevchenko
2020-06-01 17:42 ` John Hubbard
0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-06-01 17:25 UTC (permalink / raw)
To: John Hubbard
Cc: linux-fbdev, Arnd Bergmann, Gustavo A . R . Silva, Jani Nikula,
Daniel Vetter, Bartlomiej Zolnierkiewicz, LKML, dri-devel,
Paul Mundt, Sam Ravnborg
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] video: fbdev: fix error handling, convert to pin_user_pages*()
2020-06-01 17:25 ` Andy Shevchenko
@ 2020-06-01 17:42 ` John Hubbard
0 siblings, 0 replies; 11+ messages in thread
From: John Hubbard @ 2020-06-01 17:42 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-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
^ permalink raw reply [flat|nested] 11+ messages in thread