When pt_update() is called with arguments (..., INVALID_MFN, ..., 0 or 1),
it indicates that a mapping is being destroyed/modifyed.
In the case when modifying or destroying a mapping, it is necessary to
search until a leaf node is found, instead of searching for a page table
entry based on the precalculated `level` and `order`(look at pt_update()).
This is because when `mfn` == INVALID_MFN, the `mask` (in pt_mapping_level())
will take into account only `vfn`, which could accidentally return an
incorrect level, leading to the discovery of an incorrect page table entry.
For example, if `vfn` is page table level 1 aligned, but it was mapped as
page table level 0, then pt_mapping_level() will return `level` = 1, since
only `vfn` (which is page table level 1 aligned) is taken into account when
`mfn` == INVALID_MFN (look at pt_mapping_level()).
Fixes: c2f1ded524 ("xen/riscv: page table handling")
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v3:
- Drop ASSERT() for order as it isn't needed anymore.
- Drop PTE_LEAF_SEARCH and use instead level=CONFIG_PAGING_LEVELS;
refactor connected code correspondingly.
- Calculate order once.
- Drop initializer for local variable order.
- Drop BUG_ON(!pte_is_mapping(*entry)) for the case when leaf searching
happens as there is a similar check in pt_check_entry(). Look at
pt.c:41 and pt.c:75.
---
Changes in v2:
- Introduce PTE_LEAF_SEARCH to tell page table update operation to
walk down to wherever the leaf entry is.
- Use introduced PTE_LEAF_SEARCH to not searching pte_t entry twice.
- Update the commit message.
---
xen/arch/riscv/pt.c | 90 +++++++++++++++++++++++++++++----------------
1 file changed, 59 insertions(+), 31 deletions(-)
diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c
index 66cb976b55..8c15a48f60 100644
--- a/xen/arch/riscv/pt.c
+++ b/xen/arch/riscv/pt.c
@@ -238,11 +238,10 @@ pte_t pt_walk(vaddr_t va, unsigned int *pte_level)
/* Update an entry at the level @target. */
static int pt_update_entry(mfn_t root, vaddr_t virt,
- mfn_t mfn, unsigned int target,
+ mfn_t mfn, unsigned int *target,
unsigned int flags)
{
int rc;
- unsigned int level = HYP_PT_ROOT_LEVEL;
pte_t *table;
/*
* The intermediate page table shouldn't be allocated when MFN isn't
@@ -256,39 +255,45 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
bool alloc_tbl = !mfn_eq(mfn, INVALID_MFN) || (flags & PTE_POPULATE);
pte_t pte, *entry;
- /* convenience aliases */
- DECLARE_OFFSETS(offsets, virt);
-
- table = map_table(root);
- for ( ; level > target; level-- )
+ if ( *target == CONFIG_PAGING_LEVELS )
+ entry = _pt_walk(virt, target);
+ else
{
- rc = pt_next_level(alloc_tbl, &table, offsets[level]);
- if ( rc == XEN_TABLE_MAP_NOMEM )
+ unsigned int level = HYP_PT_ROOT_LEVEL;
+ /* convenience aliases */
+ DECLARE_OFFSETS(offsets, virt);
+
+ table = map_table(root);
+ for ( ; level > *target; level-- )
{
- rc = -ENOMEM;
- goto out;
+ rc = pt_next_level(alloc_tbl, &table, offsets[level]);
+ if ( rc == XEN_TABLE_MAP_NOMEM )
+ {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ if ( rc == XEN_TABLE_MAP_NONE )
+ {
+ rc = 0;
+ goto out;
+ }
+
+ if ( rc != XEN_TABLE_NORMAL )
+ break;
}
- if ( rc == XEN_TABLE_MAP_NONE )
+ if ( level != *target )
{
- rc = 0;
+ dprintk(XENLOG_ERR,
+ "%s: Shattering superpage is not supported\n", __func__);
+ rc = -EOPNOTSUPP;
goto out;
}
- if ( rc != XEN_TABLE_NORMAL )
- break;
- }
-
- if ( level != target )
- {
- dprintk(XENLOG_ERR,
- "%s: Shattering superpage is not supported\n", __func__);
- rc = -EOPNOTSUPP;
- goto out;
+ entry = table + offsets[level];
}
- entry = table + offsets[level];
-
rc = -EINVAL;
if ( !pt_check_entry(*entry, mfn, flags) )
goto out;
@@ -413,17 +418,40 @@ static int pt_update(vaddr_t virt, mfn_t mfn,
while ( left )
{
- unsigned int order, level;
-
- level = pt_mapping_level(vfn, mfn, left, flags);
- order = XEN_PT_LEVEL_ORDER(level);
+ unsigned int order, level = CONFIG_PAGING_LEVELS;
- ASSERT(left >= BIT(order, UL));
+ /*
+ * In the case when modifying or destroying a mapping, it is necessary
+ * to search until a leaf node is found, instead of searching for
+ * a page table entry based on the precalculated `level` and `order`
+ * (look at pt_update()).
+ * This is because when `mfn` == INVALID_MFN, the `mask`(in
+ * pt_mapping_level()) will take into account only `vfn`, which could
+ * accidentally return an incorrect level, leading to the discovery of
+ * an incorrect page table entry.
+ *
+ * For example, if `vfn` is page table level 1 aligned, but it was
+ * mapped as page table level 0, then pt_mapping_level() will return
+ * `level` = 1, since only `vfn` (which is page table level 1 aligned)
+ * is taken into account when `mfn` == INVALID_MFN
+ * (look at pt_mapping_level()).
+ *
+ * To force searching until a leaf node is found is necessary to have
+ * `level` == CONFIG_PAGING_LEVELS which is a default value for
+ * `level`.
+ *
+ * For other cases (when a mapping is not being modified or destroyed),
+ * pt_mapping_level() should be used.
+ */
+ if ( !mfn_eq(mfn, INVALID_MFN) || (flags & PTE_POPULATE) )
+ level = pt_mapping_level(vfn, mfn, left, flags);
- rc = pt_update_entry(root, vfn << PAGE_SHIFT, mfn, level, flags);
+ rc = pt_update_entry(root, vfn << PAGE_SHIFT, mfn, &level, flags);
if ( rc )
break;
+ order = XEN_PT_LEVEL_ORDER(level);
+
vfn += 1UL << order;
if ( !mfn_eq(mfn, INVALID_MFN) )
mfn = mfn_add(mfn, 1UL << order);
--
2.48.1On 07.02.2025 14:13, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/pt.c
> +++ b/xen/arch/riscv/pt.c
> @@ -238,11 +238,10 @@ pte_t pt_walk(vaddr_t va, unsigned int *pte_level)
>
> /* Update an entry at the level @target. */
> static int pt_update_entry(mfn_t root, vaddr_t virt,
> - mfn_t mfn, unsigned int target,
> + mfn_t mfn, unsigned int *target,
> unsigned int flags)
> {
> int rc;
> - unsigned int level = HYP_PT_ROOT_LEVEL;
> pte_t *table;
Considering the lack of an initializer here, ...
> @@ -256,39 +255,45 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
> bool alloc_tbl = !mfn_eq(mfn, INVALID_MFN) || (flags & PTE_POPULATE);
> pte_t pte, *entry;
>
> - /* convenience aliases */
> - DECLARE_OFFSETS(offsets, virt);
> -
> - table = map_table(root);
> - for ( ; level > target; level-- )
> + if ( *target == CONFIG_PAGING_LEVELS )
> + entry = _pt_walk(virt, target);
> + else
> {
> - rc = pt_next_level(alloc_tbl, &table, offsets[level]);
> - if ( rc == XEN_TABLE_MAP_NOMEM )
> + unsigned int level = HYP_PT_ROOT_LEVEL;
> + /* convenience aliases */
> + DECLARE_OFFSETS(offsets, virt);
> +
> + table = map_table(root);
> + for ( ; level > *target; level-- )
> {
> - rc = -ENOMEM;
> - goto out;
> + rc = pt_next_level(alloc_tbl, &table, offsets[level]);
> + if ( rc == XEN_TABLE_MAP_NOMEM )
> + {
> + rc = -ENOMEM;
> + goto out;
> + }
> +
> + if ( rc == XEN_TABLE_MAP_NONE )
> + {
> + rc = 0;
> + goto out;
> + }
> +
> + if ( rc != XEN_TABLE_NORMAL )
> + break;
> }
>
> - if ( rc == XEN_TABLE_MAP_NONE )
> + if ( level != *target )
> {
> - rc = 0;
> + dprintk(XENLOG_ERR,
> + "%s: Shattering superpage is not supported\n", __func__);
> + rc = -EOPNOTSUPP;
> goto out;
> }
>
> - if ( rc != XEN_TABLE_NORMAL )
> - break;
> - }
> -
> - if ( level != target )
> - {
> - dprintk(XENLOG_ERR,
> - "%s: Shattering superpage is not supported\n", __func__);
> - rc = -EOPNOTSUPP;
> - goto out;
> + entry = table + offsets[level];
> }
>
> - entry = table + offsets[level];
> -
> rc = -EINVAL;
> if ( !pt_check_entry(*entry, mfn, flags) )
> goto out;
... I'm surprised the compiler doesn't complain about use of a possibly
uninitialized variable at
out:
unmap_table(table);
For the new path you're adding the variable is uninitialized afaict.
Which implies that you're again failing to unmap what _pt_walk() has
handed you.
Jan
On 2/10/25 5:53 PM, Jan Beulich wrote:
> On 07.02.2025 14:13, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/pt.c
>> +++ b/xen/arch/riscv/pt.c
>> @@ -238,11 +238,10 @@ pte_t pt_walk(vaddr_t va, unsigned int *pte_level)
>>
>> /* Update an entry at the level @target. */
>> static int pt_update_entry(mfn_t root, vaddr_t virt,
>> - mfn_t mfn, unsigned int target,
>> + mfn_t mfn, unsigned int *target,
>> unsigned int flags)
>> {
>> int rc;
>> - unsigned int level = HYP_PT_ROOT_LEVEL;
>> pte_t *table;
> Considering the lack of an initializer here, ...
>
>> @@ -256,39 +255,45 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
>> bool alloc_tbl = !mfn_eq(mfn, INVALID_MFN) || (flags & PTE_POPULATE);
>> pte_t pte, *entry;
>>
>> - /* convenience aliases */
>> - DECLARE_OFFSETS(offsets, virt);
>> -
>> - table = map_table(root);
>> - for ( ; level > target; level-- )
>> + if ( *target == CONFIG_PAGING_LEVELS )
>> + entry = _pt_walk(virt, target);
>> + else
>> {
>> - rc = pt_next_level(alloc_tbl, &table, offsets[level]);
>> - if ( rc == XEN_TABLE_MAP_NOMEM )
>> + unsigned int level = HYP_PT_ROOT_LEVEL;
>> + /* convenience aliases */
>> + DECLARE_OFFSETS(offsets, virt);
>> +
>> + table = map_table(root);
>> + for ( ; level > *target; level-- )
>> {
>> - rc = -ENOMEM;
>> - goto out;
>> + rc = pt_next_level(alloc_tbl, &table, offsets[level]);
>> + if ( rc == XEN_TABLE_MAP_NOMEM )
>> + {
>> + rc = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + if ( rc == XEN_TABLE_MAP_NONE )
>> + {
>> + rc = 0;
>> + goto out;
>> + }
>> +
>> + if ( rc != XEN_TABLE_NORMAL )
>> + break;
>> }
>>
>> - if ( rc == XEN_TABLE_MAP_NONE )
>> + if ( level != *target )
>> {
>> - rc = 0;
>> + dprintk(XENLOG_ERR,
>> + "%s: Shattering superpage is not supported\n", __func__);
>> + rc = -EOPNOTSUPP;
>> goto out;
>> }
>>
>> - if ( rc != XEN_TABLE_NORMAL )
>> - break;
>> - }
>> -
>> - if ( level != target )
>> - {
>> - dprintk(XENLOG_ERR,
>> - "%s: Shattering superpage is not supported\n", __func__);
>> - rc = -EOPNOTSUPP;
>> - goto out;
>> + entry = table + offsets[level];
>> }
>>
>> - entry = table + offsets[level];
>> -
>> rc = -EINVAL;
>> if ( !pt_check_entry(*entry, mfn, flags) )
>> goto out;
> ... I'm surprised the compiler doesn't complain about use of a possibly
> uninitialized variable at
>
> out:
> unmap_table(table);
>
> For the new path you're adding the variable is uninitialized afaict.
> Which implies that you're again failing to unmap what _pt_walk() has
> handed you.
Thanks, unmapping of table and entry (in the case of the new patch) should be
really updated. I'll take care of it in the next patch version.
~ Oleksii
© 2016 - 2025 Red Hat, Inc.