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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 26/11/2021 08:07, Jan Beulich wrote:
> On 25.11.2021 17:37, Julien Grall wrote:
> > 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
> > Do you mean gnttab_get_frame_gfn()?
> No, I don't think so; I do mean gnttab_set_frame_gfn(). Its return value
> directs the caller to do (or not) certain things.
Hmmm ok. I misundertood that comment then. Thanks for the clarification!

> > > --- a/xen/include/asm-arm/grant_table.h
> > > +++ b/xen/include/asm-arm/grant_table.h
> > > @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned
> > >            XFREE((gt)->arch.status_gfn);                                    \
> > >        } while ( 0 )
> > >    
> > > -#define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
> > > -    do {                                                                 \
> > > -        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
> > > -            (gfn);                                                       \
> > > -    } while ( 0 )
> > > +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
> > > +    ({                                                                   \
> > > +        int rc_ = 0;                                                     \
> > > +        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
> > > +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
> > > +             (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn,   \
> > > +                                              0)) == 0 )                 \
> > > +            ((st) ? (gt)->arch.status_gfn                                \
> > > +                  : (gt)->arch.shared_gfn)[idx] = (gfn);                 \
> > > +        rc_;                                                             \
> > > +    })
> > I think using a function would make it a bit easier to understand what
> > it does.
> I can convert it, but it's not very likely that it would be possible
> to make it an inline one. Not sure whether that's then still desirable.
So looking at Oleksandr series, I think we should be able to have the 
Arm helper matching the x86 one. At which point, I could deal with this 
version for now.
> > However... The naming of the function is now quite confusing. The more
> > on x86...
> >
> > >    
> > >    #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
> > >       (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
> > > --- a/xen/include/asm-x86/grant_table.h
> > > +++ b/xen/include/asm-x86/grant_table.h
> > > @@ -37,7 +37,12 @@ static inline int replace_grant_host_map
> > >    
> > >    #define gnttab_init_arch(gt) 0
> > >    #define gnttab_destroy_arch(gt) do {} while ( 0 )
> > > -#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
> > > +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
> > > +    (gfn_eq(gfn, INVALID_GFN)                                            \
> > > +     ? guest_physmap_remove_page((gt)->domain,                           \
> > > +                                 gnttab_get_frame_gfn(gt, st, idx),      \
> > > +                                 mfn, 0)                                 \
> > > +     : 0 /* Handled in add_to_physmap_one(). */)
> > ... it will end up to remove the mapping. I don't have a better name at
> > the moment. However I think this would deserve some documentation in the
> > code so one can understand how to implement it for another arch.
> Hmm, perhaps. But wouldn't we better document the final behavior (i.e.
> once the remaining Arm issue got addressed)?
Fair point. I don't expect Oleksandr's series to be committed long after 
yours.
> That may then also lead
> to overall simpler code, ideally with more suitable names (if the
> present ones are deemed unsuitable, which I'm not convinced of).
I don't have a better name so far. However, without any documentation 
this is very difficult to figure out what it is meant to do (I am not 
only referring to someone implementing it for another arch here).
Cheers,

--
Julien Grall