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@xxxxxxxxxxxxx>
>>> ---
>>>   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;
>>>   };
>>>