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

From: Oleksandr <olekstysh@gmail.com>
To: Julien Grall <julien@xen.org>, xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH V6 2/2] xen/arm: Harden the P2M code in p2m_remove_mapping()
Date: Fri, 15 Jul 2022 20:24:29 +0300	[thread overview]
Message-ID: <313ce039-31c2-1081-b044-d39c940e7565@gmail.com> (raw)
In-Reply-To: <f2e5a755-0f1e-ef18-21db-cbe6ef346886@xen.org>

On 15.07.22 20:15, Julien Grall wrote:
>
>
> On 24/06/2022 16:31, Oleksandr wrote:
>>
>> On 23.06.22 21:08, Julien Grall wrote:
>>> Hi Oleksandr,
>>
>>
>> Hello Julien
>
> Hi Oleksandr,

Hello Julien

>
>
>>
>>>
>>> On 11/05/2022 19:47, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> 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 is strictly needed for the xenheap pages only
>>>> since there are several non-protected read accesses to our simplified
>>>> xenheap based M2P approach on Arm (most calls to 
>>>> page_get_xenheap_gfn()
>>>> are not protected by the P2M lock).
>>>
>>> To me, this read as you introduced a bug in patch #1 and now you are 
>>> fixing it. So this patch should have been first.
>>
>>
>> Sounds like yes, I agree. But, in that case I propose to rewrite this 
>> text like the following:
>>
>>
>> Basically, this check will be strictly needed for the xenheap pages 
>> only *and* only after applying subsequent
>
> NIT: s/only and only/, this is pretty clear that this patch is 
> necessary for a follow-up patch.

ok

>
>
> Also please add "a" in from of subsequent because the two patches may 
> not be committed together.

ok

>
>> 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@amazon.com>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>> You can find the corresponding discussion at:
>>>> https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@xen.org/ 
>>>>
>>>>
>>>> Changes V5 -> V6:
>>>>   - new patch
>>>> ---
>>>>   xen/arch/arm/p2m.c | 21 +++++++++++++++++++++
>>>>   1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index f87b48e..635e474 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -1311,11 +1311,32 @@ 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);
>>>> +    for ( i = 0; i < nr; )
>>> One bit I really hate in the x86 code is the lack of in-code 
>>> documentation. It makes really difficult to understand the logic.
>>>
>>> I know this code was taken from x86, but I would like to avoid 
>>> making same mistake (this code is definitely not trivial). So can we 
>>> document the logic?
>>
>>
>> ok, I propose the following text right after acquiring the p2m lock:
>>
>>
>>   /*
>>    * 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 passed mapping doesn't match
>>    * the existing one bail out early.
>
> NIT: I would simply write "If they don't match bail out early".

ok, I guess this is related to the last sentence only.

>
> Also, it would be good to explanation how this could happen. Something 
> like:
>
> "For instance, this could happen if two CPUs are requesting to unmap 
> the same P2M concurrently."

agree

>
>
>>    */
>>
>>
>> Could you please clarify, do you agree with both?
>
> I have proposed some changes in both cases. I originally thought I 
> would do the update in the commit. However, this is more than simple 
> tweak, so would you mind to send a new version?

yes, will do

>
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko