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
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; }
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;
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
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,
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.
SamGreat! That will work out just fine.
JFYI, we have history.git starting from v0.01.
thanks,
John Hubbard NVIDIA
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,
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/
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,
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.
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,
dri-devel@lists.freedesktop.org
-
Andy Shevchenko -
John Hubbard -
Sam Ravnborg