linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] RDMA/core: EPERM should be returned when # of pined pages is over ulimit
@ 2021-08-18  8:27 Yasunori Goto
  2021-08-19 23:10 ` Jason Gunthorpe
  0 siblings, 1 reply; 6+ messages in thread
From: Yasunori Goto @ 2021-08-18  8:27 UTC (permalink / raw)
  To: linux-rdma, Doug Ledford, Jason Gunthorpe; +Cc: y-goto

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.

Signed-off-by: Yasunori Goto <y-goto@fujitsu.com>
---
 drivers/infiniband/core/umem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
index 0eb40025075f..9771134649e9 100644
--- a/drivers/infiniband/core/umem.c
+++ b/drivers/infiniband/core/umem.c
@@ -205,7 +205,7 @@ struct ib_umem *ib_umem_get(struct ib_device *device, unsigned long addr,
 	new_pinned = atomic64_add_return(npages, &mm->pinned_vm);
 	if (new_pinned > lock_limit && !capable(CAP_IPC_LOCK)) {
 		atomic64_sub(npages, &mm->pinned_vm);
-		ret = -ENOMEM;
+		ret = -EPERM;
 		goto out;
 	}
 
-- 
2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RDMA/core: EPERM should be returned when # of pined pages is over ulimit
  2021-08-18  8:27 [PATCH] RDMA/core: EPERM should be returned when # of pined pages is over ulimit Yasunori Goto
@ 2021-08-19 23:10 ` Jason Gunthorpe
  2021-08-20  0:36   ` Yasunori Goto
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2021-08-19 23:10 UTC (permalink / raw)
  To: Yasunori Goto; +Cc: linux-rdma, Doug Ledford

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?

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RDMA/core: EPERM should be returned when # of pined pages is over ulimit
  2021-08-19 23:10 ` Jason Gunthorpe
@ 2021-08-20  0:36   ` Yasunori Goto
  2021-08-20  8:45     ` Yasunori Goto
  0 siblings, 1 reply; 6+ messages in thread
From: Yasunori Goto @ 2021-08-20  0:36 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: linux-rdma, Doug Ledford



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.

-- 
Yasunori Goto

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RDMA/core: EPERM should be returned when # of pined pages is over ulimit
  2021-08-20  0:36   ` Yasunori Goto
@ 2021-08-20  8:45     ` Yasunori Goto
  2021-08-26 13:32       ` Jason Gunthorpe
  0 siblings, 1 reply; 6+ messages in thread
From: Yasunori Goto @ 2021-08-20  8:45 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: linux-rdma, Doug Ledford



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RDMA/core: EPERM should be returned when # of pined pages is over ulimit
  2021-08-20  8:45     ` Yasunori Goto
@ 2021-08-26 13:32       ` Jason Gunthorpe
  2021-08-27  0:08         ` Gotou, Yasunori/五島 康文
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2021-08-26 13:32 UTC (permalink / raw)
  To: Yasunori Goto; +Cc: linux-rdma, Doug Ledford

On Fri, Aug 20, 2021 at 05:45:54PM +0900, Yasunori Goto wrote:

> 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?

Unprivileged user space should not be allowed to cause the kernel to
print messages.

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] RDMA/core: EPERM should be returned when # of pined pages is over ulimit
  2021-08-26 13:32       ` Jason Gunthorpe
@ 2021-08-27  0:08         ` Gotou, Yasunori/五島 康文
  0 siblings, 0 replies; 6+ messages in thread
From: Gotou, Yasunori/五島 康文 @ 2021-08-27  0:08 UTC (permalink / raw)
  To: Jason Gunthorpe, Yasunori Goto; +Cc: linux-rdma, Doug Ledford

On 2021/08/26 22:32, Jason Gunthorpe wrote:
> On Fri, Aug 20, 2021 at 05:45:54PM +0900, Yasunori Goto wrote:
> 
>> 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?
> 
> Unprivileged user space should not be allowed to cause the kernel to
> print messages.

Hmm... Ok. I see.

Thank you for your answer!

Bye,
---
Yasunori Goto


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-27  0:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18  8:27 [PATCH] RDMA/core: EPERM should be returned when # of pined pages is over ulimit Yasunori Goto
2021-08-19 23:10 ` Jason Gunthorpe
2021-08-20  0:36   ` Yasunori Goto
2021-08-20  8:45     ` Yasunori Goto
2021-08-26 13:32       ` Jason Gunthorpe
2021-08-27  0:08         ` Gotou, Yasunori/五島 康文

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).