On 20.11.2025 16:38, Oleksii Kurochko wrote:
> On 11/18/25 7:58 AM, Jan Beulich wrote:
>> On 17.11.2025 20:51, Oleksii Kurochko wrote:
>>> On 11/12/25 12:49 PM, Jan Beulich wrote:
>>>> On 20.10.2025 17:58, Oleksii Kurochko wrote:
>>>>> + if ( *md_pg )
>>>>> + metadata = __map_domain_page(*md_pg);
>>>>> +
>>>>> + if ( t < p2m_first_external )
>>>>> + {
>>>>> pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
>>>>> - return rc;
>>>>> + if ( metadata )
>>>>> + metadata[ctx->index].pte = p2m_invalid;
>>>> Shouldn't this be accompanied with a BUILD_BUG_ON(p2m_invalid), as
>>>> otherwise
>>>> p2m_alloc_page()'s clearing of the page won't have the intended effect?
>>> I think that, at least, at the moment we are always explicitly set p2m type
>>> and
>>> do not rely on that by default 0==p2m_invalid.
>> You don't, and ...
>>
>>> Just to be safe, I will add after "if ( metadata )" suggested
>>> BUILD_BUG_ON(p2m_invalid):
>>> if ( metadata )
>>> metadata[ctx->index].type = p2m_invalid;
>>> /*
>>> * metadata.type is expected to be p2m_invalid (0) after the page
>>> is
>>> * allocated and zero-initialized in p2m_alloc_page().
>>> */
>>> BUILD_BUG_ON(p2m_invalid);
>>> ...
>> ... this leaves me with the impression that you didn't read my reply
>> correctly.
>> p2m_alloc_page() clear the page, thus _implicitly_ setting all entries to
>> p2m_invalid. That's where the BUILD_BUG_ON() would want to go (the call site,
>> ftaod).
>
> I think I still don’t fully understand what the issue would be
> if|p2m_invalid| were
> ever equal to 1 instead of 0 in the context of a metadata page.
>
> Yes, if|p2m_invalid| were 1, there would be a problem if someone tried to
> read this
> metadata pagebefore it was assigned any type. They would find a value of 0,
> which
> corresponds to a valid type rather than to|p2m_invalid|, as one might expect.
> However, I’m not sure I currently see a scenario in which the metadata page
> would
> be read before being initialized.
Are you sure walks can only happen for GFNs that were set up? What you need to
do walks on is under guest control, after all.
> But just to be safe when such case will occur I am okay with putting
> BUILD_BUG_ON(p2m_invalid) before p2m_alloc_page() in p2m_set_type() function.
Thanks.
>>>>> +static void p2m_free_page(struct p2m_domain *p2m, struct page_info *pg);
>>>>> +
>>>>> +/*
>>>>> + * Free page table's page and metadata page linked to page table's page.
>>>>> + */
>>>>> +static void p2m_free_table(struct p2m_domain *p2m, struct page_info
>>>>> *tbl_pg)
>>>>> +{
>>>>> + if ( tbl_pg->v.md.pg )
>>>>> + p2m_free_page(p2m, tbl_pg->v.md.pg);
>>>> To play safe, maybe better also clear tbl_pg->v.md.pg?
>>> I thought it would be enough to clear it during allocation in
>>> p2m_alloc_page(),
>>> since I'm not sure it is critical if md.pg data were somehow leaked and
>>> read.
>>> But to be safer, we can add this here:
>>> clear_and_clean_page(tbl_pg->v.md.pg, p2m->clean_dcache);
>> I didn't say clear what tbl_pg->v.md.pg points to, though. I suggested to
>> clear
>> the struct field itself.
>
> Won't be enough just tbl_pg->v.md.pg = NULL; ?
Exactly that, yes.
Jan
