Re: [PATCH] RDMA/uverbs: Don't return the shared zero page from rdma_umap_fault()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/26/26 03:04, Zi Yan wrote:
> After disassociation, rdma_umap_fault() returns the shared zero page from
> ->fault for VMAs without VM_WRITE and VM_MAYWRITE. Such VMAs come from
> rdma_user_mmap_io() in drivers that clear VM_MAYWRITE and they are shared
> VM_PFNMAP. A page returned from ->fault is mapped as a normal page and goes
> through rmap code. Two issues:
> 
> 1. shared zero page causes VM_WARN_ONs in rmap code,
> 
> 2. normal pages in shared VM_PFNMAP VMAs cause accounting leak on arch
>    without pte_special(), namely the page refcount, mapcount, and vm stats
>    are never dropped.
> 
> Map ufile->disassociate_page for VMAs without VM_WRITE and VM_MAYWRITE,
> with vmf_insert_pfn(). The page lives until ib_uverbs_release_file(), which
> cannot run while a VMA holds a reference to the file.
> 
> Writable VMAs keep returning disassociate_page in vmf->page, thus its
> accounting leak on arch without pte_special() needs a separate fix.
> 
> Userspace can see two differences after the patch. A mapping without
> VM_WRITE and VM_MAYWRITE now reads what was written through writable
> mappings of the same uverbs file instead of zeroes; after disassociation
> both are dummy data. The first fault now allocates the page and fails with
> VM_FAULT_SIGBUS if the allocation fails, as a write fault already does.
> 
> Fixes: 67f269b37f9b ("RDMA/ucontext: Fix regression with disassociate")
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
> The issue was discovered during PG_folio development when making sure only
> rmappable folios can enter rmap code path.
> 
> Alternative: map the zero page with vmf_insert_pfn_prot()
> ===
> 
> Read-only mappings could keep reading zeroes if rdma_umap_fault()
> installed the zero page itself as a special PTE:
> 
> return vmf_insert_pfn_prot(vmf->vma, vmf->address, zero_pfn(vmf->address));
> 
> It does not work today. Commit 7b466529307be ("mm: disallow raw PFN
> mappings of huge/shared zeropage") makes vmf_insert_pfn_prot() return
> VM_FAULT_SIGBUS for the zero pfn. vmf_insert_mixed() does not support
> VM_PFNMAP, since vm_mixed_zeropage_allowed() warns on VM_PFNMAP.
> 
> The first version of that patch [1] did not change vmf_insert_pfn_prot().
> Due to no in-kernel user at that time and complication it can cause,
> vmf_insert_pfn_prot() rejects zero pages now.
> 
> Fix to writable VMA path
> ===
> A new vmf_insert_pfn_mkwrite() is needed, since vmf_insert_pfn() has a
> narrow failing scenario. When a process mapping the disassociated page has
> soft-dirty tracking enabled, vm_page_prot becomes read-only;
> vmf_insert_pfn() then uses a read-only PTE instead of a writable one;
> fixup_user_fault() is used to write to the page (e.g., KVM with the uverbs
> mapping as guest memory) and fails, because it does not take the second
> fault to upgrade the read-only PTE to a writable one.
> 
> LLM was used for investigation and code development.
> 
> [1] https://lore.kernel.org/all/20260917121010.60966-1-lance.yang@linux.dev/
> ---
>  drivers/infiniband/core/uverbs_main.c | 44 ++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 2a046c888dfab..c3638bf031b15 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -725,39 +725,51 @@ static void rdma_umap_close(struct vm_area_struct *vma)
>  
>  /*
>   * Once the zap_special_vma_range has been called touches to the VMA will come here and
> - * we return a dummy writable zero page for all the pfns.
> + * we map a dummy page for all the pfns.
>   */
>  static vm_fault_t rdma_umap_fault(struct vm_fault *vmf)
>  {
>  	struct ib_uverbs_file *ufile = vmf->vma->vm_file->private_data;
>  	struct rdma_umap_priv *priv = vmf->vma->vm_private_data;
>  	vm_fault_t ret = 0;
> +	struct page *page;
>  
>  	if (!priv)
>  		return VM_FAULT_SIGBUS;
>  
> -	/* Read only pages can just use the system zero page. */
> -	if (!(vmf->vma->vm_flags & (VM_WRITE | VM_MAYWRITE))) {
> -		vmf->page = ZERO_PAGE(vmf->address);
> -		get_page(vmf->page);
> -		return 0;
> -	}
> -
>  	mutex_lock(&ufile->umap_lock);
>  	if (!ufile->disassociate_page)
>  		ufile->disassociate_page =
>  			alloc_pages(vmf->gfp_mask | __GFP_ZERO, 0);
>  
> -	if (ufile->disassociate_page) {
> -		/*
> -		 * This VMA is forced to always be shared so this doesn't have
> -		 * to worry about COW.
> -		 */
> -		vmf->page = ufile->disassociate_page;
> -		get_page(vmf->page);
> -	} else {
> +	page = ufile->disassociate_page;
> +	if (!page) {
>  		ret = VM_FAULT_SIGBUS;
> +		goto out;
> +	}
> +
> +	/* Make sure the page is usable, like what __do_fault() does */
> +	if (PageHWPoison(page)) {
> +		ret = VM_FAULT_HWPOISON;
> +		goto out;
>  	}

Is that really required? memory_failure() complains pretty hard on pages it
doesn't know how to handle (random kernel memory).

I'd assume that this here is not an ordinary pagecache page that mamoery_failure
would know how to handle?

> +
> +	/*
> +	 * VMAs that cannot become writable map the page as a raw pfn, like
> +	 * the device memory it replaces. The page lives as long as the file.
> +	 */
> +	if (!(vmf->vma->vm_flags & (VM_WRITE | VM_MAYWRITE))) {
> +		ret = vmf_insert_pfn(vmf->vma, vmf->address, page_to_pfn(page));
> +		goto out;
> +	}
> +
> +	/*
> +	 * This VMA is forced to always be shared so this doesn't have
> +	 * to worry about COW.
> +	 */
> +	vmf->page = page;
> +	get_page(page);


This split handling is pretty horrible TBH.

In once case we take a reference in the other not?

Why not perform a PFN mapping in any case?


(Does GUP have to work on these things?)




-- 
Cheers,

David