[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.