Re: [PATCH v2 1/4] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Sequential calls to the KVM_S390_ZPCIOP_REG_AEN ioctl on the same device overwrite kzdev->user_account and kzdev->mm_account without dropping previous references, causing a permanent leak of mm_struct references and pinned pages.
--

--- Patch [1]: [PATCH v2 1/4] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages ---
commit cbbebe9ad7b7a6af2ee4a68dd67b2e520770bd20
Author: Farhan Ali <alifm@linux.ibm.com>
KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

This patch addresses a bug where unaccount_mem() could be called from a
different process context than the one that initially pinned the pages. It
resolves this by storing the pinning process's user_struct and mm_struct
when accounting for pinned pages, so they can be properly freed later.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 720bb58cabe2e..dd17f8a7b4734 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
[ ... ]
> @@ -206,17 +220,24 @@ static inline int account_mem(struct kvm_zdev *kzdev, unsigned long nr_pages)
>  	do {
>  		new_pages = cur_pages + nr_pages;
> -		if (new_pages > page_limit)
> -			return -ENOMEM;
> +		if (new_pages > page_limit) {
> +			rc =  -ENOMEM;
> +			goto out;
> +		}
>  	} while (!atomic_long_try_cmpxchg(&user->locked_vm, &cur_pages, new_pages));
>  
> +	mmgrab(current->mm);
>  	atomic64_add(nr_pages, &current->mm->pinned_vm);
> +	kzdev->user_account = user;
> +	kzdev->mm_account = current->mm;

[Severity: High]
If kvm_s390_pci_aif_enable() is called multiple times for the same device,
will this overwrite kzdev->user_account and kzdev->mm_account and leak
the previously acquired references?

It looks like kvm_s390_pci_aif_enable() doesn't check if the device is
already enabled before proceeding. If a userspace process calls the
KVM_S390_ZPCIOP_REG_AEN ioctl twice on the same device, the first call
will successfully populate these pointers. 

The second call would then overwrite them here, leaking the original
mm_struct reference and permanently stranding the initially pinned pages.
If that second call subsequently fails, unaccount_mem() drops the new
reference and sets the pointers to NULL, leaving the original references
permanently orphaned.

Should there be a check in kvm_s390_pci_aif_enable() to bail out if it
is already enabled, or a check here to ensure we aren't overwriting an
existing reference?

>  
>  	return 0;
> +
> +out:
> +	free_uid(user);
> +	return rc;
>  }
>  
>  static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,

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