Re: [PATCH V5] xen/gnttab: Store frame GFN in struct page_info on Arm

[Oleksandr <olekstysh@gmail.com>]

On 10.02.22 11:46, Julien Grall wrote:
>
>
> On 08/02/2022 19:50, Oleksandr Tyshchenko wrote:
>>
>> On 08.02.22 13:58, Julien Grall wrote:
>>> Hi,
>>
>> Hi Julien
>
> Hi,


Hi Julien


Thank you for the clarifications!


>
>>>
>>>
>>> (Jan please confirm) If I am not mistaken, on x86, a read to the M2P
>>> is not always protected. But they have code within the P2M lock to
>>> check any difference (see p2m_remove_page()). I think we would need
>>> the same, so we don't end up to introduce a behavior similar to what
>>> XSA-387 has fixed on x86.
>>
>>
>> ... OK, I assume you are speaking about the check in the loop that was
>> added by the following commit:
>> c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e "x86/p2m: don't assert that the
>> passed in MFN matches for a remove"
>
> Yes, this is the one I Have in mind.


good


>
>> Also, I assume we need that check in the same place on Arm (with P2M
>> lock held), which, I think, could be p2m_remove_mapping().
>
> I believe so. 


good


> Can you do some testing to check this would not break other types of 
> mapping? (Booting a guest and using PV device should be enough).


Sure, already checked and will check more thoroughly before submitting.


>
>
>>
>> I ported the check from x86 code, but this is not a verbatim copy due to
>> the difference in local P2M helpers/macro between arches, also I have
>> skipped a part of that check "|| t == p2m_mmio_direct" which was added
>> by one of the follow-up commit:
>> 753cb68e653002e89fdcd1c80e52905fdbfb78cb "x86/p2m: guard (in particular)
>> identity mapping entries"
>> since I have no idea whether we need the same on Arm.
>
> I am not entirely sure. For now, I would drop it so long the behavior 
> stay the same (i.e. it will go ahead with removing the mappings).t.


ok, will drop.


>
>
>> Below the diff I have locally:
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 5646343..90d7563 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1315,11 +1315,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; )
>> +    {
>> +        unsigned int cur_order;
>> +        bool valid;
>> +        mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
>> NULL, NULL,
>> +                                         &cur_order, &valid); > +
>> +        if ( valid &&
>
> valid is a copy of the LPAE bit valid. This may be 0 if Xen decided to 
> clear it (i.e when emulating set/way). Yet the mapping itself is 
> considered valid from Xen PoV.
>
> So you want to replace with a different check (see below).


Hmm, I got it, so ...


>
>
>> +             (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), 
>> mfn_return)) )
>> +        {
>> +            rc = -EILSEQ;
>> +            goto out;
>> +        }
>> +
>> +        i += (1UL << cur_order) -
>> +             ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
>> +    }
>> +
>>        rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
>>                           p2m_invalid, p2m_access_rwx);
>> +
>> +out:
>>        p2m_write_unlock(p2m);
>>
>>        return rc;
>>
>>
>> Could you please clarify, is it close to what you had in mind? If yes, I
>> am wondering, don't we need this check to be only executed for xenheap
>> pages (and, probably, which P2M's entry type in RAM?) rather than for
>> all pages?
>
> From my understanding, for the purpose of this work, we only strictly 
> need to check that for xenheap pages.


  ... yes, but ...


>
>
> But I think it would be a good opportunity to harden the P2M code. At 
> the moment, on Arm, you can remove any mappings you want (even with 
> the wrong helpers). This lead us to a few issues when mapping were 
> overriden silently (in particular when building dom0).
> So I would say we should enforce it for every RAM mapping. 


... I think this makes sense, so the proper check in that case, I 
assume, should contain p2m_is_any_ram() macro:


diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5646343..2b82c49 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1315,11 +1315,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; )
+    {
+        unsigned int cur_order;
+        p2m_type_t t;
+        mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i), 
&t, NULL,
+                                         &cur_order, NULL);
+
+        if ( p2m_is_any_ram(t) &&
+             (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)) )
+        {
+            rc = -EILSEQ;
+            goto out;
+        }
+
+        i += (1UL << cur_order) -
+             ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
+    }
+
      rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
                         p2m_invalid, p2m_access_rwx);
+
+out:
      p2m_write_unlock(p2m);

      return rc;
(END)


> Stefano, Bertrand, what do you think?
>
> Note that, I would like to see this change in a separate commit. It 
> will be easier to review.


ok, I will introduce this check by a separate patch.


>
>
>>
>>
>>>
>>>
>>> In addition to that, if p2m_get_xenheap_gfn() is going to be called
>>> locklessly. Then we need to make sure the update to type_info are
>>> atomic. This means:
>>>   - p2m_get_xenheap_gfn() should use READ_ONCE().
>>>   - p2m_set_xenheap_gfn() should use WRITE_ONCE(). We might even need
>>> to use cmpxchg() if there are other update to type_info that are not
>>> protected. I will let you have a look.
>>
>>
>> ... OK, I didn't find READ_ONCE/WRITE_ONCE in Xen. I am wondering, can
>> we use ACCESS_ONCE instead?
>
> Yes. Sorry, I keep forgetting we don't have READ_ONCE/WRITE_ONCE in Xen.


ok


>
>>
>> Below the diff I have locally:
>>
>> diff --git a/xen/arch/arm/include/asm/mm.h 
>> b/xen/arch/arm/include/asm/mm.h
>> index 9e093a6..b18acb7 100644
>> --- a/xen/arch/arm/include/asm/mm.h
>> +++ b/xen/arch/arm/include/asm/mm.h
>> @@ -373,7 +373,7 @@ unsigned int arch_get_dma_bitsize(void);
>>
>>    static inline gfn_t page_get_xenheap_gfn(const struct page_info *p)
>>    {
>> -    gfn_t gfn_ = _gfn(p->u.inuse.type_info & PGT_gfn_mask);
>> +    gfn_t gfn_ = _gfn(ACCESS_ONCE(p->u.inuse.type_info) & 
>> PGT_gfn_mask);
>>
>>        ASSERT(is_xen_heap_page(p));
>>
>> @@ -383,11 +383,14 @@ static inline gfn_t page_get_xenheap_gfn(const
>> struct page_info *p)
>>    static inline void page_set_xenheap_gfn(struct page_info *p, gfn_t 
>> gfn)
>>    {
>>        gfn_t gfn_ = gfn_eq(gfn, INVALID_GFN) ? 
>> PGT_INVALID_XENHEAP_GFN : gfn;
>> +    unsigned long type_info;
>>
>>        ASSERT(is_xen_heap_page(p));
>>
>> -    p->u.inuse.type_info &= ~PGT_gfn_mask;
>> -    p->u.inuse.type_info |= gfn_x(gfn_);
>> +    type_info = ACCESS_ONCE(p->u.inuse.type_info);
>> +    type_info &= ~PGT_gfn_mask;
>> +    type_info |= gfn_x(gfn_);
>> +    ACCESS_ONCE(p->u.inuse.type_info) = type_info;
>>    }
>>
>>    #endif /*  __ARCH_ARM_MM__ */
>>
>>
>> It is going to be a non-protected write to GFN portion of type_info.
>
> Well no. You are using a Read-Modify-Write operation on type_info. 
> This is not atomic and will overwrite any change (if any) done on 
> other part of the type_info.


I am confused a bit, to which my comment your comment above belongs (to 
the diff for page_set_xenheap_gfn() above or to sentence right after it)?
The "It is going to be a non-protected write to GFN portion of 
type_info." sentence is related to the diff for alloc_heap_pages() 
below. Sorry if I didn't separate the comments properly.


>
>
> If I am mistaken, there are two other places where type_info is 
> modified. One is...
>
>
>> But, at that time the page is not used yet, so I think this is harmless.
>>
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 50334a0..97cf0d8 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1024,7 +1024,7 @@ static struct page_info *alloc_heap_pages(
>>                                     &tlbflush_timestamp);
>>
>>             /* Initialise fields which have other uses for free 
>> pages. */
>> -        pg[i].u.inuse.type_info = 0;
>> +        pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
>>             page_set_owner(&pg[i], NULL);
>>
>>         }
>
> ... this one. I agree the page is not accessible at this time. So 
> page_set_xenheap_gfn() should not be used.


yes


>
>
> The other one is in share_xen_page_with_guest() which I think is still 
> fine because the caller page_set_xenheap_gfn() would need to acquire a 
> reference on the page. This is only possible after the count_info is 
> updated in share_xen_page_with_guest() *and* there a barrier between 
> the type_info and count_info.
>
> I think this behavior should be documented on top of type_info (along 
> with the locking). This would be helpful if type_info gain more use in 
> the future.


agree, will do.


>
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko