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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a9b8cc50-6635-0551-596b-5a67a8261e59@oracle.com>
Date:   Thu, 25 Jun 2020 19:31:27 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Souptick Joarder <jrdr.linux@...il.com>, jgross@...e.com,
        sstabellini@...nel.org
Cc:     xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
        John Hubbard <jhubbard@...dia.com>,
        Paul Durrant <xadimgnik@...il.com>
Subject: Re: [PATCH 1/2] xen/privcmd: Corrected error handling path and mark
 pages dirty

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@...il.com>
> Cc: John Hubbard <jhubbard@...dia.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@...cle.com>
> Cc: Paul Durrant <xadimgnik@...il.com>
> ---
> 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