Re: [PATCH for 4.21 v5 3/3] xen/riscv: update mfn calculation in pt_mapping_level()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.02.2025 18:44, Oleksii Kurochko wrote:
> 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@xxxxxxxxx>
> ---
> Changes in v5:
> - Rename *entry to *ptep in pt_update_entry().
> - Fix code style issue in the comment.
> - Move NULL check of pointer to `table` inside unmap_table and then drop
>   it in pt_update_entry().

Imo this last aspect wants mentioning in the description.

> @@ -422,17 +439,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`.

There looks to be an "it" missing before the 2nd "is". I'm also unconvinced the
part starting with "which" is really necessary.

Preferably with these small adjustments (I'd be happy to do so while committing)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan