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

[Julien Grall <julien.grall@xxxxxxx>]

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. 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. 
While I agree this is not ideal, the vmap will still stay in-sync with the 
page-table.
Cheers,

--
Julien Grall

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