Re: [PATCH] RDMA/core: EPERM should be returned when # of pined pages is over ulimit

[Yasunori Goto <y-goto@fujitsu.com>]

On 2021/08/20 9:36, Yasunori Goto wrote:
> 
> 
> On 2021/08/20 8:10, Jason Gunthorpe wrote:
>> On Wed, Aug 18, 2021 at 05:27:02PM +0900, Yasunori Goto wrote:
>>> Hello,
>>>
>>> When I started to use SoftRoCE, I'm very confused by
>>> ENOMEM error output even if I gave enough memory.
>>>
>>> I think EPERM is more suitable for uses to solve error rather than
>>> ENOMEM at here of ib_umem_get() when # of pinned pages is over ulimit.
>>> This is not "memory is not enough" problem, because driver can
>>> succeed to pin enough amount of pages, but it is larger than ulimit 
>>> value.
>>>
>>> The hard limit of "max locked memory" can be changed by limit.conf.
>>> In addition, this checks also CAP_IPC_LOCK, it is indeed permmission 
>>> check.
>>> So, I think the following patch.
>>>
>>> If there is a intention why ENOMEM is used here, please let me know.
>>> Otherwise, I'm glad if this is merged.
>>>
>>> Thanks.
>>>
>>>
>>> ---
>>> When # of pinned pages are larger than ulimit of "max locked memory"
>>> without CAP_IPC_LOCK, current ib_umem_get() returns ENOMEM.
>>> But it does not mean "not enough memory", because driver could 
>>> succeed to
>>> pinned enough pages.
>>> This is just capability error. Even if a normal user is limited
>>> his/her # of pinned pages, system administrator can give permission
>>> by change hard limit of this ulimit value.
>>> To notify correct information to user, ib_umem_get()
>>> should return EPERM instead of ENOMEM at here.
>>
>> I'm not convinced, can you find other places checking the ulimit and
>> list what codes they return?
> 
> Hmm, OK.
> 
> I'll investigate it.

After the investigation, I found the followings.

- Many codes return ENOMEM in kernel/driver.
- Only one exception I could find is perf_mmap() in kernel/events/core.c
   It returns EPERM.

----
static int perf_mmap(struct file *file, struct vm_area_struct *vma)
{
    :
    :
         lock_limit = rlimit(RLIMIT_MEMLOCK);
         lock_limit >>= PAGE_SHIFT;
         locked = atomic64_read(&vma->vm_mm->pinned_vm) + extra;

         if ((locked > lock_limit) && perf_is_paranoid() &&
                 !capable(CAP_IPC_LOCK)) {
                 ret = -EPERM; <----!!!
                 goto unlock;
         }
----

- The man pages of mlock(2) says the followings. This seems to be cause
   why ENOMEM is returned in many place.
----
ENOMEM (Linux  2.6.9  and later) the caller had a nonzero RLIMIT_MEMLOCK
        soft resource limit, but tried to lock more memory than the limit
        permitted.   This  limit  is  not  enforced  if  the  process  is
        privileged (CAP_IPC_LOCK).
---

- In addition, POSIX specification(*) also says the followings at
   mlock(2).
---
[ENOMEM]
Locking the pages mapped by the specified range would exceed an
implementation-defined limit on the amount of memory that the process
may lock.
----
(*) https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/

So, I changed my mind now. ib_umem_get() should return ENOMEM.

However, I want to provide some information to make it easy for users to 
understand. For example, sev_pin_memory() of arch/x86/kvm/svm/sev.c 
outputs error message like the followings.

---
static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
    :
    :
         if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
                 pr_err("SEV: %lu locked pages exceed the lock limit of 
%lu.\n", locked, lock_limit);
                 return ERR_PTR(-ENOMEM);
         }
---

I think it is better than nothing. How do you think?

Thanks,
-- -
Yasunori Goto