All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf] bpftool: fix the error when lookup in no-btf maps
@ 2022-02-07 16:00 Yinjun Zhang
  2022-02-07 17:42 ` Andrii Nakryiko
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Yinjun Zhang @ 2022-02-07 16:00 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii
  Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh,
	niklas.soderlund, Yinjun Zhang, Simon Horman

When reworking btf__get_from_id() in commit a19f93cfafdf the error
handling when calling bpf_btf_get_fd_by_id() changed. Before the rework
if bpf_btf_get_fd_by_id() failed the error would not be propagated to
callers of btf__get_from_id(), after the rework it is. This lead to a
change in behavior in print_key_value() that now prints an error when
trying to lookup keys in maps with no btf available.

Fix this by following the way used in dumping maps to allow to look up
keys in no-btf maps, by which it decides whether and where to get the
btf info according to the btf value type.

Fixes: a19f93cfafdf ("libbpf: Add internal helper to load BTF data by FD")
Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 tools/bpf/bpftool/map.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index cc530a229812..4fc772d66e3a 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -1054,11 +1054,9 @@ static void print_key_value(struct bpf_map_info *info, void *key,
 	json_writer_t *btf_wtr;
 	struct btf *btf;
 
-	btf = btf__load_from_kernel_by_id(info->btf_id);
-	if (libbpf_get_error(btf)) {
-		p_err("failed to get btf");
+	btf = get_map_kv_btf(info);
+	if (libbpf_get_error(btf))
 		return;
-	}
 
 	if (json_output) {
 		print_entry_json(info, key, value, btf);
-- 
1.8.3.1


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

* Re: [PATCH bpf] bpftool: fix the error when lookup in no-btf maps
  2022-02-07 16:00 [PATCH bpf] bpftool: fix the error when lookup in no-btf maps Yinjun Zhang
@ 2022-02-07 17:42 ` Andrii Nakryiko
  2022-02-07 17:57   ` Jiri Olsa
  2022-02-15  9:47 ` Jiri Olsa
  2022-02-15 17:10 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2022-02-07 17:42 UTC (permalink / raw)
  To: Yinjun Zhang
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin Lau, Song Liu, Yonghong Song, john fastabend, KP Singh,
	niklas.soderlund, Simon Horman

On Mon, Feb 7, 2022 at 8:00 AM Yinjun Zhang <yinjun.zhang@corigine.com> wrote:
>
> When reworking btf__get_from_id() in commit a19f93cfafdf the error
> handling when calling bpf_btf_get_fd_by_id() changed. Before the rework
> if bpf_btf_get_fd_by_id() failed the error would not be propagated to
> callers of btf__get_from_id(), after the rework it is. This lead to a
> change in behavior in print_key_value() that now prints an error when
> trying to lookup keys in maps with no btf available.
>
> Fix this by following the way used in dumping maps to allow to look up
> keys in no-btf maps, by which it decides whether and where to get the
> btf info according to the btf value type.
>
> Fixes: a19f93cfafdf ("libbpf: Add internal helper to load BTF data by FD")
> Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> ---
>  tools/bpf/bpftool/map.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index cc530a229812..4fc772d66e3a 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -1054,11 +1054,9 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>         json_writer_t *btf_wtr;
>         struct btf *btf;
>
> -       btf = btf__load_from_kernel_by_id(info->btf_id);
> -       if (libbpf_get_error(btf)) {
> -               p_err("failed to get btf");
> +       btf = get_map_kv_btf(info);
> +       if (libbpf_get_error(btf))

See discussion in [0], it seems relevant.

  [0] https://lore.kernel.org/bpf/20220204225823.339548-3-jolsa@kernel.org/

>                 return;
> -       }
>
>         if (json_output) {
>                 print_entry_json(info, key, value, btf);
> --
> 1.8.3.1
>

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

* Re: [PATCH bpf] bpftool: fix the error when lookup in no-btf maps
  2022-02-07 17:42 ` Andrii Nakryiko
@ 2022-02-07 17:57   ` Jiri Olsa
  2022-02-07 18:26     ` Jiri Olsa
  0 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2022-02-07 17:57 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Yinjun Zhang, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
	john fastabend, KP Singh, niklas.soderlund, Simon Horman

On Mon, Feb 07, 2022 at 09:42:25AM -0800, Andrii Nakryiko wrote:
> On Mon, Feb 7, 2022 at 8:00 AM Yinjun Zhang <yinjun.zhang@corigine.com> wrote:
> >
> > When reworking btf__get_from_id() in commit a19f93cfafdf the error
> > handling when calling bpf_btf_get_fd_by_id() changed. Before the rework
> > if bpf_btf_get_fd_by_id() failed the error would not be propagated to
> > callers of btf__get_from_id(), after the rework it is. This lead to a
> > change in behavior in print_key_value() that now prints an error when
> > trying to lookup keys in maps with no btf available.
> >
> > Fix this by following the way used in dumping maps to allow to look up
> > keys in no-btf maps, by which it decides whether and where to get the
> > btf info according to the btf value type.
> >
> > Fixes: a19f93cfafdf ("libbpf: Add internal helper to load BTF data by FD")
> > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> > ---
> >  tools/bpf/bpftool/map.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > index cc530a229812..4fc772d66e3a 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -1054,11 +1054,9 @@ static void print_key_value(struct bpf_map_info *info, void *key,
> >         json_writer_t *btf_wtr;
> >         struct btf *btf;
> >
> > -       btf = btf__load_from_kernel_by_id(info->btf_id);
> > -       if (libbpf_get_error(btf)) {
> > -               p_err("failed to get btf");
> > +       btf = get_map_kv_btf(info);
> > +       if (libbpf_get_error(btf))
> 
> See discussion in [0], it seems relevant.
> 
>   [0] https://lore.kernel.org/bpf/20220204225823.339548-3-jolsa@kernel.org/

I checked and this patch does not fix the problem for me,
but looks like similar issue, do you have test case for this?

mine is to dump any no-btf map with -p option

thanks,
jirka


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

* Re: [PATCH bpf] bpftool: fix the error when lookup in no-btf maps
  2022-02-07 17:57   ` Jiri Olsa
@ 2022-02-07 18:26     ` Jiri Olsa
  2022-02-07 20:34       ` Yonghong Song
  2022-02-08  4:53       ` Yinjun Zhang
  0 siblings, 2 replies; 8+ messages in thread
From: Jiri Olsa @ 2022-02-07 18:26 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Yinjun Zhang, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
	john fastabend, KP Singh, niklas.soderlund, Simon Horman

On Mon, Feb 07, 2022 at 06:57:20PM +0100, Jiri Olsa wrote:
> On Mon, Feb 07, 2022 at 09:42:25AM -0800, Andrii Nakryiko wrote:
> > On Mon, Feb 7, 2022 at 8:00 AM Yinjun Zhang <yinjun.zhang@corigine.com> wrote:
> > >
> > > When reworking btf__get_from_id() in commit a19f93cfafdf the error
> > > handling when calling bpf_btf_get_fd_by_id() changed. Before the rework
> > > if bpf_btf_get_fd_by_id() failed the error would not be propagated to
> > > callers of btf__get_from_id(), after the rework it is. This lead to a
> > > change in behavior in print_key_value() that now prints an error when
> > > trying to lookup keys in maps with no btf available.
> > >
> > > Fix this by following the way used in dumping maps to allow to look up
> > > keys in no-btf maps, by which it decides whether and where to get the
> > > btf info according to the btf value type.
> > >
> > > Fixes: a19f93cfafdf ("libbpf: Add internal helper to load BTF data by FD")
> > > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> > > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> > > ---
> > >  tools/bpf/bpftool/map.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > > index cc530a229812..4fc772d66e3a 100644
> > > --- a/tools/bpf/bpftool/map.c
> > > +++ b/tools/bpf/bpftool/map.c
> > > @@ -1054,11 +1054,9 @@ static void print_key_value(struct bpf_map_info *info, void *key,
> > >         json_writer_t *btf_wtr;
> > >         struct btf *btf;
> > >
> > > -       btf = btf__load_from_kernel_by_id(info->btf_id);
> > > -       if (libbpf_get_error(btf)) {
> > > -               p_err("failed to get btf");
> > > +       btf = get_map_kv_btf(info);
> > > +       if (libbpf_get_error(btf))
> > 
> > See discussion in [0], it seems relevant.
> > 
> >   [0] https://lore.kernel.org/bpf/20220204225823.339548-3-jolsa@kernel.org/
> 
> I checked and this patch does not fix the problem for me,
> but looks like similar issue, do you have test case for this?
> 
> mine is to dump any no-btf map with -p option

anyway I think your change should go in separately,
I can send change below (v2 for [0] above) on top of yours

thanks,
jirka


---
 tools/bpf/bpftool/map.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index c66a3c979b7a..8562add7417d 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -805,29 +805,28 @@ static int maps_have_btf(int *fds, int nb_fds)
 
 static struct btf *btf_vmlinux;
 
-static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
+static int get_map_kv_btf(const struct bpf_map_info *info, struct btf **btf)
 {
-	struct btf *btf = NULL;
+	int err = 0;
 
 	if (info->btf_vmlinux_value_type_id) {
 		if (!btf_vmlinux) {
 			btf_vmlinux = libbpf_find_kernel_btf();
-			if (libbpf_get_error(btf_vmlinux))
+			err = libbpf_get_error(btf_vmlinux);
+			if (err) {
 				p_err("failed to get kernel btf");
+				return err;
+			}
 		}
-		return btf_vmlinux;
+		*btf = btf_vmlinux;
 	} else if (info->btf_value_type_id) {
-		int err;
-
-		btf = btf__load_from_kernel_by_id(info->btf_id);
-		err = libbpf_get_error(btf);
-		if (err) {
+		*btf = btf__load_from_kernel_by_id(info->btf_id);
+		err = libbpf_get_error(*btf);
+		if (err)
 			p_err("failed to get btf");
-			btf = ERR_PTR(err);
-		}
 	}
 
-	return btf;
+	return err;
 }
 
 static void free_map_kv_btf(struct btf *btf)
@@ -862,8 +861,7 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
 	prev_key = NULL;
 
 	if (wtr) {
-		btf = get_map_kv_btf(info);
-		err = libbpf_get_error(btf);
+		err = get_map_kv_btf(info, &btf);
 		if (err) {
 			goto exit_free;
 		}
@@ -1054,11 +1052,8 @@ static void print_key_value(struct bpf_map_info *info, void *key,
 	json_writer_t *btf_wtr;
 	struct btf *btf;
 
-	btf = btf__load_from_kernel_by_id(info->btf_id);
-	if (libbpf_get_error(btf)) {
-		p_err("failed to get btf");
+	if (get_map_kv_btf(info, &btf))
 		return;
-	}
 
 	if (json_output) {
 		print_entry_json(info, key, value, btf);
-- 
2.34.1


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

* Re: [PATCH bpf] bpftool: fix the error when lookup in no-btf maps
  2022-02-07 18:26     ` Jiri Olsa
@ 2022-02-07 20:34       ` Yonghong Song
  2022-02-08  4:53       ` Yinjun Zhang
  1 sibling, 0 replies; 8+ messages in thread
From: Yonghong Song @ 2022-02-07 20:34 UTC (permalink / raw)
  To: Jiri Olsa, Andrii Nakryiko
  Cc: Yinjun Zhang, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin Lau, Song Liu, john fastabend, KP Singh,
	niklas.soderlund, Simon Horman



On 2/7/22 10:26 AM, Jiri Olsa wrote:
> On Mon, Feb 07, 2022 at 06:57:20PM +0100, Jiri Olsa wrote:
>> On Mon, Feb 07, 2022 at 09:42:25AM -0800, Andrii Nakryiko wrote:
>>> On Mon, Feb 7, 2022 at 8:00 AM Yinjun Zhang <yinjun.zhang@corigine.com> wrote:
>>>>
>>>> When reworking btf__get_from_id() in commit a19f93cfafdf the error
>>>> handling when calling bpf_btf_get_fd_by_id() changed. Before the rework
>>>> if bpf_btf_get_fd_by_id() failed the error would not be propagated to
>>>> callers of btf__get_from_id(), after the rework it is. This lead to a
>>>> change in behavior in print_key_value() that now prints an error when
>>>> trying to lookup keys in maps with no btf available.
>>>>
>>>> Fix this by following the way used in dumping maps to allow to look up
>>>> keys in no-btf maps, by which it decides whether and where to get the
>>>> btf info according to the btf value type.
>>>>
>>>> Fixes: a19f93cfafdf ("libbpf: Add internal helper to load BTF data by FD")
>>>> Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
>>>> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>>>> ---
>>>>   tools/bpf/bpftool/map.c | 6 ++----
>>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>> index cc530a229812..4fc772d66e3a 100644
>>>> --- a/tools/bpf/bpftool/map.c
>>>> +++ b/tools/bpf/bpftool/map.c
>>>> @@ -1054,11 +1054,9 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>>>>          json_writer_t *btf_wtr;
>>>>          struct btf *btf;
>>>>
>>>> -       btf = btf__load_from_kernel_by_id(info->btf_id);
>>>> -       if (libbpf_get_error(btf)) {
>>>> -               p_err("failed to get btf");
>>>> +       btf = get_map_kv_btf(info);
>>>> +       if (libbpf_get_error(btf))
>>>
>>> See discussion in [0], it seems relevant.
>>>
>>>    [0] https://lore.kernel.org/bpf/20220204225823.339548-3-jolsa@kernel.org/

For the patch in the above link:

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index c66a3c979b7a..2ccf85042e75 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -862,6 +862,7 @@ map_dump(int fd, struct bpf_map_info *info, 
json_writer_t *wtr,
  	prev_key = NULL;

  	if (wtr) {
+		errno = 0;
  		btf = get_map_kv_btf(info);
  		err = libbpf_get_error(btf);
  		if (err) {
-- 

Do we know who sets non-zero errno in the above?
Maybe we can fix the issue in that place?

>>
>> I checked and this patch does not fix the problem for me,
>> but looks like similar issue, do you have test case for this?
>>
>> mine is to dump any no-btf map with -p option
> 
> anyway I think your change should go in separately,
> I can send change below (v2 for [0] above) on top of yours
> 
> thanks,
> jirka
> 
> 
> ---
>   tools/bpf/bpftool/map.c | 31 +++++++++++++------------------
>   1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index c66a3c979b7a..8562add7417d 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -805,29 +805,28 @@ static int maps_have_btf(int *fds, int nb_fds)
>   
>   static struct btf *btf_vmlinux;
>   
> -static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
> +static int get_map_kv_btf(const struct bpf_map_info *info, struct btf **btf)
>   {
> -	struct btf *btf = NULL;
> +	int err = 0;
>   
>   	if (info->btf_vmlinux_value_type_id) {
>   		if (!btf_vmlinux) {
>   			btf_vmlinux = libbpf_find_kernel_btf();
> -			if (libbpf_get_error(btf_vmlinux))
> +			err = libbpf_get_error(btf_vmlinux);
> +			if (err) {
>   				p_err("failed to get kernel btf");
> +				return err;
> +			}
>   		}
> -		return btf_vmlinux;
> +		*btf = btf_vmlinux;
>   	} else if (info->btf_value_type_id) {
> -		int err;
> -
> -		btf = btf__load_from_kernel_by_id(info->btf_id);
> -		err = libbpf_get_error(btf);
> -		if (err) {
> +		*btf = btf__load_from_kernel_by_id(info->btf_id);
> +		err = libbpf_get_error(*btf);
> +		if (err)
>   			p_err("failed to get btf");
> -			btf = ERR_PTR(err);
> -		}
>   	}
>   
> -	return btf;
> +	return err;
>   }
>   
>   static void free_map_kv_btf(struct btf *btf)
> @@ -862,8 +861,7 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
>   	prev_key = NULL;
>   
>   	if (wtr) {
> -		btf = get_map_kv_btf(info);
> -		err = libbpf_get_error(btf);
> +		err = get_map_kv_btf(info, &btf);
>   		if (err) {
>   			goto exit_free;
>   		}
> @@ -1054,11 +1052,8 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>   	json_writer_t *btf_wtr;
>   	struct btf *btf;
>   
> -	btf = btf__load_from_kernel_by_id(info->btf_id);
> -	if (libbpf_get_error(btf)) {
> -		p_err("failed to get btf");
> +	if (get_map_kv_btf(info, &btf))
>   		return;
> -	}
>   
>   	if (json_output) {
>   		print_entry_json(info, key, value, btf);

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

* Re: [PATCH bpf] bpftool: fix the error when lookup in no-btf maps
  2022-02-07 18:26     ` Jiri Olsa
  2022-02-07 20:34       ` Yonghong Song
@ 2022-02-08  4:53       ` Yinjun Zhang
  1 sibling, 0 replies; 8+ messages in thread
From: Yinjun Zhang @ 2022-02-08  4:53 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
	john fastabend, KP Singh, niklas.soderlund, Simon Horman

On Mon, Feb 07, 2022 at 07:26:29PM +0100, Jiri Olsa wrote:
> On Mon, Feb 07, 2022 at 06:57:20PM +0100, Jiri Olsa wrote:
> > On Mon, Feb 07, 2022 at 09:42:25AM -0800, Andrii Nakryiko wrote:
> > > On Mon, Feb 7, 2022 at 8:00 AM Yinjun Zhang <yinjun.zhang@corigine.com> wrote:
> > > >
> > > > When reworking btf__get_from_id() in commit a19f93cfafdf the error
> > > > handling when calling bpf_btf_get_fd_by_id() changed. Before the rework
> > > > if bpf_btf_get_fd_by_id() failed the error would not be propagated to
> > > > callers of btf__get_from_id(), after the rework it is. This lead to a
> > > > change in behavior in print_key_value() that now prints an error when
> > > > trying to lookup keys in maps with no btf available.
> > > >
> > > > Fix this by following the way used in dumping maps to allow to look up
> > > > keys in no-btf maps, by which it decides whether and where to get the
> > > > btf info according to the btf value type.
> > > >
> > > > Fixes: a19f93cfafdf ("libbpf: Add internal helper to load BTF data by FD")
> > > > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> > > > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> > > > ---
> > > >  tools/bpf/bpftool/map.c | 6 ++----
> > > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > > > index cc530a229812..4fc772d66e3a 100644
> > > > --- a/tools/bpf/bpftool/map.c
> > > > +++ b/tools/bpf/bpftool/map.c
> > > > @@ -1054,11 +1054,9 @@ static void print_key_value(struct bpf_map_info *info, void *key,
> > > >         json_writer_t *btf_wtr;
> > > >         struct btf *btf;
> > > >
> > > > -       btf = btf__load_from_kernel_by_id(info->btf_id);
> > > > -       if (libbpf_get_error(btf)) {
> > > > -               p_err("failed to get btf");
> > > > +       btf = get_map_kv_btf(info);
> > > > +       if (libbpf_get_error(btf))
> > > 
> > > See discussion in [0], it seems relevant.
> > > 
> > >   [0] https://lore.kernel.org/bpf/20220204225823.339548-3-jolsa@kernel.org/
> > 
> > I checked and this patch does not fix the problem for me,
> > but looks like similar issue, do you have test case for this?
> > 
> > mine is to dump any no-btf map with -p option
> 
> anyway I think your change should go in separately,
> I can send change below (v2 for [0] above) on top of yours
> 
> thanks,
> jirka
> 

Yeah, I agree they are separate issues, and my test case is also
simple, just lookup a key in a no-btf map, example:
# ./bpftool map create /sys/fs/bpf/xx type array key 4 value 4 entries 2 name def
# ./bpftool map lookup name def key 0 0 0 0
Error: failed to get btf

> 
> ---
>  tools/bpf/bpftool/map.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index c66a3c979b7a..8562add7417d 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -805,29 +805,28 @@ static int maps_have_btf(int *fds, int nb_fds)
>  
>  static struct btf *btf_vmlinux;
>  
> -static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
> +static int get_map_kv_btf(const struct bpf_map_info *info, struct btf **btf)
>  {
> -	struct btf *btf = NULL;
> +	int err = 0;
>  
>  	if (info->btf_vmlinux_value_type_id) {
>  		if (!btf_vmlinux) {
>  			btf_vmlinux = libbpf_find_kernel_btf();
> -			if (libbpf_get_error(btf_vmlinux))
> +			err = libbpf_get_error(btf_vmlinux);
> +			if (err) {
>  				p_err("failed to get kernel btf");
> +				return err;
> +			}
>  		}
> -		return btf_vmlinux;
> +		*btf = btf_vmlinux;
>  	} else if (info->btf_value_type_id) {
> -		int err;
> -
> -		btf = btf__load_from_kernel_by_id(info->btf_id);
> -		err = libbpf_get_error(btf);
> -		if (err) {
> +		*btf = btf__load_from_kernel_by_id(info->btf_id);
> +		err = libbpf_get_error(*btf);
> +		if (err)
>  			p_err("failed to get btf");
> -			btf = ERR_PTR(err);
> -		}
>  	}
>  
> -	return btf;
> +	return err;
>  }
>  
>  static void free_map_kv_btf(struct btf *btf)
> @@ -862,8 +861,7 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
>  	prev_key = NULL;
>  
>  	if (wtr) {
> -		btf = get_map_kv_btf(info);
> -		err = libbpf_get_error(btf);
> +		err = get_map_kv_btf(info, &btf);
>  		if (err) {
>  			goto exit_free;
>  		}
> @@ -1054,11 +1052,8 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>  	json_writer_t *btf_wtr;
>  	struct btf *btf;
>  
> -	btf = btf__load_from_kernel_by_id(info->btf_id);
> -	if (libbpf_get_error(btf)) {
> -		p_err("failed to get btf");
> +	if (get_map_kv_btf(info, &btf))
>  		return;
> -	}
>  
>  	if (json_output) {
>  		print_entry_json(info, key, value, btf);
> -- 
> 2.34.1
> 

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

* Re: [PATCH bpf] bpftool: fix the error when lookup in no-btf maps
  2022-02-07 16:00 [PATCH bpf] bpftool: fix the error when lookup in no-btf maps Yinjun Zhang
  2022-02-07 17:42 ` Andrii Nakryiko
@ 2022-02-15  9:47 ` Jiri Olsa
  2022-02-15 17:10 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2022-02-15  9:47 UTC (permalink / raw)
  To: Yinjun Zhang
  Cc: bpf, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, niklas.soderlund, Simon Horman

On Tue, Feb 08, 2022 at 12:00:25AM +0800, Yinjun Zhang wrote:
> When reworking btf__get_from_id() in commit a19f93cfafdf the error
> handling when calling bpf_btf_get_fd_by_id() changed. Before the rework
> if bpf_btf_get_fd_by_id() failed the error would not be propagated to
> callers of btf__get_from_id(), after the rework it is. This lead to a
> change in behavior in print_key_value() that now prints an error when
> trying to lookup keys in maps with no btf available.
> 
> Fix this by following the way used in dumping maps to allow to look up
> keys in no-btf maps, by which it decides whether and where to get the
> btf info according to the btf value type.
> 
> Fixes: a19f93cfafdf ("libbpf: Add internal helper to load BTF data by FD")
> Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>

once this one gets merged, I'd send this fix on top of that:
  https://lore.kernel.org/bpf/20220204225823.339548-3-jolsa@kernel.org/

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka


> ---
>  tools/bpf/bpftool/map.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index cc530a229812..4fc772d66e3a 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -1054,11 +1054,9 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>  	json_writer_t *btf_wtr;
>  	struct btf *btf;
>  
> -	btf = btf__load_from_kernel_by_id(info->btf_id);
> -	if (libbpf_get_error(btf)) {
> -		p_err("failed to get btf");
> +	btf = get_map_kv_btf(info);
> +	if (libbpf_get_error(btf))
>  		return;
> -	}
>  
>  	if (json_output) {
>  		print_entry_json(info, key, value, btf);
> -- 
> 1.8.3.1
> 


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

* Re: [PATCH bpf] bpftool: fix the error when lookup in no-btf maps
  2022-02-07 16:00 [PATCH bpf] bpftool: fix the error when lookup in no-btf maps Yinjun Zhang
  2022-02-07 17:42 ` Andrii Nakryiko
  2022-02-15  9:47 ` Jiri Olsa
@ 2022-02-15 17:10 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-02-15 17:10 UTC (permalink / raw)
  To: Yinjun Zhang
  Cc: bpf, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, niklas.soderlund, simon.horman

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Tue,  8 Feb 2022 00:00:25 +0800 you wrote:
> When reworking btf__get_from_id() in commit a19f93cfafdf the error
> handling when calling bpf_btf_get_fd_by_id() changed. Before the rework
> if bpf_btf_get_fd_by_id() failed the error would not be propagated to
> callers of btf__get_from_id(), after the rework it is. This lead to a
> change in behavior in print_key_value() that now prints an error when
> trying to lookup keys in maps with no btf available.
> 
> [...]

Here is the summary with links:
  - [bpf] bpftool: fix the error when lookup in no-btf maps
    https://git.kernel.org/bpf/bpf-next/c/edc21dc909c6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



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

end of thread, other threads:[~2022-02-15 17:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-07 16:00 [PATCH bpf] bpftool: fix the error when lookup in no-btf maps Yinjun Zhang
2022-02-07 17:42 ` Andrii Nakryiko
2022-02-07 17:57   ` Jiri Olsa
2022-02-07 18:26     ` Jiri Olsa
2022-02-07 20:34       ` Yonghong Song
2022-02-08  4:53       ` Yinjun Zhang
2022-02-15  9:47 ` Jiri Olsa
2022-02-15 17:10 ` patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.