Re: [Xen-devel] [PATCH] vunmap: let vunmap align virtual address by itself

[Jan Beulich <JBeulich@xxxxxxxx>]

On 22.07.2019 14:02, Julien Grall wrote:
> On 22/07/2019 11:23, Jan Beulich wrote:
>> On 22.07.2019 11:30, Andrii Anisov wrote:
>>>
>>>
>>> On 19.07.19 12:37, Jan Beulich wrote:
>>>> On 18.07.2019 19:11, Andrii Anisov wrote:
>>>>> Let vunmap align passed virtual address by PAGE_SIZE.
>>>>
>>>> Despite seeing Andrew's R-b you've already got I'd like to point out
>>>> that this increases the risk of some code passing the wrong pointer
>>>> into here. {,un}map_domain_page() act on single pages only, so there's
>>>> no ambiguity. When talking about multi-page regions though, not undoing
>>>> arithmetic previously done may mean actually pointing at other than the
>>>> first page of the mapping.
>>>
>>> Well, what we are moving into vunmap(), is a page alignment only. It can't 
>>> save us from the wrong pointer, even if it is done outside the vunmap().
>>>
>>>>> With the main change, also:
>>>>>     - strip all existing vunmap() calls from prior masking
>>>>
>>>> _If_ we are to go this route, then unmap_domain_page_global()
>>>> callers should also be adjusted. There, as for unmap_domain_page(),
>>>> I agree it would make sense to be more tolerant.
>>>
>>> I've found two page alignments prior to `unmap_domain_page_global()` call. 
>>> Should I wipe them in this patch, or in separate?
>>
>> If we're to go the suggested route then it might not matter much.
>> As I'm not entirely certain yet that we will, making this a prereq
>> one dealing with the alignment in unmap_domain_page_global() might
>> be better. Your existing patch would then further shift this into
>> vunmap().
> 
> I think allowing vunmap to deal with unaligned address could simplify some of 
> the callers. Passing the wrong page-aligned pointer is less critical than 
> passing an unaligned address.
> 
> The latter may result in misbehaving code.

Why only the latter?

> The vmap will end-up to not be sync with the page-table as we assume
> page-table update cannot fail (this probably should be changed). On
> Arm, this will result to any new vmap function to likely fail (we
> don't allow replace an existing valid page-table entry).
> 
> IIUC the code, the former will result to only unmapping some part of
> the vmap. 

AIUI the unmap request will simply fail: vm_index() and hence vm_size()
will simply return 0. Hence, with vunmap() not itself returning any
error, there'll be a silent leak of mappings.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel