Re: [GIT PULL v1 18/23] KVM: s390: cmma: Fix dirty tracking when removing memslot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mixing of `goto`-based error handling with scope-based cleanups (`__free()` and `guard()`).
--

--- Patch [18]: [GIT PULL v1 18/23] KVM: s390: cmma: Fix dirty tracking when removing memslot ---
commit b4b4bb1a465e2a5c28c8cf71b60ac80548c5f396
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: cmma: Fix dirty tracking when removing memslot

This patch fixes CMMA dirty tracking when a memslot is removed during
migration mode. It also opportunistically updates the memory region commit
path to use the __free() cleanup helper for the MMU cache.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index e5c5e9f61cb24..ba811f0673d10 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
>  void kvm_arch_commit_memory_region(struct kvm *kvm,
>  				struct kvm_memory_slot *old,
>  				const struct kvm_memory_slot *new,
>  				enum kvm_mr_change change)
>  {
> -	struct kvm_s390_mmu_cache *mc = NULL;
> +	const struct dat_walk_ops ops = { .pte_entry = cmma_d_count_pte, };
> +	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
>  	int rc = 0;
>  
> +	guard(mutex)(&kvm->slots_arch_lock);

[Severity: Medium]
Does this code mix goto-based error handling with scope-based cleanups?

According to the guidelines in include/linux/cleanup.h, functions should
convert all resources that need a goto cleanup to scope-based cleanup,
or convert none of them. This function introduces guard() and __free()
declarations here, but it still retains the `out:` label and the `goto out;`
error path when kvm_s390_new_mmu_cache() fails.

Also, is it possible to combine the __free() declaration and initialization of
mc at the point of allocation? Declaring it at the top as = NULL and
assigning it later makes LIFO ordering mistakes more likely. Could the goto
path be replaced with a direct return, and the mc variable be defined where
it is first allocated?

[ ... ]
> @@ -5851,7 +5873,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  out:
>  	if (rc)
>  		pr_warn("failed to commit memory region\n");
> -	kvm_s390_free_mmu_cache(mc);
>  	return;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803160924.236807-1-imbrenda@linux.ibm.com?part=18