Re: [PATCH v1 1/2] KVM: arm64: Introduce KVM_PGTABLE_WALK_SKIP_LEVEL* walk flags

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Combining skip flags causes the walker to ignore the more restrictive skip levels due to fls() usage.
- [Medium] Skipping a page table level bypasses the KVM_PGTABLE_WALK_TABLE_POST callback, breaking PRE/POST callback symmetry.
- [Low] The skip logic cannot prevent the root level from being visited, making SKIP_LEVEL0 ineffective when start_level == 0.
--

commit 4e6415e54e22d985a0c97622b8e8b28b4c84b54b
Author: Leonardo Bras <leo.bras@arm.com>

KVM: arm64: Introduce KVM_PGTABLE_WALK_SKIP_LEVEL* walk flags

Add the new walking flags that tell kvm_pgtable_walk() to skip lower levels
when walking the pagetables.

> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 91a7dfad66866..48d88a290a538 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c

[ ... ]

> @@ -144,6 +144,19 @@ static bool kvm_pgtable_walk_continue(const struct kvm_pgtable_walker *walker,
>  	return !r;
>  }
>  
> +static __always_inline bool kvm_pgtable_skip_level(s8 level, enum kvm_pgtable_walk_flags flags)
> +{
> +	flags &= KVM_PGTABLE_WALK_SKIP_LEVELS;
> +
> +	if (likely(!flags))
> +		return false;
> +
> +	if (level >= (fls(flags) - ffs(KVM_PGTABLE_WALK_SKIP_LEVELS)))

[Severity: Medium]
Will this evaluate to the highest level if multiple skip flags are ORed
together?

If a caller combines flags like KVM_PGTABLE_WALK_SKIP_LEVEL1 and
KVM_PGTABLE_WALK_SKIP_LEVEL2, fls() will select the highest set bit. This
means the threshold evaluates to level 2, effectively ignoring the more
restrictive SKIP_LEVEL1 flag and incorrectly descending into levels the
caller requested to skip.

Should kvm_pgtable_skip_level() use ffs() instead of fls() to select the
lowest requested level as the definitive stopping point?

> +		return true;
> +
> +	return false;
> +}

[ ... ]

> @@ -192,7 +205,7 @@ static inline int __kvm_pgtable_visit(struct kvm_pgtable_walk_data *data,
>  	if (!kvm_pgtable_walk_continue(data->walker, ret))
>  		goto out;
>  
> -	if (!table) {
> +	if (!table || kvm_pgtable_skip_level(level + 1, ctx.flags)) {

[Severity: Medium]
Does skipping the level here bypass the KVM_PGTABLE_WALK_TABLE_POST callback?

If the current entry is a table, the KVM_PGTABLE_WALK_TABLE_PRE callback is
executed earlier in __kvm_pgtable_visit(). If kvm_pgtable_skip_level() then
evaluates to true, the code executes an early exit via goto out.

This completely bypasses the KVM_PGTABLE_WALK_TABLE_POST callback at the
end of the function, leaving the TABLE_PRE call unbalanced. If a walker
relies on symmetric PRE and POST callbacks to clean up resources or track
traversal depth, could this asymmetric execution flow lead to resource leaks
or state inconsistency?

[Severity: Low]
Does checking level + 1 prevent the root level from being skipped?

The walker unconditionally calls __kvm_pgtable_visit() for entries at the
root page table (start_level). Because the skip evaluation checks level + 1,
it only governs whether the walker descends into the children of the current
level.

If start_level == 0 and the walker requests SKIP_LEVEL0, won't the walker
still visit level 0 and invoke its callbacks, making SKIP_LEVEL0 functionally
identical to SKIP_LEVEL1 in this case?

>  		data->addr = ALIGN_DOWN(data->addr, kvm_granule_size(level));
>  		data->addr += kvm_granule_size(level);
>  		goto out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610202112.2695205-2-leo.bras@arm.com?part=1