Re: [PATCH v6 1/6] KVM: s390: pci: Reject adapter interrupt forwarding if already enabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Memory leak of `user_struct` and incorrect memory quota accounting.
--

--- Patch [1]: [PATCH v6 1/6] KVM: s390: pci: Reject adapter interrupt forwarding if already enabled ---
commit e0b697cb30b22f286c181b6e14ea16a36c89254c
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Reject adapter interrupt forwarding if already enabled

The MPCIFC instruction does not allow registering adapter interrupts without
first unregistering. This patch rejects any request to enable interrupt
forwarding if it is already enabled for the zPCI device. This prevents
overwriting and leaking resources when the ioctl is called multiple times for
the same device.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 720bb58cabe2e..d2a11cdf6941d 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -237,6 +237,10 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	if (zdev->gisa == 0)
>  		return -EINVAL;
>  
> +	/* AIF already enabled for the device */
> +	if (zdev->kzdev->fib.fmt0.aibv != 0)
> +		return -EINVAL;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the resource accounting
in account_mem() and unaccount_mem() leak the user_struct reference and
mis-account memory quotas during teardown?

In account_mem(), get_uid() increments the user_struct reference count, but
this reference does not appear to be freed via free_uid() on the success path:

arch/s390/kvm/pci.c:account_mem() {
	struct user_struct *user = get_uid(current_user());
	...
	kzdev->user_account = user;
	kzdev->mm_account = current->mm;

	return 0;
	...
}

Additionally, does unaccount_mem() use the current task context for teardown
instead of the saved context?

arch/s390/kvm/pci.c:unaccount_mem() {
	struct user_struct *user = get_uid(current_user());

	if (user)
		atomic_long_sub(nr_pages, &user->locked_vm);
	if (current->mm)
		atomic64_sub(nr_pages, &current->mm->pinned_vm);
}

If teardown happens in a different context, such as a worker thread during
delayed_fput() (e.g., during kvm_s390_pci_aif_disable), could unaccount_mem()
subtract from the wrong task's locked_vm?

Because locked_vm is an atomic_long_t, subtracting unallocated pages could
cause a negative underflow. During subsequent account_mem() checks, this
negative value might be treated as a large unsigned integer, which would cause
new_pages > page_limit to return -ENOMEM, potentially preventing the affected
user from allocating locked memory in the future.

>  	kvm = zdev->kzdev->kvm;
>  	msi_vecs = min_t(unsigned int, fib->fmt0.noi, zdev->max_msi);
>

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