Re: [PATCH v2 08/12] KVM: arm64: CCA: Add Realm RTT mapping and invalidation helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] KVM: arm64: wrong granule level when skipping missing RTT in
  realm_fold_rtt_level()
- [Medium] KVM: arm64: cca: unhandled GFP_ATOMIC failure in
  realm_create_rtt_levels()
--

commit 68d993c0f3cb9846fd5972acfd789cb5ebe22bac
Author: Steven Price <steven.price@arm.com>

KVM: arm64: CCA: Add Realm RTT mapping and invalidation helpers

This commit adds helpers to create, fold, and unmap Realm Translation
Tables (RTTs) for shared and private IPA ranges.

> diff --git a/arch/arm64/kvm/rmi.c b/arch/arm64/kvm/rmi.c
> index b904e6541fb07..5aa34d68de595 100644
> --- a/arch/arm64/kvm/rmi.c
> +++ b/arch/arm64/kvm/rmi.c

[ ... ]

> @@ -156,12 +286,94 @@ static int realm_tear_down_rtt_range(struct realm *realm,
>  					 start, end);
>  }
>  
> +/*
> + * Returns 0 on successful fold, a negative value on error, a positive value if
> + * we were not able to fold all tables at this level.
> + */
> +static int realm_fold_rtt_level(struct realm *realm, int level,
> +				unsigned long start, unsigned long end)
> +{
> +	int not_folded = 0;
> +	ssize_t map_size;
> +	unsigned long addr, next_addr;
> +
> +	if (WARN_ON(level > KVM_PGTABLE_LAST_LEVEL))
> +		return -EINVAL;
> +
> +	map_size = kvm_granule_size(level - 1);
> +
> +	for (addr = start; addr < end; addr = next_addr) {
> +		phys_addr_t rtt_granule;
> +		long ret;
> +
> +		next_addr = ALIGN(addr + 1, map_size);
> +
> +		ret = realm_rtt_fold(realm, addr, level, &rtt_granule);
> +		if (ret < 0)
> +			return ret;
> +
> +		switch (RMI_RESULT_STATUS(ret)) {
> +		case RMI_SUCCESS:
> +			free_rtt(rtt_granule);
> +			break;
> +		case RMI_ERROR_RTT: {
> +			int err_level = RMI_RESULT_DATA_LEVEL(ret);
> +
> +			if (err_level < level) {
> +				/* No RTT at this level; skip the missing ancestor. */
> +				if (WARN_ON(err_level < get_start_level(realm)))
> +					return -EINVAL;
> +				next_addr = ALIGN(addr + 1,
> +						  kvm_granule_size(err_level - 1));
> +				break;
> +			}

[Severity: Medium]
Should this advance next_addr by kvm_granule_size(err_level) instead of
kvm_granule_size(err_level - 1) (matching realm_unmap_shared_range() and
realm_unmap_private_range())?

When realm_rtt_fold() fails at err_level < level, the walk stopped at a
single non-table entry in the table at err_level, which spans
kvm_granule_size(err_level).  Using err_level - 1 skips the span of the
entire table at err_level, leaving any foldable sibling RTTs in the rest of
that table unfolded, and evaluates kvm_granule_size(-1) when err_level is 0
on 4-level stage-2 tables (shifting past 64 bits on 16KB and 64KB pages).

[ ... ]

> @@ -191,6 +403,154 @@ int kvm_realm_teardown_stage2(struct kvm *kvm)
>  	return 0;
>  }
>  
> +static void realm_unmap_shared_range(struct kvm *kvm,
> +				     unsigned long start,
> +				     unsigned long end,
> +				     bool may_block)
> +{

[ ... ]

> +		case RMI_ERROR_RTT: {
> +			int err_level = RMI_RESULT_DATA_LEVEL(ret);
> +			int level = find_map_level(realm, addr, end);
> +
> +			if (WARN_ON(err_level > KVM_PGTABLE_LAST_LEVEL))
> +				return;
> +
> +			if (err_level >= level) {
> +				/* Nothing present, so skip */
> +				next_addr = ALIGN(addr + 1,
> +						  kvm_granule_size(err_level));
> +				break;
> +			}
> +
> +			ret = realm_create_rtt_levels(realm, addr, err_level,
> +						      level, NULL);
> +			if (WARN_ON(ret))
> +				return;

[ ... ]

> +static void realm_unmap_private_range(struct kvm *kvm,
> +				      unsigned long start,
> +				      unsigned long end,
> +				      bool may_block)
> +{

[ ... ]

> +		case RMI_ERROR_RTT: {
> +			int err_level = RMI_RESULT_DATA_LEVEL(ret);
> +			int level = find_map_level(realm, addr, end);
> +
> +			if (WARN_ON(err_level > KVM_PGTABLE_LAST_LEVEL))
> +				return;
> +
> +			if (err_level >= level) {
> +				/* Nothing present, so skip this RTT entry. */
> +				next_addr = ALIGN(addr + 1,
> +						  kvm_granule_size(err_level));
> +				break;
> +			}
> +
> +			ret = realm_create_rtt_levels(realm, addr, err_level,
> +						      level, NULL);
> +			if (WARN_ON(ret))
> +				return;

[Severity: Medium]
Can realm_create_rtt_levels() fail with -ENOMEM here, triggering WARN_ON()
and aborting the unmap?

Passing mc = NULL causes alloc_delegated_granule() to allocate RTT pages
with GFP_ATOMIC | __GFP_ZERO | __GFP_ACCOUNT.  When unmapping a sub-block
range under memory pressure or a memcg limit, an allocation failure fires
WARN_ON(ret) and returns early, leaving the rest of [addr, end) mapped in
the Realm RTTs (for example across an MMU notifier invalidation before the
host frees the backing pages).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111145758.217946-1-steven.price@arm.com?part=8