[PATCH bpf-next] libbpf: Print hint about ulimit when getting permission denied error

[PATCH bpf-next] libbpf: Print hint about ulimit when getting permission denied error

[Toke Høiland-Jørgensen]

Probably the single most common error newcomers to XDP are stumped by is
the 'permission denied' error they get when trying to load their program
and 'ulimit -r' is set too low. For examples, see [0], [1].

Since the error code is UAPI, we can't change that. Instead, this patch
adds a few heuristics in libbpf and outputs an additional hint if they are
met: If an EPERM is returned on map create or program load, and geteuid()
shows we are root, and the current RLIMIT_MEMLOCK is not infinity, we
output a hint about raising 'ulimit -r' as an additional log line.

[0] https://marc.info/?l=xdp-newbies&m=157043612505624&w=2
[1] https://github.com/xdp-project/xdp-tutorial/issues/86

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index a2cc7313763a..aec7995674d2 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -41,6 +41,7 @@
 #include <sys/types.h>
 #include <sys/vfs.h>
 #include <sys/utsname.h>
+#include <sys/resource.h>
 #include <tools/libc_compat.h>
 #include <libelf.h>
 #include <gelf.h>
@@ -100,6 +101,24 @@ void libbpf_print(enum libbpf_print_level level, const char *format, ...)
 	va_end(args);
 }
 
+static void pr_perm_msg(int err)
+{
+	struct rlimit limit;
+
+	if (err != -EPERM || geteuid() != 0)
+		return;
+
+	err = getrlimit(RLIMIT_MEMLOCK, &limit);
+	if (err)
+		return;
+
+	if (limit.rlim_cur == RLIM_INFINITY)
+		return;
+
+	pr_warn("permission error while running as root; try raising 'ulimit -r'? current value: %lu\n",
+		limit.rlim_cur);
+}
+
 #define STRERR_BUFSIZE  128
 
 /* Copied from tools/perf/util/util.h */
@@ -2983,6 +3002,7 @@ bpf_object__create_maps(struct bpf_object *obj)
 			cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
 			pr_warn("failed to create map (name: '%s'): %s(%d)\n",
 				map->name, cp, err);
+			pr_perm_msg(err);
 			for (j = 0; j < i; j++)
 				zclose(obj->maps[j].fd);
 			return err;
@@ -4381,6 +4401,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 	ret = -errno;
 	cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
 	pr_warn("load bpf program failed: %s\n", cp);
+	pr_perm_msg(ret);
 
 	if (log_buf && log_buf[0] != '\0') {
 		ret = -LIBBPF_ERRNO__VERIFY;
-- 
2.24.0

Re: [PATCH bpf-next] libbpf: Print hint about ulimit when getting permission denied error

[Jesper Dangaard Brouer]

On Mon, 16 Dec 2019 13:40:31 +0100
Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> Probably the single most common error newcomers to XDP are stumped by is
> the 'permission denied' error they get when trying to load their program
> and 'ulimit -r' is set too low. For examples, see [0], [1].
> 
> Since the error code is UAPI, we can't change that. Instead, this patch
> adds a few heuristics in libbpf and outputs an additional hint if they are
> met: If an EPERM is returned on map create or program load, and geteuid()
> shows we are root, and the current RLIMIT_MEMLOCK is not infinity, we
> output a hint about raising 'ulimit -r' as an additional log line.
> 
> [0] https://marc.info/?l=xdp-newbies&m=157043612505624&w=2
> [1] https://github.com/xdp-project/xdp-tutorial/issues/86
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

This is the top #1 issue users hit again-and-again, too bad we cannot
change the return code as it is UAPI now.  Thanks for taking care of
this mitigation.

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

Re: [PATCH bpf-next] libbpf: Print hint about ulimit when getting permission denied error

[Daniel Borkmann]

On Mon, Dec 16, 2019 at 02:52:30PM +0100, Jesper Dangaard Brouer wrote:
> On Mon, 16 Dec 2019 13:40:31 +0100
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> > Probably the single most common error newcomers to XDP are stumped by is
> > the 'permission denied' error they get when trying to load their program
> > and 'ulimit -r' is set too low. For examples, see [0], [1].
> > 
> > Since the error code is UAPI, we can't change that. Instead, this patch
> > adds a few heuristics in libbpf and outputs an additional hint if they are
> > met: If an EPERM is returned on map create or program load, and geteuid()
> > shows we are root, and the current RLIMIT_MEMLOCK is not infinity, we
> > output a hint about raising 'ulimit -r' as an additional log line.
> > 
> > [0] https://marc.info/?l=xdp-newbies&m=157043612505624&w=2
> > [1] https://github.com/xdp-project/xdp-tutorial/issues/86
> > 
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> This is the top #1 issue users hit again-and-again, too bad we cannot
> change the return code as it is UAPI now.  Thanks for taking care of
> this mitigation.

It's an annoying error that comes up very often, agree, and tooling then
sets it to a high value / inf anyway as next step if it has the rights
to do so. Probably time to revisit the idea that if the user has the same
rights as being able to set setrlimit() anyway, we should just not account
for it ... incomplete hack:

 kernel/bpf/syscall.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b08c362f4e02..116581c32848 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -203,12 +203,17 @@ void bpf_map_init_from_attr(struct bpf_map *map, union bpf_attr *attr)

 static int bpf_charge_memlock(struct user_struct *user, u32 pages)
 {
-	unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
+	unsigned long memlock_limit;

+	if (capable(CAP_SYS_RESOURCE))
+		return 0;
+
+	memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
 	if (atomic_long_add_return(pages, &user->locked_vm) > memlock_limit) {
 		atomic_long_sub(pages, &user->locked_vm);
 		return -EPERM;
 	}
+
 	return 0;
 }

@@ -1339,12 +1344,12 @@ static void bpf_audit_prog(const struct bpf_prog *prog, unsigned int op)

 int __bpf_prog_charge(struct user_struct *user, u32 pages)
 {
-	unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
-	unsigned long user_bufs;
+	unsigned long memlock_limit;

-	if (user) {
-		user_bufs = atomic_long_add_return(pages, &user->locked_vm);
-		if (user_bufs > memlock_limit) {
+	if (user && !capable(CAP_SYS_RESOURCE)) {
+		memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
+		if (atomic_long_add_return(pages, &user->locked_vm) >
+		    memlock_limit) {
 			atomic_long_sub(pages, &user->locked_vm);
 			return -EPERM;
 		}
--
2.21.0

Re: [PATCH bpf-next] libbpf: Print hint about ulimit when getting permission denied error

[Alexei Starovoitov]

On Mon, Dec 16, 2019 at 04:53:36PM +0100, Daniel Borkmann wrote:
> On Mon, Dec 16, 2019 at 02:52:30PM +0100, Jesper Dangaard Brouer wrote:
> > On Mon, 16 Dec 2019 13:40:31 +0100
> > Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > 
> > > Probably the single most common error newcomers to XDP are stumped by is
> > > the 'permission denied' error they get when trying to load their program
> > > and 'ulimit -r' is set too low. For examples, see [0], [1].
> > > 
> > > Since the error code is UAPI, we can't change that. Instead, this patch
> > > adds a few heuristics in libbpf and outputs an additional hint if they are
> > > met: If an EPERM is returned on map create or program load, and geteuid()
> > > shows we are root, and the current RLIMIT_MEMLOCK is not infinity, we
> > > output a hint about raising 'ulimit -r' as an additional log line.
> > > 
> > > [0] https://marc.info/?l=xdp-newbies&m=157043612505624&w=2
> > > [1] https://github.com/xdp-project/xdp-tutorial/issues/86
> > > 
> > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > 
> > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > 
> > This is the top #1 issue users hit again-and-again, too bad we cannot
> > change the return code as it is UAPI now.  Thanks for taking care of
> > this mitigation.
> 
> It's an annoying error that comes up very often, agree, and tooling then
> sets it to a high value / inf anyway as next step if it has the rights
> to do so. Probably time to revisit the idea that if the user has the same
> rights as being able to set setrlimit() anyway, we should just not account
> for it ... incomplete hack:

We cannot drop it quite yet.
There are services that run under root that are relying on this rlimit
to prevent other root services from consuming too much memory.
We need memcg based alternative first before we can remove this limit.
Otherwise users have no way to restrict.

Re: [PATCH bpf-next] libbpf: Print hint about ulimit when getting permission denied error

[Toke Høiland-Jørgensen]

Daniel Borkmann <daniel@iogearbox.net> writes:

> On Mon, Dec 16, 2019 at 02:52:30PM +0100, Jesper Dangaard Brouer wrote:
>> On Mon, 16 Dec 2019 13:40:31 +0100
>> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> 
>> > Probably the single most common error newcomers to XDP are stumped by is
>> > the 'permission denied' error they get when trying to load their program
>> > and 'ulimit -r' is set too low. For examples, see [0], [1].
>> > 
>> > Since the error code is UAPI, we can't change that. Instead, this patch
>> > adds a few heuristics in libbpf and outputs an additional hint if they are
>> > met: If an EPERM is returned on map create or program load, and geteuid()
>> > shows we are root, and the current RLIMIT_MEMLOCK is not infinity, we
>> > output a hint about raising 'ulimit -r' as an additional log line.
>> > 
>> > [0] https://marc.info/?l=xdp-newbies&m=157043612505624&w=2
>> > [1] https://github.com/xdp-project/xdp-tutorial/issues/86
>> > 
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> 
>> This is the top #1 issue users hit again-and-again, too bad we cannot
>> change the return code as it is UAPI now.  Thanks for taking care of
>> this mitigation.
>
> It's an annoying error that comes up very often, agree, and tooling then
> sets it to a high value / inf anyway as next step if it has the rights
> to do so. Probably time to revisit the idea that if the user has the same
> rights as being able to set setrlimit() anyway, we should just not account
> for it ... incomplete hack:

It did always seem a bit odd to me that there was this limit that was
setable by the user it was supposed to limit (for XDP anyway). So I
would totally be in favour of fixing it in the kernel; but probably a
good idea to put the hint into libbpf anyway, for those with older
kernels...

-Toke

Re: [PATCH bpf-next] libbpf: Print hint about ulimit when getting permission denied error

[Toke Høiland-Jørgensen]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Mon, Dec 16, 2019 at 04:53:36PM +0100, Daniel Borkmann wrote:
>> On Mon, Dec 16, 2019 at 02:52:30PM +0100, Jesper Dangaard Brouer wrote:
>> > On Mon, 16 Dec 2019 13:40:31 +0100
>> > Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> > 
>> > > Probably the single most common error newcomers to XDP are stumped by is
>> > > the 'permission denied' error they get when trying to load their program
>> > > and 'ulimit -r' is set too low. For examples, see [0], [1].
>> > > 
>> > > Since the error code is UAPI, we can't change that. Instead, this patch
>> > > adds a few heuristics in libbpf and outputs an additional hint if they are
>> > > met: If an EPERM is returned on map create or program load, and geteuid()
>> > > shows we are root, and the current RLIMIT_MEMLOCK is not infinity, we
>> > > output a hint about raising 'ulimit -r' as an additional log line.
>> > > 
>> > > [0] https://marc.info/?l=xdp-newbies&m=157043612505624&w=2
>> > > [1] https://github.com/xdp-project/xdp-tutorial/issues/86
>> > > 
>> > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> > 
>> > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> > 
>> > This is the top #1 issue users hit again-and-again, too bad we cannot
>> > change the return code as it is UAPI now.  Thanks for taking care of
>> > this mitigation.
>> 
>> It's an annoying error that comes up very often, agree, and tooling then
>> sets it to a high value / inf anyway as next step if it has the rights
>> to do so. Probably time to revisit the idea that if the user has the same
>> rights as being able to set setrlimit() anyway, we should just not account
>> for it ... incomplete hack:
>
> We cannot drop it quite yet.
> There are services that run under root that are relying on this rlimit
> to prevent other root services from consuming too much memory.

How do they do that? Set a pre-defined limit and rely on the other
services not calling setrlimit()? There's no way to read the current
value of how much memory is locked either (is there?), so for multiple
daemons there's a central policy thing that does
SUM(requirement_per_daemon)?

> We need memcg based alternative first before we can remove this limit.
> Otherwise users have no way to restrict.

Yeah, something cg-based would make a lot of sense here (and also
presumably make it possible to read out the current value, right?).

-Toke

Re: [PATCH bpf-next] libbpf: Print hint about ulimit when getting permission denied error

[Yonghong Song]

On 12/16/19 4:40 AM, Toke Høiland-Jørgensen wrote:
> Probably the single most common error newcomers to XDP are stumped by is
> the 'permission denied' error they get when trying to load their program
> and 'ulimit -r' is set too low. For examples, see [0], [1].
> 
> Since the error code is UAPI, we can't change that. Instead, this patch
> adds a few heuristics in libbpf and outputs an additional hint if they are
> met: If an EPERM is returned on map create or program load, and geteuid()
> shows we are root, and the current RLIMIT_MEMLOCK is not infinity, we
> output a hint about raising 'ulimit -r' as an additional log line.
> 
> [0] https://marc.info/?l=xdp-newbies&m=157043612505624&w=2
> [1] https://github.com/xdp-project/xdp-tutorial/issues/86
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

LGTM with one minor no-essential suggestion below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   tools/lib/bpf/libbpf.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index a2cc7313763a..aec7995674d2 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -41,6 +41,7 @@
>   #include <sys/types.h>
>   #include <sys/vfs.h>
>   #include <sys/utsname.h>
> +#include <sys/resource.h>
>   #include <tools/libc_compat.h>
>   #include <libelf.h>
>   #include <gelf.h>
> @@ -100,6 +101,24 @@ void libbpf_print(enum libbpf_print_level level, const char *format, ...)
>   	va_end(args);
>   }
>   
> +static void pr_perm_msg(int err)
> +{
> +	struct rlimit limit;
> +
> +	if (err != -EPERM || geteuid() != 0)
> +		return;
> +
> +	err = getrlimit(RLIMIT_MEMLOCK, &limit);
> +	if (err)
> +		return;
> +
> +	if (limit.rlim_cur == RLIM_INFINITY)
> +		return;
> +
> +	pr_warn("permission error while running as root; try raising 'ulimit -r'? current value: %lu\n",
> +		limit.rlim_cur);

Here we print out in terms of bytes. Maybe in terms of kilo bytes or 
mega bytes is more user friendly, esp. we want them to set a different 
value?

> +}
> +
>   #define STRERR_BUFSIZE  128
>   
>   /* Copied from tools/perf/util/util.h */
> @@ -2983,6 +3002,7 @@ bpf_object__create_maps(struct bpf_object *obj)
>   			cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
>   			pr_warn("failed to create map (name: '%s'): %s(%d)\n",
>   				map->name, cp, err);
> +			pr_perm_msg(err);
>   			for (j = 0; j < i; j++)
>   				zclose(obj->maps[j].fd);
>   			return err;
> @@ -4381,6 +4401,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>   	ret = -errno;
>   	cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
>   	pr_warn("load bpf program failed: %s\n", cp);
> +	pr_perm_msg(ret);
>   
>   	if (log_buf && log_buf[0] != '\0') {
>   		ret = -LIBBPF_ERRNO__VERIFY;
> 

Re: [PATCH bpf-next] libbpf: Print hint about ulimit when getting permission denied error

[Toke Høiland-Jørgensen]

Yonghong Song <yhs@fb.com> writes:

> On 12/16/19 4:40 AM, Toke Høiland-Jørgensen wrote:
>> Probably the single most common error newcomers to XDP are stumped by is
>> the 'permission denied' error they get when trying to load their program
>> and 'ulimit -r' is set too low. For examples, see [0], [1].
>> 
>> Since the error code is UAPI, we can't change that. Instead, this patch
>> adds a few heuristics in libbpf and outputs an additional hint if they are
>> met: If an EPERM is returned on map create or program load, and geteuid()
>> shows we are root, and the current RLIMIT_MEMLOCK is not infinity, we
>> output a hint about raising 'ulimit -r' as an additional log line.
>> 
>> [0] https://marc.info/?l=xdp-newbies&m=157043612505624&w=2
>> [1] https://github.com/xdp-project/xdp-tutorial/issues/86
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
> LGTM with one minor no-essential suggestion below.
>
> Acked-by: Yonghong Song <yhs@fb.com>
>
>> ---
>>   tools/lib/bpf/libbpf.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>> 
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index a2cc7313763a..aec7995674d2 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -41,6 +41,7 @@
>>   #include <sys/types.h>
>>   #include <sys/vfs.h>
>>   #include <sys/utsname.h>
>> +#include <sys/resource.h>
>>   #include <tools/libc_compat.h>
>>   #include <libelf.h>
>>   #include <gelf.h>
>> @@ -100,6 +101,24 @@ void libbpf_print(enum libbpf_print_level level, const char *format, ...)
>>   	va_end(args);
>>   }
>>   
>> +static void pr_perm_msg(int err)
>> +{
>> +	struct rlimit limit;
>> +
>> +	if (err != -EPERM || geteuid() != 0)
>> +		return;
>> +
>> +	err = getrlimit(RLIMIT_MEMLOCK, &limit);
>> +	if (err)
>> +		return;
>> +
>> +	if (limit.rlim_cur == RLIM_INFINITY)
>> +		return;
>> +
>> +	pr_warn("permission error while running as root; try raising 'ulimit -r'? current value: %lu\n",
>> +		limit.rlim_cur);
>
> Here we print out in terms of bytes. Maybe in terms of kilo bytes or 
> mega bytes is more user friendly, esp. we want them to set a different 
> value?

Yeah, thought about that, but was too lazy to actually implement it :)

Can send a v2 with that added...

-Toke