Re: [PATCH net-next] bpf: return errno -ENOMEM when exceeding RLIMIT_MEMLOCK

2017-02-21 Thread Alexei Starovoitov 
On Tue, Feb 21, 2017 at 02:00:13PM +0100, Jesper Dangaard Brouer wrote:
> On Tue, 21 Feb 2017 00:06:11 -0800
> Alexei Starovoitov  wrote:
> 
> > On Mon, Feb 20, 2017 at 05:25:58PM +0100, Jesper Dangaard Brouer wrote:
> > > On Mon, 20 Feb 2017 16:57:34 +0100
> > > Daniel Borkmann  wrote:
> > >   
> > > > On 02/20/2017 04:35 PM, Jesper Dangaard Brouer wrote:  
> > > > > It is confusing users of samples/bpf that exceeding the resource
> > > > > limits for RLIMIT_MEMLOCK result in an "Operation not permitted"
> > > > > message.  This is due to bpf limits check return -EPERM.
> > > > >
> > > > > Instead return -ENOMEM, like most other users of this API.
> > > > >
> > > > > Fixes: aaac3ba95e4c ("bpf: charge user for creation of BPF maps and 
> > > > > programs")
> > > > > Fixes: 6c9059817432 ("bpf: pre-allocate hash map elements")
> > > > > Fixes: 5ccb071e97fb ("bpf: fix overflow in prog accounting")
> > > > 
> > > > Btw, last one just moves the helper so fixes doesn't really apply
> > > > there, but apart from that this is already uapi exposed behavior
> > > > like this for ~1.5yrs, so unfortunately too late to change now. I
> > > > think the original intention (arguably confusing in this context)
> > > > was that user doesn't have (rlimit) permission to allocate this
> > > > resource.  
> > > 
> > > This is obviously confusing end-users, thus it should be fixed IMHO.  
> > 
> > I don't think it's confusing and I think EPERM makes
> > the most sense as return code in such situation.
> 
> Most other kernel users return ENOMEM.

the places in the kernel that check for memlock are not fully consistent.

perf does this:
  lock_limit = rlimit(RLIMIT_MEMLOCK);
  lock_limit >>= PAGE_SHIFT;
  locked = vma->vm_mm->pinned_vm + extra;

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

and in bpf land we got an idea of using memlock limit from perf.

> Documented it here:
>  https://prototype-kernel.readthedocs.io/en/latest/bpf/troubleshooting.html

Thanks! Looks good.

Re: [PATCH net-next] bpf: return errno -ENOMEM when exceeding RLIMIT_MEMLOCK

2017-02-21 Thread Jesper Dangaard Brouer 
On Tue, 21 Feb 2017 00:06:11 -0800
Alexei Starovoitov  wrote:

> On Mon, Feb 20, 2017 at 05:25:58PM +0100, Jesper Dangaard Brouer wrote:
> > On Mon, 20 Feb 2017 16:57:34 +0100
> > Daniel Borkmann  wrote:
> >   
> > > On 02/20/2017 04:35 PM, Jesper Dangaard Brouer wrote:  
> > > > It is confusing users of samples/bpf that exceeding the resource
> > > > limits for RLIMIT_MEMLOCK result in an "Operation not permitted"
> > > > message.  This is due to bpf limits check return -EPERM.
> > > >
> > > > Instead return -ENOMEM, like most other users of this API.
> > > >
> > > > Fixes: aaac3ba95e4c ("bpf: charge user for creation of BPF maps and 
> > > > programs")
> > > > Fixes: 6c9059817432 ("bpf: pre-allocate hash map elements")
> > > > Fixes: 5ccb071e97fb ("bpf: fix overflow in prog accounting")
> > > 
> > > Btw, last one just moves the helper so fixes doesn't really apply
> > > there, but apart from that this is already uapi exposed behavior
> > > like this for ~1.5yrs, so unfortunately too late to change now. I
> > > think the original intention (arguably confusing in this context)
> > > was that user doesn't have (rlimit) permission to allocate this
> > > resource.  
> > 
> > This is obviously confusing end-users, thus it should be fixed IMHO.  
> 
> I don't think it's confusing and I think EPERM makes
> the most sense as return code in such situation.

Most other kernel users return ENOMEM.

> There is also code in iovisor/bcc that specifically looking
> for EPERM to adjust ulimit.

If there is already a program that depend on this, then it is ABI and
we cannot change it... drop this patch.

> May be it's not documented properly, but that's different story.

Documented it here:
 https://prototype-kernel.readthedocs.io/en/latest/bpf/troubleshooting.html

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net-next] bpf: return errno -ENOMEM when exceeding RLIMIT_MEMLOCK

2017-02-21 Thread Alexei Starovoitov 
On Mon, Feb 20, 2017 at 05:25:58PM +0100, Jesper Dangaard Brouer wrote:
> On Mon, 20 Feb 2017 16:57:34 +0100
> Daniel Borkmann  wrote:
> 
> > On 02/20/2017 04:35 PM, Jesper Dangaard Brouer wrote:
> > > It is confusing users of samples/bpf that exceeding the resource
> > > limits for RLIMIT_MEMLOCK result in an "Operation not permitted"
> > > message.  This is due to bpf limits check return -EPERM.
> > >
> > > Instead return -ENOMEM, like most other users of this API.
> > >
> > > Fixes: aaac3ba95e4c ("bpf: charge user for creation of BPF maps and 
> > > programs")
> > > Fixes: 6c9059817432 ("bpf: pre-allocate hash map elements")
> > > Fixes: 5ccb071e97fb ("bpf: fix overflow in prog accounting")  
> > 
> > Btw, last one just moves the helper so fixes doesn't really apply
> > there, but apart from that this is already uapi exposed behavior
> > like this for ~1.5yrs, so unfortunately too late to change now. I
> > think the original intention (arguably confusing in this context)
> > was that user doesn't have (rlimit) permission to allocate this
> > resource.
> 
> This is obviously confusing end-users, thus it should be fixed IMHO.

I don't think it's confusing and I think EPERM makes
the most sense as return code in such situation.
There is also code in iovisor/bcc that specifically looking
for EPERM to adjust ulimit.
May be it's not documented properly, but that's different story.

Re: [PATCH net-next] bpf: return errno -ENOMEM when exceeding RLIMIT_MEMLOCK

2017-02-20 Thread Jesper Dangaard Brouer 
On Mon, 20 Feb 2017 16:57:34 +0100
Daniel Borkmann  wrote:

> On 02/20/2017 04:35 PM, Jesper Dangaard Brouer wrote:
> > It is confusing users of samples/bpf that exceeding the resource
> > limits for RLIMIT_MEMLOCK result in an "Operation not permitted"
> > message.  This is due to bpf limits check return -EPERM.
> >
> > Instead return -ENOMEM, like most other users of this API.
> >
> > Fixes: aaac3ba95e4c ("bpf: charge user for creation of BPF maps and 
> > programs")
> > Fixes: 6c9059817432 ("bpf: pre-allocate hash map elements")
> > Fixes: 5ccb071e97fb ("bpf: fix overflow in prog accounting")  
> 
> Btw, last one just moves the helper so fixes doesn't really apply
> there, but apart from that this is already uapi exposed behavior
> like this for ~1.5yrs, so unfortunately too late to change now. I
> think the original intention (arguably confusing in this context)
> was that user doesn't have (rlimit) permission to allocate this
> resource.

This is obviously confusing end-users, thus it should be fixed IMHO.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net-next] bpf: return errno -ENOMEM when exceeding RLIMIT_MEMLOCK

2017-02-20 Thread Daniel Borkmann 

On 02/20/2017 04:35 PM, Jesper Dangaard Brouer wrote:

It is confusing users of samples/bpf that exceeding the resource
limits for RLIMIT_MEMLOCK result in an "Operation not permitted"
message.  This is due to bpf limits check return -EPERM.

Instead return -ENOMEM, like most other users of this API.

Fixes: aaac3ba95e4c ("bpf: charge user for creation of BPF maps and programs")
Fixes: 6c9059817432 ("bpf: pre-allocate hash map elements")
Fixes: 5ccb071e97fb ("bpf: fix overflow in prog accounting")


Btw, last one just moves the helper so fixes doesn't really apply
there, but apart from that this is already uapi exposed behavior
like this for ~1.5yrs, so unfortunately too late to change now. I
think the original intention (arguably confusing in this context)
was that user doesn't have (rlimit) permission to allocate this
resource.

Thanks,
Daniel

[PATCH net-next] bpf: return errno -ENOMEM when exceeding RLIMIT_MEMLOCK

2017-02-20 Thread Jesper Dangaard Brouer 
It is confusing users of samples/bpf that exceeding the resource
limits for RLIMIT_MEMLOCK result in an "Operation not permitted"
message.  This is due to bpf limits check return -EPERM.

Instead return -ENOMEM, like most other users of this API.

Fixes: aaac3ba95e4c ("bpf: charge user for creation of BPF maps and programs")
Fixes: 6c9059817432 ("bpf: pre-allocate hash map elements")
Fixes: 5ccb071e97fb ("bpf: fix overflow in prog accounting")
Signed-off-by: Jesper Dangaard Brouer 
---
 kernel/bpf/syscall.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 08a4d287226b..37387a9b0d46 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -85,7 +85,7 @@ int bpf_map_precharge_memlock(u32 pages)
cur = atomic_long_read(>locked_vm);
free_uid(user);
if (cur + pages > memlock_limit)
-   return -EPERM;
+   return -ENOMEM;
return 0;
 }
 
@@ -101,7 +101,7 @@ static int bpf_map_charge_memlock(struct bpf_map *map)
if (atomic_long_read(>locked_vm) > memlock_limit) {
atomic_long_sub(map->pages, >locked_vm);
free_uid(user);
-   return -EPERM;
+   return -ENOMEM;
}
map->user = user;
return 0;
@@ -658,7 +658,7 @@ int __bpf_prog_charge(struct user_struct *user, u32 pages)
user_bufs = atomic_long_add_return(pages, >locked_vm);
if (user_bufs > memlock_limit) {
atomic_long_sub(pages, >locked_vm);
-   return -EPERM;
+   return -ENOMEM;
}
}