Re: [GIT PULL 2/7] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

Am 27.07.26 um 12:11 schrieb [email protected]:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] The patch introduces `mm_struct` and `user_struct` reference count leaks on the `airq_iv_alloc_bit()` error path.

For some reason sashiko failed to apply patch3, which fixes this exact problem.

> - [High] Calling the `KVM_S390_ZPCIOP_REG_AEN` ioctl multiple times sequentially overwrites `kzdev->mm_account` and `kzdev->user_account`, permanently leaking the previously acquired kernel object references.

same here. Looks like sashiko applied this series to a wrong tree.

All PCI fixes have been reviewed before here:

https://sashiko.dev/#/patchset/20260723221409.5874-1-alifm%40linux.ibm.com


> 
> Pre-existing issues:
> - [High] Missing NULL check for `airq_iv_create()` return value causes a guaranteed NULL pointer dereference.
> --
> 
> --- Patch [1]: [GIT PULL 2/7] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages ---
> commit c8a84dcb0557ca0ba1070398f64a0a7a9a08e82b
> Author: Farhan Ali <[email protected]>
> Date:   Mon Jul 27 09:57:15 2026 +0000
> 
>      KVM: s390: pci: Fix memory accounting for pinned/unpinned pages
> 
> This patch stores the pinning process user_struct and mm_struct when
> accounting for pinned pages. It subsequently frees these resources when the
> pages are unpinned to resolve context issues.
> 
>> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
>> index 5b075c38998e3..c2f5085a0f5f1 100644
>> --- a/arch/s390/kvm/pci.c
>> +++ b/arch/s390/kvm/pci.c
> [ ... ]
>> @@ -190,33 +190,54 @@ static int kvm_zpci_clear_airq(struct zpci_dev *zdev)
> [ ... ]
>> -static inline int account_mem(unsigned long nr_pages)
>> +static inline int account_mem(struct kvm_zdev *kzdev, unsigned long nr_pages)
>>   {
>>   	struct user_struct *user = get_uid(current_user());
>>   	unsigned long page_limit, cur_pages, new_pages;
>> +	int rc = 0;
>>   
>>   	page_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>>   
>>   	cur_pages = atomic_long_read(&user->locked_vm);
>>   	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));
>>   
>> -	atomic64_add(nr_pages, &current->mm->pinned_vm);
>> +	if (current->mm) {
>> +		mmgrab(current->mm);
>> +		atomic64_add(nr_pages, &current->mm->pinned_vm);
>> +	}
>> +
>> +	kzdev->user_account = user;
>> +	kzdev->mm_account = current->mm;
> 
> [Severity: High]
> If the KVM_S390_ZPCIOP_REG_AEN ioctl invokes kvm_s390_pci_aif_enable()
> multiple times sequentially, will this unconditionally overwrite
> kzdev->user_account and kzdev->mm_account?
> 
> It appears that if the device is already enabled, the previously acquired
> references would be permanently leaked since unaccount_mem() isn't called
> before assigning the new pointers here.
> 
>>   
>>   	return 0;
>> +
>> +out:
>> +	free_uid(user);
>> +	return rc;
>>   }
>>   
>>   static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>> @@ -275,7 +296,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>>   	}
>>   
>>   	/* Account for pinned pages, roll back on failure */
>> -	if (account_mem(pcount))
>> +	if (account_mem(zdev->kzdev, pcount))
>>   		goto unpin2;
>>   
>>   	/* AISB must be allocated before we can fill in GAITE */
> 
> [Severity: High]
> If airq_iv_alloc_bit() fails later in this function, the error path
> jumps to the unlock and unpin labels:
> 
> kvm_s390_pci_aif_enable() {
>          ...
>          bit = airq_iv_alloc_bit(aift->sbv);
>          if (bit == -1UL)
>                  goto unlock;
>          ...
> unlock:
>          mutex_unlock(&aift->aift_lock);
> unpin2:
>          if (fib->fmt0.sum == 1)
>                  unpin_user_page(aisb_page);
>          ...
> }
> 
> Does this error path need to call unaccount_mem() to prevent leaking the
> mm_struct and user_struct references acquired by account_mem() earlier?
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible for airq_iv_create() to
> return NULL under memory pressure?
> 
> If it fails, zdev->aibv is assigned NULL a bit later in
> kvm_s390_pci_aif_enable():
> 
> kvm_s390_pci_aif_enable() {
>          ...
>          zdev->aibv = airq_iv_create(...);
>          ...
>          rc = kvm_zpci_set_airq(zdev);
>          ...
> }
> 
> Could this lead to a NULL pointer dereference when kvm_zpci_set_airq()
> attempts to access airq_iv_end(zdev->aibv)?
>