Re: [PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 6/24/20 11:02 PM, 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.
>
> As discussed, pages need to be marked as dirty before unpinned it in
> unlock_pages() which was oversight.


There are two unrelated changes here (improving error path and marking
pages dirty), they should be handled by separate patches.


(I assume marking pages dirty is something you want to go to stable tree
since otherwise there is no reason for making this change)


>
> Signed-off-by: Souptick Joarder <jrdr.linux@xxxxxxxxx>
> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Cc: Paul Durrant <xadimgnik@xxxxxxxxx>
> ---
> Hi,
>
> I'm compile tested this,


I don't think so.


>  but unable to run-time test, so any testing
> help is much appriciated.
>
>  drivers/xen/privcmd.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index a250d11..0da417c 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -580,43 +580,44 @@ 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, int *pinned)
>  {
>       unsigned int i;
> +     int errno = 0, page_count = 0;


No need for error, really --- you can return the value immediately.


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


I'd move this lower, after a successful call to get_user_pages_fast()
(and then you won't need to initialize it)


>               requested = DIV_ROUND_UP(
>                       offset_in_page(kbufs[i].uptr) + kbufs[i].size,
>                       PAGE_SIZE);
>               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) {
> +                     errno = page_count;
> +                     return errno;
> +             }
>  
> -             nr_pages -= pinned;
> -             pages += pinned;
> +             nr_pages -= page_count;
> +             pages += page_count;
>       }
>  
> -     return 0;
> +     return errno;
>  }
>  
>  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]);
> +             if (!PageDirty(page))
> +                     set_page_dirty_lock(page);
> +             put_page(pages[i]);
>       }


This won't compile.


-boris