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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> --- 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.

> 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)? 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).

Jan