* [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path
@ 2020-09-01 21:21 Souptick Joarder
2020-09-01 21:30 ` John Hubbard
0 siblings, 1 reply; 6+ messages in thread
From: Souptick Joarder @ 2020-09-01 21:21 UTC (permalink / raw)
To: akpm, jhubbard
Cc: jgg, dan.j.williams, gregkh, timur, linux-kernel, dan.carpenter,
Souptick Joarder
First, when memory allocation for sg_list_unaligned failed, there
is a bug of calling put_pages() as we haven't pinned any pages.
Second, if get_user_pages_fast() failed we should unpin num_pinned
pages.
This will address both.
As part of these changes, minor update in documentation.
Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
management driver")
Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
---
v2:
Added review tag.
v3:
Address review comment on v2 from John.
Added review tag.
v4:
Address another set of review comments from John.
drivers/virt/fsl_hypervisor.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
index 1b0b11b..46ee0a0 100644
--- a/drivers/virt/fsl_hypervisor.c
+++ b/drivers/virt/fsl_hypervisor.c
@@ -157,7 +157,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
unsigned int i;
long ret = 0;
- int num_pinned; /* return value from get_user_pages() */
+ int num_pinned = 0; /* return value from get_user_pages_fast() */
phys_addr_t remote_paddr; /* The next address in the remote buffer */
uint32_t count; /* The number of bytes left to copy */
@@ -174,7 +174,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
return -EINVAL;
/*
- * The array of pages returned by get_user_pages() covers only
+ * The array of pages returned by get_user_pages_fast() covers only
* page-aligned memory. Since the user buffer is probably not
* page-aligned, we need to handle the discrepancy.
*
@@ -224,7 +224,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
/*
* 'pages' is an array of struct page pointers that's initialized by
- * get_user_pages().
+ * get_user_pages_fast().
*/
pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
if (!pages) {
@@ -241,7 +241,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
if (!sg_list_unaligned) {
pr_debug("fsl-hv: could not allocate S/G list\n");
ret = -ENOMEM;
- goto exit;
+ goto free_pages;
}
sg_list = PTR_ALIGN(sg_list_unaligned, sizeof(struct fh_sg_list));
@@ -250,7 +250,6 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
if (num_pinned != num_pages) {
- /* get_user_pages() failed */
pr_debug("fsl-hv: could not lock source buffer\n");
ret = (num_pinned < 0) ? num_pinned : -EFAULT;
goto exit;
@@ -292,13 +291,13 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
virt_to_phys(sg_list), num_pages);
exit:
- if (pages) {
- for (i = 0; i < num_pages; i++)
- if (pages[i])
- put_page(pages[i]);
+ if (pages && (num_pinned > 0)) {
+ for (i = 0; i < num_pinned; i++)
+ put_page(pages[i]);
}
kfree(sg_list_unaligned);
+free_pages:
kfree(pages);
if (!ret)
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path
2020-09-01 21:21 [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path Souptick Joarder
@ 2020-09-01 21:30 ` John Hubbard
2020-09-05 1:16 ` Souptick Joarder
0 siblings, 1 reply; 6+ messages in thread
From: John Hubbard @ 2020-09-01 21:30 UTC (permalink / raw)
To: Souptick Joarder, akpm
Cc: jgg, dan.j.williams, gregkh, timur, linux-kernel, dan.carpenter
On 9/1/20 2:21 PM, Souptick Joarder wrote:
> First, when memory allocation for sg_list_unaligned failed, there
> is a bug of calling put_pages() as we haven't pinned any pages.
>
> Second, if get_user_pages_fast() failed we should unpin num_pinned
> pages.
>
> This will address both.
>
> As part of these changes, minor update in documentation.
>
> Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
> management driver")
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> ---
This looks good to me.
thanks,
--
John Hubbard
NVIDIA
> v2:
> Added review tag.
>
> v3:
> Address review comment on v2 from John.
> Added review tag.
>
> v4:
> Address another set of review comments from John.
>
> drivers/virt/fsl_hypervisor.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
> index 1b0b11b..46ee0a0 100644
> --- a/drivers/virt/fsl_hypervisor.c
> +++ b/drivers/virt/fsl_hypervisor.c
> @@ -157,7 +157,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
>
> unsigned int i;
> long ret = 0;
> - int num_pinned; /* return value from get_user_pages() */
> + int num_pinned = 0; /* return value from get_user_pages_fast() */
> phys_addr_t remote_paddr; /* The next address in the remote buffer */
> uint32_t count; /* The number of bytes left to copy */
>
> @@ -174,7 +174,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> return -EINVAL;
>
> /*
> - * The array of pages returned by get_user_pages() covers only
> + * The array of pages returned by get_user_pages_fast() covers only
> * page-aligned memory. Since the user buffer is probably not
> * page-aligned, we need to handle the discrepancy.
> *
> @@ -224,7 +224,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
>
> /*
> * 'pages' is an array of struct page pointers that's initialized by
> - * get_user_pages().
> + * get_user_pages_fast().
> */
> pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
> if (!pages) {
> @@ -241,7 +241,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> if (!sg_list_unaligned) {
> pr_debug("fsl-hv: could not allocate S/G list\n");
> ret = -ENOMEM;
> - goto exit;
> + goto free_pages;
> }
> sg_list = PTR_ALIGN(sg_list_unaligned, sizeof(struct fh_sg_list));
>
> @@ -250,7 +250,6 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
>
> if (num_pinned != num_pages) {
> - /* get_user_pages() failed */
> pr_debug("fsl-hv: could not lock source buffer\n");
> ret = (num_pinned < 0) ? num_pinned : -EFAULT;
> goto exit;
> @@ -292,13 +291,13 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> virt_to_phys(sg_list), num_pages);
>
> exit:
> - if (pages) {
> - for (i = 0; i < num_pages; i++)
> - if (pages[i])
> - put_page(pages[i]);
> + if (pages && (num_pinned > 0)) {
> + for (i = 0; i < num_pinned; i++)
> + put_page(pages[i]);
> }
>
> kfree(sg_list_unaligned);
> +free_pages:
> kfree(pages);
>
> if (!ret)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path
2020-09-01 21:30 ` John Hubbard
@ 2020-09-05 1:16 ` Souptick Joarder
2020-09-05 1:21 ` John Hubbard
0 siblings, 1 reply; 6+ messages in thread
From: Souptick Joarder @ 2020-09-05 1:16 UTC (permalink / raw)
To: Andrew Morton
Cc: Jason Gunthorpe, Dan Williams, Greg KH, timur, linux-kernel,
Dan Carpenter, John Hubbard
Hi Andrew,
On Wed, Sep 2, 2020 at 3:00 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 9/1/20 2:21 PM, Souptick Joarder wrote:
> > First, when memory allocation for sg_list_unaligned failed, there
> > is a bug of calling put_pages() as we haven't pinned any pages.
> >
> > Second, if get_user_pages_fast() failed we should unpin num_pinned
> > pages.
> >
> > This will address both.
> >
> > As part of these changes, minor update in documentation.
> >
> > Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
> > management driver")
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> > ---
>
> This looks good to me.
Can you please take this patch through the mm tree ?
>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> > v2:
> > Added review tag.
> >
> > v3:
> > Address review comment on v2 from John.
> > Added review tag.
> >
> > v4:
> > Address another set of review comments from John.
> >
> > drivers/virt/fsl_hypervisor.c | 17 ++++++++---------
> > 1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
> > index 1b0b11b..46ee0a0 100644
> > --- a/drivers/virt/fsl_hypervisor.c
> > +++ b/drivers/virt/fsl_hypervisor.c
> > @@ -157,7 +157,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> >
> > unsigned int i;
> > long ret = 0;
> > - int num_pinned; /* return value from get_user_pages() */
> > + int num_pinned = 0; /* return value from get_user_pages_fast() */
> > phys_addr_t remote_paddr; /* The next address in the remote buffer */
> > uint32_t count; /* The number of bytes left to copy */
> >
> > @@ -174,7 +174,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> > return -EINVAL;
> >
> > /*
> > - * The array of pages returned by get_user_pages() covers only
> > + * The array of pages returned by get_user_pages_fast() covers only
> > * page-aligned memory. Since the user buffer is probably not
> > * page-aligned, we need to handle the discrepancy.
> > *
> > @@ -224,7 +224,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> >
> > /*
> > * 'pages' is an array of struct page pointers that's initialized by
> > - * get_user_pages().
> > + * get_user_pages_fast().
> > */
> > pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
> > if (!pages) {
> > @@ -241,7 +241,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> > if (!sg_list_unaligned) {
> > pr_debug("fsl-hv: could not allocate S/G list\n");
> > ret = -ENOMEM;
> > - goto exit;
> > + goto free_pages;
> > }
> > sg_list = PTR_ALIGN(sg_list_unaligned, sizeof(struct fh_sg_list));
> >
> > @@ -250,7 +250,6 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> > num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
> >
> > if (num_pinned != num_pages) {
> > - /* get_user_pages() failed */
> > pr_debug("fsl-hv: could not lock source buffer\n");
> > ret = (num_pinned < 0) ? num_pinned : -EFAULT;
> > goto exit;
> > @@ -292,13 +291,13 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
> > virt_to_phys(sg_list), num_pages);
> >
> > exit:
> > - if (pages) {
> > - for (i = 0; i < num_pages; i++)
> > - if (pages[i])
> > - put_page(pages[i]);
> > + if (pages && (num_pinned > 0)) {
> > + for (i = 0; i < num_pinned; i++)
> > + put_page(pages[i]);
> > }
> >
> > kfree(sg_list_unaligned);
> > +free_pages:
> > kfree(pages);
> >
> > if (!ret)
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path
2020-09-05 1:16 ` Souptick Joarder
@ 2020-09-05 1:21 ` John Hubbard
2020-09-06 2:17 ` Souptick Joarder
0 siblings, 1 reply; 6+ messages in thread
From: John Hubbard @ 2020-09-05 1:21 UTC (permalink / raw)
To: Souptick Joarder, Andrew Morton
Cc: Jason Gunthorpe, Dan Williams, Greg KH, timur, linux-kernel,
Dan Carpenter
On 9/4/20 6:16 PM, Souptick Joarder wrote:
> Hi Andrew,
>
> On Wed, Sep 2, 2020 at 3:00 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 9/1/20 2:21 PM, Souptick Joarder wrote:
>>> First, when memory allocation for sg_list_unaligned failed, there
>>> is a bug of calling put_pages() as we haven't pinned any pages.
>>>
>>> Second, if get_user_pages_fast() failed we should unpin num_pinned
>>> pages.
>>>
>>> This will address both.
>>>
>>> As part of these changes, minor update in documentation.
>>>
>>> Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
>>> management driver")
>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>>> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
>>> ---
>>
>> This looks good to me.
>
> Can you please take this patch through the mm tree ?
>
Is there a problem with sending it through it's normal tree? It would probably
get better testing coverage there.
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path
2020-09-05 1:21 ` John Hubbard
@ 2020-09-06 2:17 ` Souptick Joarder
2020-09-06 7:39 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Souptick Joarder @ 2020-09-06 2:17 UTC (permalink / raw)
To: John Hubbard
Cc: Andrew Morton, Jason Gunthorpe, Dan Williams, Greg KH, timur,
linux-kernel, Dan Carpenter
On Sat, Sep 5, 2020 at 6:51 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 9/4/20 6:16 PM, Souptick Joarder wrote:
> > Hi Andrew,
> >
> > On Wed, Sep 2, 2020 at 3:00 AM John Hubbard <jhubbard@nvidia.com> wrote:
> >>
> >> On 9/1/20 2:21 PM, Souptick Joarder wrote:
> >>> First, when memory allocation for sg_list_unaligned failed, there
> >>> is a bug of calling put_pages() as we haven't pinned any pages.
> >>>
> >>> Second, if get_user_pages_fast() failed we should unpin num_pinned
> >>> pages.
> >>>
> >>> This will address both.
> >>>
> >>> As part of these changes, minor update in documentation.
> >>>
> >>> Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
> >>> management driver")
> >>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> >>> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> >>> ---
> >>
> >> This looks good to me.
> >
> > Can you please take this patch through the mm tree ?
> >
>
> Is there a problem with sending it through it's normal tree? It would probably
> get better testing coverage there.
scripts/get_maintainer.pl is showing only general lkml list and few
commit signer.
I didn't receive any feedback from anyone except you & Dan in more
than 12+ weeks
and mail bounced back from actual author Timur Tabi.
As it is more than 12 weeks, I requested Andrew to take it through mm tree.
Note -
While running ./scripts/get_maintainer.pl, I observed one issue. Everytime I run
the script, list is getting changed. Is it an expected behaviour ?
Below is the details ->
jordon@jordon-HP-15-Notebook-PC:~/Documents/virt/linux-next$
./scripts/get_maintainer.pl
0001-drivers-virt-fsl_hypervisor-Fix-error-handling-path.patch
Bjorn Andersson <bjorn.andersson@linaro.org> (commit_signer:1/1=100%)
Daniel Vetter <daniel.vetter@ffwll.ch> (commit_signer:1/1=100%)
Dan Williams <dan.j.williams@intel.com> (commit_signer:1/1=100%)
"Darren Hart (VMware)" <dvhart@infradead.org> (commit_signer:1/1=100%)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:1/1=100%)
Arnd Bergmann <arnd@arndb.de>
(authored:1/1=100%,added_lines:1/1=100%,removed_lines:1/1=100%,blamed_fixes:1/1=100%)
Timur Tabi <timur@freescale.com> (blamed_fixes:1/1=100%)
Kumar Gala <galak@kernel.crashing.org> (blamed_fixes:1/1=100%)
linux-kernel@vger.kernel.org (open list)
jordon@jordon-HP-15-Notebook-PC:~/Documents/virt/linux-next$
./scripts/get_maintainer.pl
0001-drivers-virt-fsl_hypervisor-Fix-error-handling-path.patch
Bjorn Andersson <bjorn.andersson@linaro.org> (commit_signer:1/1=100%)
Daniel Vetter <daniel.vetter@ffwll.ch> (commit_signer:1/1=100%)
Dan Williams <dan.j.williams@intel.com> (commit_signer:1/1=100%)
"Darren Hart (VMware)" <dvhart@infradead.org> (commit_signer:1/1=100%)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:1/1=100%)
Arnd Bergmann <arnd@arndb.de>
(authored:1/1=100%,added_lines:1/1=100%,removed_lines:1/1=100%,blamed_fixes:1/1=100%)
Timur Tabi <timur@freescale.com> (blamed_fixes:1/1=100%)
Kumar Gala <galak@kernel.crashing.org> (blamed_fixes:1/1=100%)
linux-kernel@vger.kernel.org (open list)
jordon@jordon-HP-15-Notebook-PC:~/Documents/virt/linux-next$
./scripts/get_maintainer.pl
0001-drivers-virt-fsl_hypervisor-Fix-error-handling-path.patch
Bjorn Andersson <bjorn.andersson@linaro.org> (commit_signer:1/1=100%)
Daniel Vetter <daniel.vetter@ffwll.ch> (commit_signer:1/1=100%)
Dan Williams <dan.j.williams@intel.com> (commit_signer:1/1=100%)
"Darren Hart (VMware)" <dvhart@infradead.org> (commit_signer:1/1=100%)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:1/1=100%)
Arnd Bergmann <arnd@arndb.de>
(authored:1/1=100%,added_lines:1/1=100%,removed_lines:1/1=100%,blamed_fixes:1/1=100%)
Timur Tabi <timur@freescale.com> (blamed_fixes:1/1=100%)
Kumar Gala <galak@kernel.crashing.org> (blamed_fixes:1/1=100%)
linux-kernel@vger.kernel.org (open list)
>
>
> thanks,
> --
> John Hubbard
> NVIDIA
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path
2020-09-06 2:17 ` Souptick Joarder
@ 2020-09-06 7:39 ` Joe Perches
0 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2020-09-06 7:39 UTC (permalink / raw)
To: Souptick Joarder, John Hubbard
Cc: Andrew Morton, Jason Gunthorpe, Dan Williams, Greg KH, timur,
linux-kernel, Dan Carpenter
On Sun, 2020-09-06 at 07:47 +0530, Souptick Joarder wrote:
> While running ./scripts/get_maintainer.pl, I observed one issue. Everytime I run
> the script, list is getting changed. Is it an expected behaviour ?
Yes.
https://lkml.org/lkml/2017/7/13/789
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-06 7:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01 21:21 [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path Souptick Joarder
2020-09-01 21:30 ` John Hubbard
2020-09-05 1:16 ` Souptick Joarder
2020-09-05 1:21 ` John Hubbard
2020-09-06 2:17 ` Souptick Joarder
2020-09-06 7:39 ` Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).