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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 02.12.21 11:09, Julien Grall wrote:

Hi Julien, Jan

> Hi Jan,
>
> On 13/09/2021 07:41, Jan Beulich wrote:
> > 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(). 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.
> >
> > In turn this then requires replacing the other use in
> > gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
> > x86's gnttab_set_frame_gfn(). Note that with proper locking inside
> > guest_physmap_remove_page() combined with checking the GFN's mapping
> > there against the passed in MFN, there then is no issue with the
> > involved multiple gnttab_set_frame_gfn()-s potentially returning varying
> > values (due to a racing XENMAPSPACE_grant_table request).
> >
> > This, as a side effect, does away with gnttab_map_frame() having a local
> > variable "gfn" which shadows a function parameter of the same name.
> >
> > Together with XSA-379 this points out that XSA-255's addition to
> > gnttab_map_frame() was really useless.
> >
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> As discussed in the thread, I expect the Arm part to be simplified 
> after Oleksandr's series.

I assume, current patch is going to be committed soon(?), so I am in the 
process of preparing a rebased version of my patch as both touch Arm's 
gnttab_set_frame_gfn at least (I did a rebase sometime, but I lost these 
changes somehow).
Anyway, according to the recent suggestion how to eliminate the possible 
lock inversion introduced by my patch and taking into the account 
changes in current patch, the Arm's gnttab_set_frame_gfn, I think, needs 
to be updated to the following form:


#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
({ \
        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
        (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn, gfn))               \
         ? guest_physmap_remove_page((gt)->domain, ogfn, mfn, 0)         \
         : 0;                                                            \
    })


> So for the Arm part:
>
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
>
> Cheers,
--
Regards,

Oleksandr Tyshchenko