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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 22/07/2019 13:11, Jan Beulich wrote:
> 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.
Hmmm, I misread the read. vm_index() will indeed return 0 in that case. But, 
this will not silently leak the mappings thanks to the WARN_ON() in the code.
However, even if there are a leak, the vmap and page-table will stay in sync. So 
this is not as bad as the unaligned case.
Cheers,

--
Julien Grall

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