Re: [PATCH V7 1/2] xen/arm: Harden the P2M code in p2m_remove_mapping()

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 16.07.22 18:06, Julien Grall wrote:
> Hi Oleksandr,


Hello Julien



>
> In the future, please provide a cover letter (even if it is short) 
> when you bundle multiple patches. This is useful to make generic 
> comments and help threading in e-mail client (each patch would be a 
> subthread of 0 rather than 1).


Yes, will do.


>
> On 16/07/2022 15:56, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> Borrow the x86's check from p2m_remove_page() which was added
>> by the following commit: c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e
>> "x86/p2m: don't assert that the passed in MFN matches for a remove"
>> and adjust it to the Arm code base.
>>
>> Basically, this check will be strictly needed for the xenheap pages
>> after applying a subsequent commit which will introduce xenheap based
>> M2P approach on Arm. But, it will be a good opportunity to harden
>> the P2M code for *every* RAM pages since it is possible to remove
>> any GFN - MFN mapping currently on Arm (even with the wrong helpers).
>>
>> Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> ---
>> You can find the corresponding discussion at:
>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@xxxxxxx/__;!!GF_29dbcQIUBPA!3a2u-XL4NvAzSMfz72LARrdWVFvq2In5ZpUdxP2cSt7bM8PgV7P_ZclZG2R-rE9PcosUHyqsKRNfVG2TiM9Tlg$
>>  
>> [lore[.]kernel[.]org]
>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/1652294845-13980-2-git-send-email-olekstysh@xxxxxxxxx/__;!!GF_29dbcQIUBPA!3a2u-XL4NvAzSMfz72LARrdWVFvq2In5ZpUdxP2cSt7bM8PgV7P_ZclZG2R-rE9PcosUHyqsKRNfVG0kg7IZSA$
>>  
>> [lore[.]kernel[.]org]
>>
>> Changes V6 -> V7:
>>     - make this commit to be the first
>>     - update commit description and add a comment in code
>> ---
>>   xen/arch/arm/p2m.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d00c2e462a..2a0d383df4 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1308,11 +1308,39 @@ static inline int p2m_remove_mapping(struct 
>> domain *d,
>>                                        mfn_t mfn)
>>   {
>>       struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +    unsigned long i;
>>       int rc;
>>         p2m_write_lock(p2m);
>> +    /*
>> +     * Before removing the GFN - MFN mapping for any RAM pages make 
>> sure
>> +     * that there is no difference between what is already mapped 
>> and what
>> +     * is requested to be unmapped.
>> +     * If they don't match bail out early. For instance, this could 
>> happen
>> +     * if two CPUs are requesting to unmap the same P2M concurrently.
>
> Missing word: P2M *entry*

Yes. May I please ask, could this be done on the commit if this appears 
to be the last version?


>
> Other than that:
>
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>


Thank you!


>
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko