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

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

>>> +	/*
>>> +	 * 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?
> 
> For writable path, a new vmf_insert_pfn_mkwrite() is needed, so it was
> not fixed. I can fix that part as well.

We should do that; returning kernel allocations from this function in ->fault is
bad.

How urgent is this fix here? (can it actually be triggered?Specific hardware?)

> 
> The read-only zero page part might need more attention, since
> we just prevented vmf_insert_pfn_prot() from using zero_page. Do we want
> to add the support for rdma_umap_fault()?

We just have to make sure that we cannot install it writable etc. But that's not
a concern.

Just have to be careful :)

> 
>>
>> (Does GUP have to work on these things?)
> 
> These are VM_PFNMAP mappings, I guess not. But I am not a GUP expert.

So we installed folios with ->fault in VM_PFNMAP!?!?!?!

That doesn't sound right :)

-- 
Cheers,

David