Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 22/09/2021 15:47, Jan Beulich wrote:
> On 22.09.2021 12:28, Julien Grall wrote:
> > Hi,
> >
> > On 22/09/2021 14:42, Jan Beulich wrote:
> > > On 22.09.2021 11:26, Roger Pau Monné wrote:
> > > > On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
> > > > > On 21.09.2021 10:32, Roger Pau Monné wrote:
> > > > > > On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
> > > > > > > On 20.09.2021 12:20, Roger Pau Monné wrote:
> > > > > > > > On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
> > > > > > > > > --- a/xen/include/asm-arm/grant_table.h
> > > > > > > > > +++ b/xen/include/asm-arm/grant_table.h
> > > > > > > > > +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
> > > > > > > > I'm slightly confused by this checks, don't you need to check for
> > > > > > > > gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
> > > > > > > > guest_physmap_remove_page?
> > > > > > > Why? It's ogfn which gets passed to the function. And it indeed is the
> > > > > > > prior GFN's mapping that we want to remove here.
> > > > > > >
> > > > > > > > Or assuming that ogfn is not invalid can be used to imply a removal?
> > > > > > > That implication can be (and on x86 is) used for the incoming argument,
> > > > > > > i.e. "gfn". I don't think "ogfn" can serve this purpose.
> > > > > > I guess I'm confused due to the ogfn checks done on the Arm side that
> > > > > > are not performed on x86. So on Arm you always need to explicitly
> > > > > > unhook the previous GFN before attempting to setup a new mapping,
> > > > > > while on x86 you only need to do this when it's a removal in order to
> > > > > > clear the entry?
> > > > > The difference isn't with guest_physmap_add_entry() (both x86 and
> > > > > Arm only insert a new mapping there), but with
> > > > > xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
> > > > > mappings. And gnttab_map_frame() gets called only from there. This
> > > > > is effectively what the first paragraph of the description is about.
> > > > OK, sorry, it wasn't clear to me from the description. Could you
> > > > explicitly mention in the description that the removal is moved into
> > > > gnttab_set_frame_gfn on Arm in order to cope with the fact that
> > > > xenmem_add_to_physmap_one doesn't perform it.
> > > Well, it's not really "in order to cope" - that's true for the placement
> > > prior to this change as well, so not a justification for the change.
> > > Nevertheless I've tried to make this more clear by changing the 1st
> > > paragraph to:
> > >
> > > "Without holding appropriate locks, attempting to remove a prior mapping
> > >    of the underlying page is pointless, as the same (or another) mapping
> > >    could be re-established by a parallel request on another vCPU. Move the
> > >    code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
> > >    xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
> > >    course this new placement doesn't improve things in any way as far as
> > >    the security of grant status frame mappings goes (see XSA-379). Proper
> > >    locking would be needed here to allow status frames to be mapped
> > >    securely."
> > >
> > > > TBH I think it would be in our best interest to try to make
> > > > xenmem_add_to_physmap_one behave as close as possible between arches.
> > > > This discrepancy between x86 and Arm regarding page removal is just
> > > > going to bring more trouble in the long term, and hiding the
> > > > differences inside gnttab_set_frame_gfn just makes it even more
> > > > obscure.
> > > Stefano, Julien?
> > This would be ideal as I don't particular like the approach taken in
> > this patch. But AFAICT, this would require us to implement an M2P. Or is
> > there another way to do it?
> For the purpose of just XENMAPSPACE_grant_table the "restricted" M2P
> (to just grant table pages), as introduced by an on-list patch, would
> do afaict. That wouldn't remove all the differences between the two
> implementations, but at least the one affecting the difference in how
> gnttab_map_frame() needs to behave.
This would work. I will add it in my todo list but I am not such when I 
will have time to look at it. I am happy if someone else wants to look 
at it.
Cheers,

--
Julien Grall