[Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update)

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 11/06/2019 19:37, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> Currently, the MFN will be incremented even if it is invalid. This will
>> result to have a valid MFN after the first iteration.
>>
>> While this is not a major problem today, this will be in the future if
>> the code expect an invalid MFN at every iteration.
>>
>> Such behavior is prevented by avoiding to increment an invalid MFN.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>> Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
>>
>> ---
>>      Changes in v2:
>>          - Move the patch earlier on in the series
>>          - Add Andrii's reviewed-by
>> ---
>>   xen/arch/arm/mm.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index f956aa6399..9de2a1150f 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation op,
>>   
>>       spin_lock(&xen_pt_lock);
>>   
>> -    for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>> +    for( ; addr < addr_end; addr += PAGE_SIZE )
>>       {
>>           rc = xen_pt_update_entry(op, addr, mfn, flags);
>>           if ( rc )
>>               break;
>> +
>> +        if ( !mfn_eq(mfn, INVALID_MFN) )
>> +            mfn = mfn_add(mfn, 1);
>>       }
> 
> This is OK but got me thinking: should we be updating the mfn in mfn_add
> if the mfn is INVALID? The mfn_eq(mfn, INVALID_MFN) could live inside
> the static inline mfn_t mfn_add function. What do you think? I don't
> think there are any valid scenarios where we want to increment
> INVALID_MFN...

My first thought is mfn_add(...) may be used in place where we know the 
mfn is not INVALID_MFN. So we would add extra check when it may not be 
necessary. Although, I am not sure if it is important.

I have added Andrew & Jan to get any opinions.

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel