[PATCH v4 0/6] KVM s390x PCI fixes

[PATCH v4 0/6] KVM s390x PCI fixes

[Farhan Ali]

Hi,

This series attempts to fix some the pre-existing issues[1] found by sashiko.

[1] https://lore.kernel.org/all/20260624063447.85DF51F000E9@smtp.kernel.org/

Thanks
Farhan

ChangeLog
---------
v3: https://lore.kernel.org/all/20260720175819.1723-1-alifm@linux.ibm.com/
v3 -> v4
   - Add validation checks for AISB/AIBV spanning more than a page.
   - Reject multiple ioctl call for the same device, if adapter interrupt
   forwarding is already enabled for the device.
   - Rebase on 7.2-rc4.

v2: https://lore.kernel.org/all/20260716175241.1039-1-alifm@linux.ibm.com/
v2 -> v3
   - Remove overwriting guest FIB since we don't use it for
   re-issue (patch 4).

v1: https://lore.kernel.org/all/20260713172600.1284-1-alifm@linux.ibm.com/
v1 -> v2
   - Drop fix handling AISB/AIBV spanning multiple pages.
   - Fix memory accounting functions for the case when interrupt forwarding
   is enabled by one process but disabled by a different process (patch 1).


Farhan Ali (6):
  KVM: s390: pci: Reject adapter interrupt forwarding if already enabled
  KVM: s390: pci: Fix memory accounting for pinned/unpinned pages
  KVM: s390: pci: Fix missing error codes and memory unaccounting
  KVM: s390: pci: Fix NULL dereference on AIBV allocation failure
  KVM: s390: pci: Fix resource leak on IRQ registration failure
  KVM: s390: pci: Validate AIBV and AISB before pinning guest pages

 arch/s390/kvm/pci.c | 102 +++++++++++++++++++++++++++++++++++---------
 arch/s390/kvm/pci.h |   2 +
 2 files changed, 84 insertions(+), 20 deletions(-)

-- 
2.43.0

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

[Farhan Ali]

The MPCIFC instruction doesn't allow registering adapter interrupts without
first unregistering. So reject any request to enable interrupt forwarding
if its already enabled for the zPCI device. This also fixes overwriting and
thus leaking resources when the ioctl is called multiple times for the same
device.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 720bb58cabe2..d2a11cdf6941 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;
+
 	kvm = zdev->kzdev->kvm;
 	msi_vecs = min_t(unsigned int, fib->fmt0.noi, zdev->max_msi);
 
-- 
2.43.0

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

[Christian Borntraeger]

Am 22.07.26 um 19:06 schrieb Farhan Ali:
> The MPCIFC instruction doesn't allow registering adapter interrupts without
> first unregistering. So reject any request to enable interrupt forwarding
> if its already enabled for the zPCI device. This also fixes overwriting and
> thus leaking resources when the ioctl is called multiple times for the same
> device.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>   arch/s390/kvm/pci.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 720bb58cabe2..d2a11cdf6941 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;
> +
>   	kvm = zdev->kzdev->kvm;
>   	msi_vecs = min_t(unsigned int, fib->fmt0.noi, zdev->max_msi);
>   

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

[Matthew Rosato]

On 7/22/26 1:06 PM, Farhan Ali wrote:
> The MPCIFC instruction doesn't allow registering adapter interrupts without
> first unregistering. So reject any request to enable interrupt forwarding
> if its already enabled for the zPCI device. This also fixes overwriting and
> thus leaking resources when the ioctl is called multiple times for the same
> device.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  arch/s390/kvm/pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 720bb58cabe2..d2a11cdf6941 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;
> +
>  	kvm = zdev->kzdev->kvm;
>  	msi_vecs = min_t(unsigned int, fib->fmt0.noi, zdev->max_msi);
>  

[PATCH v4 2/6] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

[Farhan Ali]

The account_mem() and unaccount_mem() functions call get_uid() which
increments the reference count of struct user_struct on every invocation.
But we don't decrement the count by calling free_uid(). It also
accounted/unaccounted the pages against the current->mm. But its possible
the unaccount_mem() can be called from a different process context than the
one that originally pinned the pages.

Let's fix this by storing the pinning process user_struct and mm_struct
when accounting for pinned pages, and subsequently free these resources
when the pages are unpinned.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 38 ++++++++++++++++++++++++++++----------
 arch/s390/kvm/pci.h |  2 ++
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index d2a11cdf6941..1b3114c7cfbb 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -190,33 +190,51 @@ static int kvm_zpci_clear_airq(struct zpci_dev *zdev)
 	return cc ? -EIO : 0;
 }
 
-static inline void unaccount_mem(unsigned long nr_pages)
+static inline void unaccount_mem(struct kvm_zdev *kzdev, unsigned long nr_pages)
 {
-	struct user_struct *user = get_uid(current_user());
+	struct user_struct *user = kzdev->user_account;
+	struct mm_struct *mm_account = kzdev->mm_account;
 
-	if (user)
+	if (user) {
 		atomic_long_sub(nr_pages, &user->locked_vm);
-	if (current->mm)
-		atomic64_sub(nr_pages, &current->mm->pinned_vm);
+		free_uid(user);
+		kzdev->user_account = NULL;
+	}
+
+	if (mm_account) {
+		atomic64_sub(nr_pages, &mm_account->pinned_vm);
+		mmdrop(mm_account);
+		kzdev->mm_account = NULL;
+	}
 }
 
-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));
 
+	mmgrab(current->mm);
 	atomic64_add(nr_pages, &current->mm->pinned_vm);
+	kzdev->user_account = user;
+	kzdev->mm_account = current->mm;
 
 	return 0;
+
+out:
+	free_uid(user);
+	return rc;
 }
 
 static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
@@ -279,7 +297,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 */
@@ -400,7 +418,7 @@ static int kvm_s390_pci_aif_disable(struct zpci_dev *zdev, bool force)
 		pcount++;
 	}
 	if (pcount > 0)
-		unaccount_mem(pcount);
+		unaccount_mem(kzdev, pcount);
 out:
 	mutex_unlock(&aift->aift_lock);
 
diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h
index ff0972dd5e71..544e6aa75e38 100644
--- a/arch/s390/kvm/pci.h
+++ b/arch/s390/kvm/pci.h
@@ -21,6 +21,8 @@ struct kvm_zdev {
 	struct zpci_dev *zdev;
 	struct kvm *kvm;
 	struct zpci_fib fib;
+	struct user_struct *user_account;
+	struct mm_struct *mm_account;
 	struct list_head entry;
 };
 
-- 
2.43.0

Re: [PATCH v4 2/6] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

[Christian Borntraeger]

Am 22.07.26 um 19:06 schrieb Farhan Ali:
> The account_mem() and unaccount_mem() functions call get_uid() which
> increments the reference count of struct user_struct on every invocation.
> But we don't decrement the count by calling free_uid(). It also
> accounted/unaccounted the pages against the current->mm. But its possible
> the unaccount_mem() can be called from a different process context than the
> one that originally pinned the pages.
> 
> Let's fix this by storing the pinning process user_struct and mm_struct
> when accounting for pinned pages, and subsequently free these resources
> when the pages are unpinned.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>


[..]
> -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;
                             ^^double space. I can fixup during apply.

Re: [PATCH v4 2/6] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

[Matthew Rosato]

On 7/22/26 1:06 PM, Farhan Ali wrote:
> The account_mem() and unaccount_mem() functions call get_uid() which
> increments the reference count of struct user_struct on every invocation.
> But we don't decrement the count by calling free_uid(). It also
> accounted/unaccounted the pages against the current->mm. But its possible
> the unaccount_mem() can be called from a different process context than the
> one that originally pinned the pages.
> 
> Let's fix this by storing the pinning process user_struct and mm_struct
> when accounting for pinned pages, and subsequently free these resources
> when the pages are unpinned.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  arch/s390/kvm/pci.c | 38 ++++++++++++++++++++++++++++----------
>  arch/s390/kvm/pci.h |  2 ++
>  2 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index d2a11cdf6941..1b3114c7cfbb 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -190,33 +190,51 @@ static int kvm_zpci_clear_airq(struct zpci_dev *zdev)
>  	return cc ? -EIO : 0;
>  }
>  
> -static inline void unaccount_mem(unsigned long nr_pages)
> +static inline void unaccount_mem(struct kvm_zdev *kzdev, unsigned long nr_pages)
>  {
> -	struct user_struct *user = get_uid(current_user());
> +	struct user_struct *user = kzdev->user_account;
> +	struct mm_struct *mm_account = kzdev->mm_account;
>  
> -	if (user)
> +	if (user) {
>  		atomic_long_sub(nr_pages, &user->locked_vm);
> -	if (current->mm)
> -		atomic64_sub(nr_pages, &current->mm->pinned_vm);

Previous code handled the case where current->mm could be NULL...

> +		free_uid(user);
> +		kzdev->user_account = NULL;
> +	}
> +
> +	if (mm_account) {

... And you check for kzdev->mm_account being NULL here...

> +		atomic64_sub(nr_pages, &mm_account->pinned_vm);
> +		mmdrop(mm_account);
> +		kzdev->mm_account = NULL;
> +	}
>  }
>  
> -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));
>  
> +	mmgrab(current->mm);

... But here you do not check if current->mm is NULL.  I'm not sure if
it will happen in practice, but we did guard against it before this
patch.  Just add if (!current->mm) around this mmgrab?

>  	atomic64_add(nr_pages, &current->mm->pinned_vm);
> +	kzdev->user_account = user;
> +	kzdev->mm_account = current->mm;

Then if it IS NULL, we will stash a NULL into kzdev->mm_account here and
handle it above as before with the if (mm_account) check.

>  
>  	return 0;
> +
> +out:
> +	free_uid(user);
> +	return rc;
>  }
>  
>  static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
> @@ -279,7 +297,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 */
> @@ -400,7 +418,7 @@ static int kvm_s390_pci_aif_disable(struct zpci_dev *zdev, bool force)
>  		pcount++;
>  	}
>  	if (pcount > 0)
> -		unaccount_mem(pcount);
> +		unaccount_mem(kzdev, pcount);
>  out:
>  	mutex_unlock(&aift->aift_lock);
>  
> diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h
> index ff0972dd5e71..544e6aa75e38 100644
> --- a/arch/s390/kvm/pci.h
> +++ b/arch/s390/kvm/pci.h
> @@ -21,6 +21,8 @@ struct kvm_zdev {
>  	struct zpci_dev *zdev;
>  	struct kvm *kvm;
>  	struct zpci_fib fib;
> +	struct user_struct *user_account;
> +	struct mm_struct *mm_account;
>  	struct list_head entry;
>  };
>  

Re: [PATCH v4 2/6] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

[Farhan Ali]

On 7/23/2026 7:12 AM, Matthew Rosato wrote:
> On 7/22/26 1:06 PM, Farhan Ali wrote:
>> The account_mem() and unaccount_mem() functions call get_uid() which
>> increments the reference count of struct user_struct on every invocation.
>> But we don't decrement the count by calling free_uid(). It also
>> accounted/unaccounted the pages against the current->mm. But its possible
>> the unaccount_mem() can be called from a different process context than the
>> one that originally pinned the pages.
>>
>> Let's fix this by storing the pinning process user_struct and mm_struct
>> when accounting for pinned pages, and subsequently free these resources
>> when the pages are unpinned.
>>
>> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   arch/s390/kvm/pci.c | 38 ++++++++++++++++++++++++++++----------
>>   arch/s390/kvm/pci.h |  2 ++
>>   2 files changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
>> index d2a11cdf6941..1b3114c7cfbb 100644
>> --- a/arch/s390/kvm/pci.c
>> +++ b/arch/s390/kvm/pci.c
>> @@ -190,33 +190,51 @@ static int kvm_zpci_clear_airq(struct zpci_dev *zdev)
>>   	return cc ? -EIO : 0;
>>   }
>>   
>> -static inline void unaccount_mem(unsigned long nr_pages)
>> +static inline void unaccount_mem(struct kvm_zdev *kzdev, unsigned long nr_pages)
>>   {
>> -	struct user_struct *user = get_uid(current_user());
>> +	struct user_struct *user = kzdev->user_account;
>> +	struct mm_struct *mm_account = kzdev->mm_account;
>>   
>> -	if (user)
>> +	if (user) {
>>   		atomic_long_sub(nr_pages, &user->locked_vm);
>> -	if (current->mm)
>> -		atomic64_sub(nr_pages, &current->mm->pinned_vm);
> Previous code handled the case where current->mm could be NULL...
>
>> +		free_uid(user);
>> +		kzdev->user_account = NULL;
>> +	}
>> +
>> +	if (mm_account) {
> ... And you check for kzdev->mm_account being NULL here...
>
>> +		atomic64_sub(nr_pages, &mm_account->pinned_vm);
>> +		mmdrop(mm_account);
>> +		kzdev->mm_account = NULL;
>> +	}
>>   }
>>   
>> -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));
>>   
>> +	mmgrab(current->mm);
> ... But here you do not check if current->mm is NULL.  I'm not sure if
> it will happen in practice, but we did guard against it before this
> patch.  Just add if (!current->mm) around this mmgrab?

But wouldn't the atomic64_add just below this also need to be in the 
check? FWIW looking at some of the other references to mmgrab, I didn't 
see explicit checks for the current->mm [1] [2]

[1] 
https://elixir.bootlin.com/linux/v7.2-rc4/source/io_uring/io_uring.c#L3047

[2] 
https://elixir.bootlin.com/linux/v7.2-rc4/source/drivers/vfio/vfio_iommu_type1.c#L1675

Thanks

Farhan


>
>>   	atomic64_add(nr_pages, &current->mm->pinned_vm);
>> +	kzdev->user_account = user;
>> +	kzdev->mm_account = current->mm;
> Then if it IS NULL, we will stash a NULL into kzdev->mm_account here and
> handle it above as before with the if (mm_account) check.
>
>>   
>>   	return 0;
>> +
>> +out:
>> +	free_uid(user);
>> +	return rc;
>>   }
>>   
>>   static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>> @@ -279,7 +297,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 */
>> @@ -400,7 +418,7 @@ static int kvm_s390_pci_aif_disable(struct zpci_dev *zdev, bool force)
>>   		pcount++;
>>   	}
>>   	if (pcount > 0)
>> -		unaccount_mem(pcount);
>> +		unaccount_mem(kzdev, pcount);
>>   out:
>>   	mutex_unlock(&aift->aift_lock);
>>   
>> diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h
>> index ff0972dd5e71..544e6aa75e38 100644
>> --- a/arch/s390/kvm/pci.h
>> +++ b/arch/s390/kvm/pci.h
>> @@ -21,6 +21,8 @@ struct kvm_zdev {
>>   	struct zpci_dev *zdev;
>>   	struct kvm *kvm;
>>   	struct zpci_fib fib;
>> +	struct user_struct *user_account;
>> +	struct mm_struct *mm_account;
>>   	struct list_head entry;
>>   };
>>   

Re: [PATCH v4 2/6] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

[Matthew Rosato]

On 7/23/26 1:08 PM, Farhan Ali wrote:
> 
> On 7/23/2026 7:12 AM, Matthew Rosato wrote:
>> On 7/22/26 1:06 PM, Farhan Ali wrote:
>>> The account_mem() and unaccount_mem() functions call get_uid() which
>>> increments the reference count of struct user_struct on every
>>> invocation.
>>> But we don't decrement the count by calling free_uid(). It also
>>> accounted/unaccounted the pages against the current->mm. But its
>>> possible
>>> the unaccount_mem() can be called from a different process context
>>> than the
>>> one that originally pinned the pages.
>>>
>>> Let's fix this by storing the pinning process user_struct and mm_struct
>>> when accounting for pinned pages, and subsequently free these resources
>>> when the pages are unpinned.
>>>
>>> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/
>>> disabling interrupt forwarding")
>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>> ---
>>>   arch/s390/kvm/pci.c | 38 ++++++++++++++++++++++++++++----------
>>>   arch/s390/kvm/pci.h |  2 ++
>>>   2 files changed, 30 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
>>> index d2a11cdf6941..1b3114c7cfbb 100644
>>> --- a/arch/s390/kvm/pci.c
>>> +++ b/arch/s390/kvm/pci.c
>>> @@ -190,33 +190,51 @@ static int kvm_zpci_clear_airq(struct zpci_dev
>>> *zdev)
>>>       return cc ? -EIO : 0;
>>>   }
>>>   -static inline void unaccount_mem(unsigned long nr_pages)
>>> +static inline void unaccount_mem(struct kvm_zdev *kzdev, unsigned
>>> long nr_pages)
>>>   {
>>> -    struct user_struct *user = get_uid(current_user());
>>> +    struct user_struct *user = kzdev->user_account;
>>> +    struct mm_struct *mm_account = kzdev->mm_account;
>>>   -    if (user)
>>> +    if (user) {
>>>           atomic_long_sub(nr_pages, &user->locked_vm);
>>> -    if (current->mm)
>>> -        atomic64_sub(nr_pages, &current->mm->pinned_vm);
>> Previous code handled the case where current->mm could be NULL...
>>
>>> +        free_uid(user);
>>> +        kzdev->user_account = NULL;
>>> +    }
>>> +
>>> +    if (mm_account) {
>> ... And you check for kzdev->mm_account being NULL here...
>>
>>> +        atomic64_sub(nr_pages, &mm_account->pinned_vm);
>>> +        mmdrop(mm_account);
>>> +        kzdev->mm_account = NULL;
>>> +    }
>>>   }
>>>   -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));
>>>   +    mmgrab(current->mm);
>> ... But here you do not check if current->mm is NULL.  I'm not sure if
>> it will happen in practice, but we did guard against it before this
>> patch.  Just add if (!current->mm) around this mmgrab?
> 
> But wouldn't the atomic64_add just below this also need to be in the
> check? FWIW looking at some of the other references to mmgrab, I didn't

Yep good point - my intent was to not mess with a structure pointing at
address 0, so you'd want to skip that too.

As for other references to mmgrab, I don't know, but
https://www.kernel.org/doc/Documentation/mm/active_mm.rst
Specifically says to check
if (!current->mm)
to ensure we have user context.

Now, here's the thing:  I believe in practice today all of our calls to
account_mem will have a user context because they are done in response
to an ioctl.  And it is likely that the examples you looked at are also
guaranteed to be in user context.

The problem would be if we were to ever call this accounting function
later from a kernel thread where current->mm was indeed NULL.  I suspect
it was either a review comment on the initial implementation or a case
of 'easy enough to protect against it'

> see explicit checks for the current->mm [1] [2]
> 
> [1] https://elixir.bootlin.com/linux/v7.2-rc4/source/io_uring/
> io_uring.c#L3047
> 
> [2] https://elixir.bootlin.com/linux/v7.2-rc4/source/drivers/vfio/
> vfio_iommu_type1.c#L1675
> 
> Thanks
> 
> Farhan
> 
> 
>>
>>>       atomic64_add(nr_pages, &current->mm->pinned_vm);
>>> +    kzdev->user_account = user;
>>> +    kzdev->mm_account = current->mm;
>> Then if it IS NULL, we will stash a NULL into kzdev->mm_account here and
>> handle it above as before with the if (mm_account) check.
>>
>>>         return 0;
>>> +
>>> +out:
>>> +    free_uid(user);
>>> +    return rc;
>>>   }
>>>     static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct
>>> zpci_fib *fib,
>>> @@ -279,7 +297,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 */
>>> @@ -400,7 +418,7 @@ static int kvm_s390_pci_aif_disable(struct
>>> zpci_dev *zdev, bool force)
>>>           pcount++;
>>>       }
>>>       if (pcount > 0)
>>> -        unaccount_mem(pcount);
>>> +        unaccount_mem(kzdev, pcount);
>>>   out:
>>>       mutex_unlock(&aift->aift_lock);
>>>   diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h
>>> index ff0972dd5e71..544e6aa75e38 100644
>>> --- a/arch/s390/kvm/pci.h
>>> +++ b/arch/s390/kvm/pci.h
>>> @@ -21,6 +21,8 @@ struct kvm_zdev {
>>>       struct zpci_dev *zdev;
>>>       struct kvm *kvm;
>>>       struct zpci_fib fib;
>>> +    struct user_struct *user_account;
>>> +    struct mm_struct *mm_account;
>>>       struct list_head entry;
>>>   };
>>>   

[PATCH v4 3/6] KVM: s390: pci: Fix missing error codes and memory unaccounting

[Farhan Ali]

In kvm_s390_pci_aif_enable() two error paths failed to set error code,
causing the function to return 0 on failure. It also failed to rollback
memory accounting on failure. Fix both by propagating error code on
failure and calling unaccount_mem() in the cleanup path.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 1b3114c7cfbb..33abc15aa685 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -297,14 +297,17 @@ 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(zdev->kzdev, pcount))
+	rc = account_mem(zdev->kzdev, pcount);
+	if (rc)
 		goto unpin2;
 
 	/* AISB must be allocated before we can fill in GAITE */
 	mutex_lock(&aift->aift_lock);
 	bit = airq_iv_alloc_bit(aift->sbv);
-	if (bit == -1UL)
+	if (bit == -1UL) {
+		rc = -ENOMEM;
 		goto unlock;
+	}
 	zdev->aisb = bit; /* store the summary bit number */
 	zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA |
 				    AIRQ_IV_BITLOCK |
@@ -348,6 +351,8 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 	return rc;
 
 unlock:
+	if (pcount > 0)
+		unaccount_mem(zdev->kzdev, pcount);
 	mutex_unlock(&aift->aift_lock);
 unpin2:
 	if (fib->fmt0.sum == 1)
-- 
2.43.0

Re: [PATCH v4 3/6] KVM: s390: pci: Fix missing error codes and memory unaccounting

[Christian Borntraeger]

Am 22.07.26 um 19:06 schrieb Farhan Ali:
> In kvm_s390_pci_aif_enable() two error paths failed to set error code,
> causing the function to return 0 on failure. It also failed to rollback
> memory accounting on failure. Fix both by propagating error code on
> failure and calling unaccount_mem() in the cleanup path.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> ---
>   arch/s390/kvm/pci.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 1b3114c7cfbb..33abc15aa685 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -297,14 +297,17 @@ 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(zdev->kzdev, pcount))
> +	rc = account_mem(zdev->kzdev, pcount);
> +	if (rc)
>   		goto unpin2;
>   
>   	/* AISB must be allocated before we can fill in GAITE */
>   	mutex_lock(&aift->aift_lock);
>   	bit = airq_iv_alloc_bit(aift->sbv);
> -	if (bit == -1UL)
> +	if (bit == -1UL) {
> +		rc = -ENOMEM;
>   		goto unlock;
> +	}
>   	zdev->aisb = bit; /* store the summary bit number */
>   	zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA |
>   				    AIRQ_IV_BITLOCK |
> @@ -348,6 +351,8 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>   	return rc;
>   
>   unlock:
> +	if (pcount > 0)
> +		unaccount_mem(zdev->kzdev, pcount);
>   	mutex_unlock(&aift->aift_lock);
>   unpin2:
>   	if (fib->fmt0.sum == 1)

Re: [PATCH v4 3/6] KVM: s390: pci: Fix missing error codes and memory unaccounting

[Matthew Rosato]

On 7/22/26 1:06 PM, Farhan Ali wrote:
> In kvm_s390_pci_aif_enable() two error paths failed to set error code,

nit: set 'an' error code

> causing the function to return 0 on failure. It also failed to rollback
> memory accounting on failure. Fix both by propagating error code on

'an' error code

> failure and calling unaccount_mem() in the cleanup path.

With or without those changes:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  arch/s390/kvm/pci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 1b3114c7cfbb..33abc15aa685 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -297,14 +297,17 @@ 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(zdev->kzdev, pcount))
> +	rc = account_mem(zdev->kzdev, pcount);
> +	if (rc)
>  		goto unpin2;
>  
>  	/* AISB must be allocated before we can fill in GAITE */
>  	mutex_lock(&aift->aift_lock);
>  	bit = airq_iv_alloc_bit(aift->sbv);
> -	if (bit == -1UL)
> +	if (bit == -1UL) {
> +		rc = -ENOMEM;
>  		goto unlock;
> +	}
>  	zdev->aisb = bit; /* store the summary bit number */
>  	zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA |
>  				    AIRQ_IV_BITLOCK |
> @@ -348,6 +351,8 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	return rc;
>  
>  unlock:
> +	if (pcount > 0)
> +		unaccount_mem(zdev->kzdev, pcount);
>  	mutex_unlock(&aift->aift_lock);
>  unpin2:
>  	if (fib->fmt0.sum == 1)

[PATCH v4 4/6] KVM: s390: pci: Fix NULL dereference on AIBV allocation failure

[Farhan Ali]

The airq_iv_create() can return NULL on failure, but the return value was
never checked. If it fails, zdev->aibv will be NULL and fail when
derefenced in kvm_zpci_set_airq(). Add a NULL check and free the previously
allocated AISB bit and zdev->aisb on failure.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 33abc15aa685..231a4236fc3c 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -314,6 +314,11 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 				    AIRQ_IV_GUESTVEC,
 				    phys_to_virt(fib->fmt0.aibv));
 
+	if (!zdev->aibv) {
+		rc = -ENOMEM;
+		goto free_aisb;
+	}
+
 	spin_lock_irq(&aift->gait_lock);
 	gaite = aift->gait + zdev->aisb;
 
@@ -350,6 +355,9 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 	rc = kvm_zpci_set_airq(zdev);
 	return rc;
 
+free_aisb:
+	airq_iv_free_bit(aift->sbv, zdev->aisb);
+	zdev->aisb = 0;
 unlock:
 	if (pcount > 0)
 		unaccount_mem(zdev->kzdev, pcount);
-- 
2.43.0

Re: [PATCH v4 4/6] KVM: s390: pci: Fix NULL dereference on AIBV allocation failure

[Christian Borntraeger]

Am 22.07.26 um 19:06 schrieb Farhan Ali:
> The airq_iv_create() can return NULL on failure, but the return value was
> never checked. If it fails, zdev->aibv will be NULL and fail when
> derefenced in kvm_zpci_set_airq(). Add a NULL check and free the previously
> allocated AISB bit and zdev->aisb on failure.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>


> ---
>   arch/s390/kvm/pci.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 33abc15aa685..231a4236fc3c 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -314,6 +314,11 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>   				    AIRQ_IV_GUESTVEC,
>   				    phys_to_virt(fib->fmt0.aibv));
>   
> +	if (!zdev->aibv) {
> +		rc = -ENOMEM;
> +		goto free_aisb;
> +	}
> +
>   	spin_lock_irq(&aift->gait_lock);
>   	gaite = aift->gait + zdev->aisb;
>   
> @@ -350,6 +355,9 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>   	rc = kvm_zpci_set_airq(zdev);
>   	return rc;
>   
> +free_aisb:
> +	airq_iv_free_bit(aift->sbv, zdev->aisb);
> +	zdev->aisb = 0;
>   unlock:
>   	if (pcount > 0)
>   		unaccount_mem(zdev->kzdev, pcount);

Re: [PATCH v4 4/6] KVM: s390: pci: Fix NULL dereference on AIBV allocation failure

[Matthew Rosato]

On 7/22/26 1:06 PM, Farhan Ali wrote:
> The airq_iv_create() can return NULL on failure, but the return value was
> never checked. If it fails, zdev->aibv will be NULL and fail when
> derefenced in kvm_zpci_set_airq(). Add a NULL check and free the previously

Nit: s/derefenced/dereferenced/


> allocated AISB bit and zdev->aisb on failure.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  arch/s390/kvm/pci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 33abc15aa685..231a4236fc3c 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -314,6 +314,11 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  				    AIRQ_IV_GUESTVEC,
>  				    phys_to_virt(fib->fmt0.aibv));
>  
> +	if (!zdev->aibv) {
> +		rc = -ENOMEM;
> +		goto free_aisb;
> +	}
> +
>  	spin_lock_irq(&aift->gait_lock);
>  	gaite = aift->gait + zdev->aisb;
>  
> @@ -350,6 +355,9 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	rc = kvm_zpci_set_airq(zdev);
>  	return rc;
>  
> +free_aisb:
> +	airq_iv_free_bit(aift->sbv, zdev->aisb);
> +	zdev->aisb = 0;
>  unlock:
>  	if (pcount > 0)
>  		unaccount_mem(zdev->kzdev, pcount);

[PATCH v4 5/6] KVM: s390: pci: Fix resource leak on IRQ registration failure

[Farhan Ali]

Currently if kvm_zpci_set_airq() fails, kvm_s390_pci_aif_enable() returns
the error code but doesn't do any resource cleanup thus leaking resources.
Fix this by cleaning up all the resources such as the GAITE, AIBV, AISB and
unpinning any pinned pages.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 231a4236fc3c..d76b2c5484ac 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -341,19 +341,32 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 	aift->kzdev[zdev->aisb] = zdev->kzdev;
 	spin_unlock_irq(&aift->gait_lock);
 
-	/* Update guest FIB for re-issue */
-	fib->fmt0.aisbo = zdev->aisb & 63;
-	fib->fmt0.aisb = virt_to_phys(aift->sbv->vector) + (zdev->aisb / 64) * 8;
-	fib->fmt0.isc = gisc;
-
 	/* Save some guest fib values in the host for later use */
-	zdev->kzdev->fib.fmt0.isc = fib->fmt0.isc;
+	zdev->kzdev->fib.fmt0.isc = gisc;
 	zdev->kzdev->fib.fmt0.aibv = fib->fmt0.aibv;
-	mutex_unlock(&aift->aift_lock);
 
 	/* Issue the clp to setup the irq now */
 	rc = kvm_zpci_set_airq(zdev);
-	return rc;
+	if (!rc) {
+		mutex_unlock(&aift->aift_lock);
+		return rc;
+	}
+
+	/* Start cleanup */
+	zdev->kzdev->fib.fmt0.isc = 0;
+	zdev->kzdev->fib.fmt0.aibv = 0;
+
+	spin_lock_irq(&aift->gait_lock);
+	gaite->count--;
+	gaite->aisb = 0;
+	gaite->gisc = 0;
+	gaite->aisbo = 0;
+	gaite->gisa = 0;
+	aift->kzdev[zdev->aisb] = NULL;
+	spin_unlock_irq(&aift->gait_lock);
+
+	airq_iv_release(zdev->aibv);
+	zdev->aibv = NULL;
 
 free_aisb:
 	airq_iv_free_bit(aift->sbv, zdev->aisb);
-- 
2.43.0

Re: [PATCH v4 5/6] KVM: s390: pci: Fix resource leak on IRQ registration failure

[Christian Borntraeger]

Am 22.07.26 um 19:06 schrieb Farhan Ali:
> Currently if kvm_zpci_set_airq() fails, kvm_s390_pci_aif_enable() returns
> the error code but doesn't do any resource cleanup thus leaking resources.
> Fix this by cleaning up all the resources such as the GAITE, AIBV, AISB and
> unpinning any pinned pages.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>


> ---
>   arch/s390/kvm/pci.c | 29 +++++++++++++++++++++--------
>   1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 231a4236fc3c..d76b2c5484ac 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -341,19 +341,32 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>   	aift->kzdev[zdev->aisb] = zdev->kzdev;
>   	spin_unlock_irq(&aift->gait_lock);
>   
> -	/* Update guest FIB for re-issue */
> -	fib->fmt0.aisbo = zdev->aisb & 63;
> -	fib->fmt0.aisb = virt_to_phys(aift->sbv->vector) + (zdev->aisb / 64) * 8;
> -	fib->fmt0.isc = gisc;
> -

This part could benefit from a sentence in the patch description but it seems
correct. But this took a while. I could add something when applying, do
you have a good sentence?

Re: [PATCH v4 5/6] KVM: s390: pci: Fix resource leak on IRQ registration failure

[Matthew Rosato]

On 7/22/26 1:06 PM, Farhan Ali wrote:
> Currently if kvm_zpci_set_airq() fails, kvm_s390_pci_aif_enable() returns
> the error code but doesn't do any resource cleanup thus leaking resources.

Nits: s/the/an/  ....  and 'resource cleanup, thus leaking' (add a comma)

> Fix this by cleaning up all the resources such as the GAITE, AIBV, AISB and
> unpinning any pinned pages.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  arch/s390/kvm/pci.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 231a4236fc3c..d76b2c5484ac 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -341,19 +341,32 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	aift->kzdev[zdev->aisb] = zdev->kzdev;
>  	spin_unlock_irq(&aift->gait_lock);
>  
> -	/* Update guest FIB for re-issue */
> -	fib->fmt0.aisbo = zdev->aisb & 63;
> -	fib->fmt0.aisb = virt_to_phys(aift->sbv->vector) + (zdev->aisb / 64) * 8;
> -	fib->fmt0.isc = gisc;
> -

OK, I had the same problem as Christian here.

This is dead code because we don't re-use the fib, we make a new one in
kvm_zpci_set_airq() and fill it with values from the kzdev.  Right?

I agree adding something to the commit message to that effect would be
good, even something like 'While at it, remove dead code that stored FIB
values that were never referenced.'

>  	/* Save some guest fib values in the host for later use */
> -	zdev->kzdev->fib.fmt0.isc = fib->fmt0.isc;
> +	zdev->kzdev->fib.fmt0.isc = gisc;
>  	zdev->kzdev->fib.fmt0.aibv = fib->fmt0.aibv;
> -	mutex_unlock(&aift->aift_lock);
>  
>  	/* Issue the clp to setup the irq now */
>  	rc = kvm_zpci_set_airq(zdev);
> -	return rc;
> +	if (!rc) {
> +		mutex_unlock(&aift->aift_lock);

This is subtle; we are now holding the aift_lock a bit longer now (over
the MPCIFC re-issue) which I don't think is strictly necessary since we
aren't messing with any of the zpci_aift fields during that timeframe,
but I think is fine to do.  Maybe also worth a mention in the commit
message that the lock is held a bit longer in order to handle the error
case vs drop/re-acquire.

For the code itself:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> +		return rc;
> +	}
> +
> +	/* Start cleanup */
> +	zdev->kzdev->fib.fmt0.isc = 0;
> +	zdev->kzdev->fib.fmt0.aibv = 0;
> +
> +	spin_lock_irq(&aift->gait_lock);
> +	gaite->count--;
> +	gaite->aisb = 0;
> +	gaite->gisc = 0;
> +	gaite->aisbo = 0;
> +	gaite->gisa = 0;
> +	aift->kzdev[zdev->aisb] = NULL;
> +	spin_unlock_irq(&aift->gait_lock);
> +
> +	airq_iv_release(zdev->aibv);
> +	zdev->aibv = NULL;
>  
>  free_aisb:
>  	airq_iv_free_bit(aift->sbv, zdev->aisb);

Re: [PATCH v4 5/6] KVM: s390: pci: Fix resource leak on IRQ registration failure

[Farhan Ali]

On 7/23/2026 8:19 AM, Matthew Rosato wrote:
> On 7/22/26 1:06 PM, Farhan Ali wrote:
>> Currently if kvm_zpci_set_airq() fails, kvm_s390_pci_aif_enable() returns
>> the error code but doesn't do any resource cleanup thus leaking resources.
> Nits: s/the/an/  ....  and 'resource cleanup, thus leaking' (add a comma)
>
>> Fix this by cleaning up all the resources such as the GAITE, AIBV, AISB and
>> unpinning any pinned pages.
>>
>> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   arch/s390/kvm/pci.c | 29 +++++++++++++++++++++--------
>>   1 file changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
>> index 231a4236fc3c..d76b2c5484ac 100644
>> --- a/arch/s390/kvm/pci.c
>> +++ b/arch/s390/kvm/pci.c
>> @@ -341,19 +341,32 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>>   	aift->kzdev[zdev->aisb] = zdev->kzdev;
>>   	spin_unlock_irq(&aift->gait_lock);
>>   
>> -	/* Update guest FIB for re-issue */
>> -	fib->fmt0.aisbo = zdev->aisb & 63;
>> -	fib->fmt0.aisb = virt_to_phys(aift->sbv->vector) + (zdev->aisb / 64) * 8;
>> -	fib->fmt0.isc = gisc;
>> -
> OK, I had the same problem as Christian here.
>
> This is dead code because we don't re-use the fib, we make a new one in
> kvm_zpci_set_airq() and fill it with values from the kzdev.  Right?

Yes, this becomes unnecessary and actually overwriting the fib->fmt0.isc 
causes a reference count leak in the cleanup path as we use 
fib->fmt0.isc when calling kvm_s390_gisc_unregister().


>
> I agree adding something to the commit message to that effect would be
> good, even something like 'While at it, remove dead code that stored FIB
> values that were never referenced.'

Ack, will add that.


>
>>   	/* Save some guest fib values in the host for later use */
>> -	zdev->kzdev->fib.fmt0.isc = fib->fmt0.isc;
>> +	zdev->kzdev->fib.fmt0.isc = gisc;
>>   	zdev->kzdev->fib.fmt0.aibv = fib->fmt0.aibv;
>> -	mutex_unlock(&aift->aift_lock);
>>   
>>   	/* Issue the clp to setup the irq now */
>>   	rc = kvm_zpci_set_airq(zdev);
>> -	return rc;
>> +	if (!rc) {
>> +		mutex_unlock(&aift->aift_lock);
> This is subtle; we are now holding the aift_lock a bit longer now (over
> the MPCIFC re-issue) which I don't think is strictly necessary since we
> aren't messing with any of the zpci_aift fields during that timeframe,
> but I think is fine to do.  Maybe also worth a mention in the commit
> message that the lock is held a bit longer in order to handle the error
> case vs drop/re-acquire.

Yeah, will do. I agree, that its not strictly necessary just makes the 
code a little less messier having to drop and re-acquire. FWIW we also 
acquire the lock before doing a kvm_zpci_clear_airq() in the disable path.

Thanks

Farhan

>
> For the code itself:
>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>
>> +		return rc;
>> +	}
>> +
>> +	/* Start cleanup */
>> +	zdev->kzdev->fib.fmt0.isc = 0;
>> +	zdev->kzdev->fib.fmt0.aibv = 0;
>> +
>> +	spin_lock_irq(&aift->gait_lock);
>> +	gaite->count--;
>> +	gaite->aisb = 0;
>> +	gaite->gisc = 0;
>> +	gaite->aisbo = 0;
>> +	gaite->gisa = 0;
>> +	aift->kzdev[zdev->aisb] = NULL;
>> +	spin_unlock_irq(&aift->gait_lock);
>> +
>> +	airq_iv_release(zdev->aibv);
>> +	zdev->aibv = NULL;
>>   
>>   free_aisb:
>>   	airq_iv_free_bit(aift->sbv, zdev->aisb);

[PATCH v4 6/6] KVM: s390: pci: Validate AIBV and AISB before pinning guest pages

[Farhan Ali]

The AIBV holds one bit per MSI-X vector for a given function. The size of
the bit vector is derived from the NOI and the AIBVO. If the size of the
AIBV exceeds a single page boundary, then reject the request as we cannot
safely pin the guest AIBV.

Similarly reject the request if the AISB address is not 8-byte aligned as
the architecture requires doubleword alignment for the summary bit address.
Since the AISBO can address up to 64 bits, the size of the AISB can only be
8 bytes for the function. This also ensures the AISB doesn't exceed a
single page boundary.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index d76b2c5484ac..e55e75b81b51 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -241,7 +241,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 				   bool assist)
 {
 	struct page *pages[1], *aibv_page, *aisb_page = NULL;
-	unsigned int msi_vecs, idx;
+	unsigned int msi_vecs, idx, size;
 	struct zpci_gaite *gaite;
 	unsigned long hva, bit;
 	struct kvm *kvm;
@@ -268,6 +268,14 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 		return gisc;
 
 	/* Replace AIBV address */
+	size = BITS_TO_LONGS(msi_vecs) * sizeof(unsigned long);
+	size += (fib->fmt0.aibvo / 8) + 1;
+	npages = DIV_ROUND_UP((fib->fmt0.aibv & ~PAGE_MASK) + size, PAGE_SIZE);
+	if (npages > 1) {
+		rc = -EINVAL;
+		goto out;
+	}
+
 	idx = srcu_read_lock(&kvm->srcu);
 	hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aibv));
 	npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
@@ -283,6 +291,12 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 
 	/* Pin the guest AISB if one was specified */
 	if (fib->fmt0.sum == 1) {
+
+		if (fib->fmt0.aisb & 0x7) {
+			rc = -EINVAL;
+			goto unpin1;
+		}
+
 		idx = srcu_read_lock(&kvm->srcu);
 		hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aisb));
 		npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM,
-- 
2.43.0

Re: [PATCH v4 6/6] KVM: s390: pci: Validate AIBV and AISB before pinning guest pages

[Christian Borntraeger]

Am 22.07.26 um 19:06 schrieb Farhan Ali:
> The AIBV holds one bit per MSI-X vector for a given function. The size of
> the bit vector is derived from the NOI and the AIBVO. If the size of the
> AIBV exceeds a single page boundary, then reject the request as we cannot
> safely pin the guest AIBV.
> 
> Similarly reject the request if the AISB address is not 8-byte aligned as
> the architecture requires doubleword alignment for the summary bit address.
> Since the AISBO can address up to 64 bits, the size of the AISB can only be
> 8 bytes for the function. This also ensures the AISB doesn't exceed a
> single page boundary.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>

Apart from the + 1 this looks good.
With that calculation fixed:
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> ---
>   arch/s390/kvm/pci.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index d76b2c5484ac..e55e75b81b51 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -241,7 +241,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>   				   bool assist)
>   {
>   	struct page *pages[1], *aibv_page, *aisb_page = NULL;
> -	unsigned int msi_vecs, idx;
> +	unsigned int msi_vecs, idx, size;
>   	struct zpci_gaite *gaite;
>   	unsigned long hva, bit;
>   	struct kvm *kvm;
> @@ -268,6 +268,14 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>   		return gisc;
>   
>   	/* Replace AIBV address */
> +	size = BITS_TO_LONGS(msi_vecs) * sizeof(unsigned long);
> +	size += (fib->fmt0.aibvo / 8) + 1;
> +	npages = DIV_ROUND_UP((fib->fmt0.aibv & ~PAGE_MASK) + size, PAGE_SIZE);
> +	if (npages > 1) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
>   	idx = srcu_read_lock(&kvm->srcu);
>   	hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aibv));
>   	npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
> @@ -283,6 +291,12 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>   
>   	/* Pin the guest AISB if one was specified */
>   	if (fib->fmt0.sum == 1) {
> +
> +		if (fib->fmt0.aisb & 0x7) {
> +			rc = -EINVAL;
> +			goto unpin1;
> +		}
> +
>   		idx = srcu_read_lock(&kvm->srcu);
>   		hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aisb));
>   		npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM,

Re: [PATCH v4 6/6] KVM: s390: pci: Validate AIBV and AISB before pinning guest pages

[Matthew Rosato]

On 7/22/26 1:06 PM, Farhan Ali wrote:
> The AIBV holds one bit per MSI-X vector for a given function. The size of
> the bit vector is derived from the NOI and the AIBVO. If the size of the
> AIBV exceeds a single page boundary, then reject the request as we cannot
> safely pin the guest AIBV.
> 
> Similarly reject the request if the AISB address is not 8-byte aligned as
> the architecture requires doubleword alignment for the summary bit address.
> Since the AISBO can address up to 64 bits, the size of the AISB can only be
> 8 bytes for the function. This also ensures the AISB doesn't exceed a
> single page boundary.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  arch/s390/kvm/pci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index d76b2c5484ac..e55e75b81b51 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -241,7 +241,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  				   bool assist)
>  {
>  	struct page *pages[1], *aibv_page, *aisb_page = NULL;
> -	unsigned int msi_vecs, idx;
> +	unsigned int msi_vecs, idx, size;
>  	struct zpci_gaite *gaite;
>  	unsigned long hva, bit;
>  	struct kvm *kvm;
> @@ -268,6 +268,14 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  		return gisc;
>  
>  	/* Replace AIBV address */
> +	size = BITS_TO_LONGS(msi_vecs) * sizeof(unsigned long);
> +	size += (fib->fmt0.aibvo / 8) + 1;

Sashiko's comment here looks valid, remove the +1.

> +	npages = DIV_ROUND_UP((fib->fmt0.aibv & ~PAGE_MASK) + size, PAGE_SIZE);
> +	if (npages > 1) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +

Nit: 1-liner comment above that the AIBV cannot span a page?

>  	idx = srcu_read_lock(&kvm->srcu);
>  	hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aibv));
>  	npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
> @@ -283,6 +291,12 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  
>  	/* Pin the guest AISB if one was specified */
>  	if (fib->fmt0.sum == 1) {
> +

Nit: ^ unnecessary newline?

> +		if (fib->fmt0.aisb & 0x7) {
> +			rc = -EINVAL;
> +			goto unpin1;
> +		}
> +

Nit: 1-liner comment above that AISB must be dword-aligned?

With the +1 issue fixed and regardless if you take the nit suggestions
or not:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

>  		idx = srcu_read_lock(&kvm->srcu);
>  		hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aisb));
>  		npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM,

Re: [PATCH v4 0/6] KVM s390x PCI fixes

[Christian Borntraeger]

Am 22.07.26 um 19:06 schrieb Farhan Ali:
> Hi,
> 
> This series attempts to fix some the pre-existing issues[1] found by sashiko.
> 
> [1] https://lore.kernel.org/all/20260624063447.85DF51F000E9@smtp.kernel.org/
> 
> Thanks
> Farhan
> 
> ChangeLog
> ---------
> v3: https://lore.kernel.org/all/20260720175819.1723-1-alifm@linux.ibm.com/
> v3 -> v4
>     - Add validation checks for AISB/AIBV spanning more than a page.
>     - Reject multiple ioctl call for the same device, if adapter interrupt
>     forwarding is already enabled for the device.
>     - Rebase on 7.2-rc4.
> 
> v2: https://lore.kernel.org/all/20260716175241.1039-1-alifm@linux.ibm.com/
> v2 -> v3
>     - Remove overwriting guest FIB since we don't use it for
>     re-issue (patch 4).
> 
> v1: https://lore.kernel.org/all/20260713172600.1284-1-alifm@linux.ibm.com/
> v1 -> v2
>     - Drop fix handling AISB/AIBV spanning multiple pages.
>     - Fix memory accounting functions for the case when interrupt forwarding
>     is enabled by one process but disabled by a different process (patch 1).
> 
> 
> Farhan Ali (6):
>    KVM: s390: pci: Reject adapter interrupt forwarding if already enabled
>    KVM: s390: pci: Fix memory accounting for pinned/unpinned pages
>    KVM: s390: pci: Fix missing error codes and memory unaccounting
>    KVM: s390: pci: Fix NULL dereference on AIBV allocation failure
>    KVM: s390: pci: Fix resource leak on IRQ registration failure
>    KVM: s390: pci: Validate AIBV and AISB before pinning guest pages
> 
>   arch/s390/kvm/pci.c | 102 +++++++++++++++++++++++++++++++++++---------
>   arch/s390/kvm/pci.h |   2 +
>   2 files changed, 84 insertions(+), 20 deletions(-)
> 

I would take this series and do small fixups where necessary. Just tell
me if the change for patch 6 is fine. Or provide a v5.

I suggest to add cc stable to all patches.

Re: [PATCH v4 0/6] KVM s390x PCI fixes

[Matthew Rosato]

> I would take this series and do small fixups where necessary. Just tell
> me if the change for patch 6 is fine. Or provide a v5.
> 

I am reviewing and testing this today, please give me a few more hours.

> I suggest to add cc stable to all patches.

Agree

Re: [PATCH v4 0/6] KVM s390x PCI fixes

[Matthew Rosato]

On 7/23/26 9:00 AM, Matthew Rosato wrote:
> 
>> I would take this series and do small fixups where necessary. Just tell
>> me if the change for patch 6 is fine. Or provide a v5.
>>
> 
> I am reviewing and testing this today, please give me a few more hours.
> 

Farhan, could you do a v5 with comments addressed?

I am willing to also tag patch 2 if you either make the change I asked
for there (or explain to me why it's not necessary).

I've also done a variety of testing, so I'd be happy to also give my
Tested-by to the series.  But would be good to do this on a v5.

>> I suggest to add cc stable to all patches.
> 
> Agree

Re: [PATCH v4 0/6] KVM s390x PCI fixes

[Farhan Ali]

On 7/23/2026 8:35 AM, Matthew Rosato wrote:
> On 7/23/26 9:00 AM, Matthew Rosato wrote:
>>> I would take this series and do small fixups where necessary. Just tell
>>> me if the change for patch 6 is fine. Or provide a v5.
>>>
>> I am reviewing and testing this today, please give me a few more hours.
>>
> Farhan, could you do a v5 with comments addressed?
>
> I am willing to also tag patch 2 if you either make the change I asked
> for there (or explain to me why it's not necessary).

Yes I can spin a v5, I think we have enough reviews to warrant a v5. 
Thanks all for reviewing.

Thanks

Farhan

>
> I've also done a variety of testing, so I'd be happy to also give my
> Tested-by to the series.  But would be good to do this on a v5.
>
>>> I suggest to add cc stable to all patches.
>> Agree