[PATCH bpf-next v3] libbpf: fix probe code to return EPERM if encountered

[PATCH bpf-next v3] libbpf: fix probe code to return EPERM if encountered

[Eelco Chaudron]

When the probe code was failing for any reason ENOTSUP was returned, even
if this was due to no having enough lock space. This patch fixes this by
returning EPERM to the user application, so it can respond and increase
the RLIMIT_MEMLOCK size.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v3: Updated error message to be more specific as suggested by Andrii
v2: Split bpf_object__probe_name() in two functions as suggested by Andrii

 tools/lib/bpf/libbpf.c |   31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 8f480e29a6b0..ad3043c5db13 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -3149,7 +3149,7 @@ int bpf_map__resize(struct bpf_map *map, __u32 max_entries)
 }
 
 static int
-bpf_object__probe_name(struct bpf_object *obj)
+bpf_object__probe_loading(struct bpf_object *obj)
 {
 	struct bpf_load_program_attr attr;
 	char *cp, errmsg[STRERR_BUFSIZE];
@@ -3170,14 +3170,34 @@ bpf_object__probe_name(struct bpf_object *obj)
 	ret = bpf_load_program_xattr(&attr, NULL, 0);
 	if (ret < 0) {
 		cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
-		pr_warn("Error in %s():%s(%d). Couldn't load basic 'r0 = 0' BPF program.\n",
-			__func__, cp, errno);
+		pr_warn("Error in %s():%s(%d). Couldn't load trivial BPF "
+			"program. Make sure your kernel supports BPF "
+			"(CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is "
+			"set to big enough value.\n", __func__, cp, errno);
 		return -errno;
 	}
 	close(ret);
 
-	/* now try the same program, but with the name */
+	return 0;
+}
+
+static int
+bpf_object__probe_name(struct bpf_object *obj)
+{
+	struct bpf_load_program_attr attr;
+	struct bpf_insn insns[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+	int ret;
+
+	/* make sure loading with name works */
 
+	memset(&attr, 0, sizeof(attr));
+	attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
+	attr.insns = insns;
+	attr.insns_cnt = ARRAY_SIZE(insns);
+	attr.license = "GPL";
 	attr.name = "test";
 	ret = bpf_load_program_xattr(&attr, NULL, 0);
 	if (ret >= 0) {
@@ -5386,7 +5406,8 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 
 	obj->loaded = true;
 
-	err = bpf_object__probe_caps(obj);
+	err = bpf_object__probe_loading(obj);
+	err = err ? : bpf_object__probe_caps(obj);
 	err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
 	err = err ? : bpf_object__sanitize_and_load_btf(obj);
 	err = err ? : bpf_object__sanitize_maps(obj);

Re: [PATCH bpf-next v3] libbpf: fix probe code to return EPERM if encountered

[Yonghong Song]

On 5/11/20 5:40 AM, Eelco Chaudron wrote:
> When the probe code was failing for any reason ENOTSUP was returned, even
> if this was due to no having enough lock space. This patch fixes this by
> returning EPERM to the user application, so it can respond and increase
> the RLIMIT_MEMLOCK size.
> 
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
> v3: Updated error message to be more specific as suggested by Andrii
> v2: Split bpf_object__probe_name() in two functions as suggested by Andrii
> 
>   tools/lib/bpf/libbpf.c |   31 ++++++++++++++++++++++++++-----
>   1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 8f480e29a6b0..ad3043c5db13 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3149,7 +3149,7 @@ int bpf_map__resize(struct bpf_map *map, __u32 max_entries)
>   }
>   
>   static int
> -bpf_object__probe_name(struct bpf_object *obj)
> +bpf_object__probe_loading(struct bpf_object *obj)
>   {
>   	struct bpf_load_program_attr attr;
>   	char *cp, errmsg[STRERR_BUFSIZE];
> @@ -3170,14 +3170,34 @@ bpf_object__probe_name(struct bpf_object *obj)
>   	ret = bpf_load_program_xattr(&attr, NULL, 0);
>   	if (ret < 0) {
>   		cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
> -		pr_warn("Error in %s():%s(%d). Couldn't load basic 'r0 = 0' BPF program.\n",
> -			__func__, cp, errno);
> +		pr_warn("Error in %s():%s(%d). Couldn't load trivial BPF "
> +			"program. Make sure your kernel supports BPF "
> +			"(CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is "
> +			"set to big enough value.\n", __func__, cp, errno);
>   		return -errno;

Just curious. Did "errno" always survive pr_warn() here? pr_warn() may 
call user supplied print function which it outside libbpf control.
Maybe should cache errno before calling pr_warn()?

>   	}
>   	close(ret);
>   
> -	/* now try the same program, but with the name */
> +	return 0;
> +}
> +
> +static int
> +bpf_object__probe_name(struct bpf_object *obj)
> +{
> +	struct bpf_load_program_attr attr;
> +	struct bpf_insn insns[] = {
> +		BPF_MOV64_IMM(BPF_REG_0, 0),
> +		BPF_EXIT_INSN(),
> +	};
> +	int ret;
> +
> +	/* make sure loading with name works */
>   
> +	memset(&attr, 0, sizeof(attr));
> +	attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> +	attr.insns = insns;
> +	attr.insns_cnt = ARRAY_SIZE(insns);
> +	attr.license = "GPL";
>   	attr.name = "test";
>   	ret = bpf_load_program_xattr(&attr, NULL, 0);
>   	if (ret >= 0) {
> @@ -5386,7 +5406,8 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>   
>   	obj->loaded = true;
>   
> -	err = bpf_object__probe_caps(obj);
> +	err = bpf_object__probe_loading(obj);
> +	err = err ? : bpf_object__probe_caps(obj);
>   	err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
>   	err = err ? : bpf_object__sanitize_and_load_btf(obj);
>   	err = err ? : bpf_object__sanitize_maps(obj);
> 

Re: [PATCH bpf-next v3] libbpf: fix probe code to return EPERM if encountered

[Daniel Borkmann]

On 5/11/20 10:43 PM, Yonghong Song wrote:
> On 5/11/20 5:40 AM, Eelco Chaudron wrote:
>> When the probe code was failing for any reason ENOTSUP was returned, even
>> if this was due to no having enough lock space. This patch fixes this by
>> returning EPERM to the user application, so it can respond and increase
>> the RLIMIT_MEMLOCK size.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>> v3: Updated error message to be more specific as suggested by Andrii
>> v2: Split bpf_object__probe_name() in two functions as suggested by Andrii
>>
>>   tools/lib/bpf/libbpf.c |   31 ++++++++++++++++++++++++++-----
>>   1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 8f480e29a6b0..ad3043c5db13 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -3149,7 +3149,7 @@ int bpf_map__resize(struct bpf_map *map, __u32 max_entries)
>>   }
>>   static int
>> -bpf_object__probe_name(struct bpf_object *obj)
>> +bpf_object__probe_loading(struct bpf_object *obj)
>>   {
>>       struct bpf_load_program_attr attr;
>>       char *cp, errmsg[STRERR_BUFSIZE];
>> @@ -3170,14 +3170,34 @@ bpf_object__probe_name(struct bpf_object *obj)
>>       ret = bpf_load_program_xattr(&attr, NULL, 0);
>>       if (ret < 0) {
>>           cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
>> -        pr_warn("Error in %s():%s(%d). Couldn't load basic 'r0 = 0' BPF program.\n",
>> -            __func__, cp, errno);
>> +        pr_warn("Error in %s():%s(%d). Couldn't load trivial BPF "
>> +            "program. Make sure your kernel supports BPF "
>> +            "(CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is "
>> +            "set to big enough value.\n", __func__, cp, errno);
>>           return -errno;
> 
> Just curious. Did "errno" always survive pr_warn() here? pr_warn() may call user supplied print function which it outside libbpf control.
> Maybe should cache errno before calling pr_warn()?

+1, I think right now it's a bit of a mess in libbpf. Plenty of cases where we cache errno
before pr_warn() and plenty of cases where we don't. I think we should avoid any surprises
and do cache it on these occasions everywhere. Maybe a cocci script would help to fix the
remaining sites for good.

Thanks,
Daniel

Re: [PATCH bpf-next v3] libbpf: fix probe code to return EPERM if encountered

[Eelco Chaudron]

On 11 May 2020, at 22:43, Yonghong Song wrote:

> On 5/11/20 5:40 AM, Eelco Chaudron wrote:
>> When the probe code was failing for any reason ENOTSUP was returned, 
>> even
>> if this was due to no having enough lock space. This patch fixes this 
>> by
>> returning EPERM to the user application, so it can respond and 
>> increase
>> the RLIMIT_MEMLOCK size.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>> v3: Updated error message to be more specific as suggested by Andrii
>> v2: Split bpf_object__probe_name() in two functions as suggested by 
>> Andrii
>>
>>   tools/lib/bpf/libbpf.c |   31 ++++++++++++++++++++++++++-----
>>   1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 8f480e29a6b0..ad3043c5db13 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -3149,7 +3149,7 @@ int bpf_map__resize(struct bpf_map *map, __u32 
>> max_entries)
>>   }
>>    static int
>> -bpf_object__probe_name(struct bpf_object *obj)
>> +bpf_object__probe_loading(struct bpf_object *obj)
>>   {
>>   	struct bpf_load_program_attr attr;
>>   	char *cp, errmsg[STRERR_BUFSIZE];
>> @@ -3170,14 +3170,34 @@ bpf_object__probe_name(struct bpf_object 
>> *obj)
>>   	ret = bpf_load_program_xattr(&attr, NULL, 0);
>>   	if (ret < 0) {
>>   		cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
>> -		pr_warn("Error in %s():%s(%d). Couldn't load basic 'r0 = 0' BPF 
>> program.\n",
>> -			__func__, cp, errno);
>> +		pr_warn("Error in %s():%s(%d). Couldn't load trivial BPF "
>> +			"program. Make sure your kernel supports BPF "
>> +			"(CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is "
>> +			"set to big enough value.\n", __func__, cp, errno);
>>   		return -errno;
>
> Just curious. Did "errno" always survive pr_warn() here? pr_warn() may 
> call user supplied print function which it outside libbpf control.
> Maybe should cache errno before calling pr_warn()?

I guess this issue has always been there, however, I sent out a v4 
fixing this case.

>>   	}
>>   	close(ret);
>>  -	/* now try the same program, but with the name */
>> +	return 0;
>> +}
>> +
>> +static int
>> +bpf_object__probe_name(struct bpf_object *obj)
>> +{
>> +	struct bpf_load_program_attr attr;
>> +	struct bpf_insn insns[] = {
>> +		BPF_MOV64_IMM(BPF_REG_0, 0),
>> +		BPF_EXIT_INSN(),
>> +	};
>> +	int ret;
>> +
>> +	/* make sure loading with name works */
>>  +	memset(&attr, 0, sizeof(attr));
>> +	attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
>> +	attr.insns = insns;
>> +	attr.insns_cnt = ARRAY_SIZE(insns);
>> +	attr.license = "GPL";
>>   	attr.name = "test";
>>   	ret = bpf_load_program_xattr(&attr, NULL, 0);
>>   	if (ret >= 0) {
>> @@ -5386,7 +5406,8 @@ int bpf_object__load_xattr(struct 
>> bpf_object_load_attr *attr)
>>    	obj->loaded = true;
>>  -	err = bpf_object__probe_caps(obj);
>> +	err = bpf_object__probe_loading(obj);
>> +	err = err ? : bpf_object__probe_caps(obj);
>>   	err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
>>   	err = err ? : bpf_object__sanitize_and_load_btf(obj);
>>   	err = err ? : bpf_object__sanitize_maps(obj);
>>