Re: [PATCH v3] userfaultfd: provide unmasked address on page-fault

[Jan Kara <jack@suse.cz>]

On Sat 26-02-22 02:26:55, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> Userfaultfd is supposed to provide the full address (i.e., unmasked) of
> the faulting access back to userspace. However, that is not the case for
> quite some time.
> 
> Even running "userfaultfd_demo" from the userfaultfd man page provides
> the wrong output (and contradicts the man page). Notice that
> "UFFD_EVENT_PAGEFAULT event" shows the masked address (7fc5e30b3000)
> and not the first read address (0x7fc5e30b300f).
> 
> 	Address returned by mmap() = 0x7fc5e30b3000
> 
> 	fault_handler_thread():
> 	    poll() returns: nready = 1; POLLIN = 1; POLLERR = 0
> 	    UFFD_EVENT_PAGEFAULT event: flags = 0; address = 7fc5e30b3000
> 		(uffdio_copy.copy returned 4096)
> 	Read address 0x7fc5e30b300f in main(): A
> 	Read address 0x7fc5e30b340f in main(): A
> 	Read address 0x7fc5e30b380f in main(): A
> 	Read address 0x7fc5e30b3c0f in main(): A
> 
> The exact address is useful for various reasons and specifically for
> prefetching decisions. If it is known that the memory is populated by
> certain objects whose size is not page-aligned, then based on the
> faulting address, the uffd-monitor can decide whether to prefetch and
> prefault the adjacent page.
> 
> This bug has been for quite some time in the kernel: since commit
> 1a29d85eb0f1 ("mm: use vmf->address instead of of vmf->virtual_address")
> vmf->virtual_address"), which dates back to 2016. A concern has been
> raised that existing userspace application might rely on the old/wrong
> behavior in which the address is masked. Therefore, it was suggested to
> provide the masked address unless the user explicitly asks for the exact
> address.
> 
> Add a new userfaultfd feature UFFD_FEATURE_EXACT_ADDRESS to direct
> userfaultfd to provide the exact address. Add a new "real_address" field
> to vmf to hold the unmasked address. Provide the address to userspace
> accordingly.
> 
> Initialize real_address in various code-paths to be consistent with
> address, even when it is not used, to be on the safe side.
> 
> Acked-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Mike Rapoport <rppt@linux.vnet.ibm.com>
> Cc: Jan Kara <jack@suse.cz>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> 
> ---
> 
> v2->v3:
> * Initialize real_address on all code paths [Jan]
> 
> v1->v2:
> * Add uffd feature to selectively enable [David, Andrea]

I've just noticed one typo below. Otherwise the patch looks good to me.
Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>


> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 213cc569b192..27df0ca0a36a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -478,7 +478,8 @@ struct vm_fault {
>  		struct vm_area_struct *vma;	/* Target VMA */
>  		gfp_t gfp_mask;			/* gfp mask to be used for allocations */
>  		pgoff_t pgoff;			/* Logical page offset based on vma */
> -		unsigned long address;		/* Faulting virtual address */
> +		unsigned long address;		/* Faulting virtual address - masked */
> +		unsigned long real_address;	/* Faulting virtual address - unmaked */
										^^ typo here

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR