[v2,1/3] xen/privcmd: Corrected error handling path
diff mbox series

Message ID 1594059372-15563-2-git-send-email-jrdr.linux@gmail.com
State Superseded
Headers show
Series
  • Few bug fixes and Convert to pin_user_pages*()
Related show

Commit Message

Souptick Joarder July 6, 2020, 6:16 p.m. UTC
Previously, if lock_pages() end up partially mapping pages, it used
to return -ERRNO due to which unlock_pages() have to go through
each pages[i] till *nr_pages* to validate them. This can be avoided
by passing correct number of partially mapped pages & -ERRNO separately,
while returning from lock_pages() due to error.

With this fix unlock_pages() doesn't need to validate pages[i] till
*nr_pages* for error scenario and few condition checks can be ignored.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Paul Durrant <xadimgnik@gmail.com>
---
 drivers/xen/privcmd.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Comments

Jürgen Groß July 7, 2020, 9:35 a.m. UTC | #1
On 06.07.20 20:16, Souptick Joarder wrote:
> Previously, if lock_pages() end up partially mapping pages, it used
> to return -ERRNO due to which unlock_pages() have to go through
> each pages[i] till *nr_pages* to validate them. This can be avoided
> by passing correct number of partially mapped pages & -ERRNO separately,
> while returning from lock_pages() due to error.
> 
> With this fix unlock_pages() doesn't need to validate pages[i] till
> *nr_pages* for error scenario and few condition checks can be ignored.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Paul Durrant <xadimgnik@gmail.com>
> ---
>   drivers/xen/privcmd.c | 31 +++++++++++++++----------------
>   1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index a250d11..33677ea 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -580,13 +580,13 @@ static long privcmd_ioctl_mmap_batch(
>   
>   static int lock_pages(
>   	struct privcmd_dm_op_buf kbufs[], unsigned int num,
> -	struct page *pages[], unsigned int nr_pages)
> +	struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
>   {
>   	unsigned int i;
> +	int page_count = 0;

Initial value shouldn't be needed, and ...

>   
>   	for (i = 0; i < num; i++) {
>   		unsigned int requested;
> -		int pinned;

... you could move the declaration here.

With that done you can add my

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen
Souptick Joarder July 7, 2020, 11:40 a.m. UTC | #2
On Tue, Jul 7, 2020 at 3:05 PM Jürgen Groß <jgross@suse.com> wrote:
>
> On 06.07.20 20:16, Souptick Joarder wrote:
> > Previously, if lock_pages() end up partially mapping pages, it used
> > to return -ERRNO due to which unlock_pages() have to go through
> > each pages[i] till *nr_pages* to validate them. This can be avoided
> > by passing correct number of partially mapped pages & -ERRNO separately,
> > while returning from lock_pages() due to error.
> >
> > With this fix unlock_pages() doesn't need to validate pages[i] till
> > *nr_pages* for error scenario and few condition checks can be ignored.
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Paul Durrant <xadimgnik@gmail.com>
> > ---
> >   drivers/xen/privcmd.c | 31 +++++++++++++++----------------
> >   1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index a250d11..33677ea 100644
> > --- a/drivers/xen/privcmd.c
> > +++ b/drivers/xen/privcmd.c
> > @@ -580,13 +580,13 @@ static long privcmd_ioctl_mmap_batch(
> >
> >   static int lock_pages(
> >       struct privcmd_dm_op_buf kbufs[], unsigned int num,
> > -     struct page *pages[], unsigned int nr_pages)
> > +     struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
> >   {
> >       unsigned int i;
> > +     int page_count = 0;
>
> Initial value shouldn't be needed, and ...
>
> >
> >       for (i = 0; i < num; i++) {
> >               unsigned int requested;
> > -             int pinned;
>
> ... you could move the declaration here.
>
> With that done you can add my
>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Ok. But does it going make any difference other than limiting scope ?

>
>
> Juergen
Jürgen Groß July 7, 2020, 11:45 a.m. UTC | #3
On 07.07.20 13:40, Souptick Joarder wrote:
> On Tue, Jul 7, 2020 at 3:05 PM Jürgen Groß <jgross@suse.com> wrote:
>>
>> On 06.07.20 20:16, Souptick Joarder wrote:
>>> Previously, if lock_pages() end up partially mapping pages, it used
>>> to return -ERRNO due to which unlock_pages() have to go through
>>> each pages[i] till *nr_pages* to validate them. This can be avoided
>>> by passing correct number of partially mapped pages & -ERRNO separately,
>>> while returning from lock_pages() due to error.
>>>
>>> With this fix unlock_pages() doesn't need to validate pages[i] till
>>> *nr_pages* for error scenario and few condition checks can be ignored.
>>>
>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Cc: Paul Durrant <xadimgnik@gmail.com>
>>> ---
>>>    drivers/xen/privcmd.c | 31 +++++++++++++++----------------
>>>    1 file changed, 15 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
>>> index a250d11..33677ea 100644
>>> --- a/drivers/xen/privcmd.c
>>> +++ b/drivers/xen/privcmd.c
>>> @@ -580,13 +580,13 @@ static long privcmd_ioctl_mmap_batch(
>>>
>>>    static int lock_pages(
>>>        struct privcmd_dm_op_buf kbufs[], unsigned int num,
>>> -     struct page *pages[], unsigned int nr_pages)
>>> +     struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
>>>    {
>>>        unsigned int i;
>>> +     int page_count = 0;
>>
>> Initial value shouldn't be needed, and ...
>>
>>>
>>>        for (i = 0; i < num; i++) {
>>>                unsigned int requested;
>>> -             int pinned;
>>
>> ... you could move the declaration here.
>>
>> With that done you can add my
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Ok. But does it going make any difference other than limiting scope ?

Dropping the initializer surely does, and in the end page_count just
replaces the former pinned variable, so why would we want to widen the
scope with this patch?


Juergen
Souptick Joarder July 8, 2020, 2:07 a.m. UTC | #4
On Tue, Jul 7, 2020 at 5:15 PM Jürgen Groß <jgross@suse.com> wrote:
>
> On 07.07.20 13:40, Souptick Joarder wrote:
> > On Tue, Jul 7, 2020 at 3:05 PM Jürgen Groß <jgross@suse.com> wrote:
> >>
> >> On 06.07.20 20:16, Souptick Joarder wrote:
> >>> Previously, if lock_pages() end up partially mapping pages, it used
> >>> to return -ERRNO due to which unlock_pages() have to go through
> >>> each pages[i] till *nr_pages* to validate them. This can be avoided
> >>> by passing correct number of partially mapped pages & -ERRNO separately,
> >>> while returning from lock_pages() due to error.
> >>>
> >>> With this fix unlock_pages() doesn't need to validate pages[i] till
> >>> *nr_pages* for error scenario and few condition checks can be ignored.
> >>>
> >>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> >>> Cc: John Hubbard <jhubbard@nvidia.com>
> >>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >>> Cc: Paul Durrant <xadimgnik@gmail.com>
> >>> ---
> >>>    drivers/xen/privcmd.c | 31 +++++++++++++++----------------
> >>>    1 file changed, 15 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> >>> index a250d11..33677ea 100644
> >>> --- a/drivers/xen/privcmd.c
> >>> +++ b/drivers/xen/privcmd.c
> >>> @@ -580,13 +580,13 @@ static long privcmd_ioctl_mmap_batch(
> >>>
> >>>    static int lock_pages(
> >>>        struct privcmd_dm_op_buf kbufs[], unsigned int num,
> >>> -     struct page *pages[], unsigned int nr_pages)
> >>> +     struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
> >>>    {
> >>>        unsigned int i;
> >>> +     int page_count = 0;
> >>
> >> Initial value shouldn't be needed, and ...
> >>
> >>>
> >>>        for (i = 0; i < num; i++) {
> >>>                unsigned int requested;
> >>> -             int pinned;
> >>
> >> ... you could move the declaration here.
> >>
> >> With that done you can add my
> >>
> >> Reviewed-by: Juergen Gross <jgross@suse.com>
> >
> > Ok. But does it going make any difference other than limiting scope ?
>
> Dropping the initializer surely does, and in the end page_count just
> replaces the former pinned variable, so why would we want to widen the
> scope with this patch?

Agree, no reason to move it up. Will change it in v3.

>
>
> Juergen

Patch
diff mbox series

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index a250d11..33677ea 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -580,13 +580,13 @@  static long privcmd_ioctl_mmap_batch(
 
 static int lock_pages(
 	struct privcmd_dm_op_buf kbufs[], unsigned int num,
-	struct page *pages[], unsigned int nr_pages)
+	struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
 {
 	unsigned int i;
+	int page_count = 0;
 
 	for (i = 0; i < num; i++) {
 		unsigned int requested;
-		int pinned;
 
 		requested = DIV_ROUND_UP(
 			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
@@ -594,14 +594,15 @@  static int lock_pages(
 		if (requested > nr_pages)
 			return -ENOSPC;
 
-		pinned = get_user_pages_fast(
+		page_count = get_user_pages_fast(
 			(unsigned long) kbufs[i].uptr,
 			requested, FOLL_WRITE, pages);
-		if (pinned < 0)
-			return pinned;
+		if (page_count < 0)
+			return page_count;
 
-		nr_pages -= pinned;
-		pages += pinned;
+		*pinned += page_count;
+		nr_pages -= page_count;
+		pages += page_count;
 	}
 
 	return 0;
@@ -611,13 +612,8 @@  static void unlock_pages(struct page *pages[], unsigned int nr_pages)
 {
 	unsigned int i;
 
-	if (!pages)
-		return;
-
-	for (i = 0; i < nr_pages; i++) {
-		if (pages[i])
-			put_page(pages[i]);
-	}
+	for (i = 0; i < nr_pages; i++)
+		put_page(pages[i]);
 }
 
 static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
@@ -630,6 +626,7 @@  static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 	struct xen_dm_op_buf *xbufs = NULL;
 	unsigned int i;
 	long rc;
+	unsigned int pinned = 0;
 
 	if (copy_from_user(&kdata, udata, sizeof(kdata)))
 		return -EFAULT;
@@ -683,9 +680,11 @@  static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 		goto out;
 	}
 
-	rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
-	if (rc)
+	rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
+	if (rc < 0) {
+		nr_pages = pinned;
 		goto out;
+	}
 
 	for (i = 0; i < kdata.num; i++) {
 		set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);