[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH for 4.21 v5 3/3] xen/riscv: update mfn calculation in pt_mapping_level()
On 2/25/25 8:59 AM, Jan Beulich wrote:
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. Agree, considering that it is a change in the code of previously introduced function, it makes to mention that. @@ -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. Lets then just drop this part. I added only to explicitly show that it is the value which is used to define `level`. Preferably with these small adjustments (I'd be happy to do so while committing) Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> Thanks! ~ Oleksii
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |