| Message ID | 20200318031437.1256423-1-kafai@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpftool: Add struct_ops support | expand |
On Tue, Mar 17, 2020 at 8:15 PM Martin KaFai Lau <kafai@fb.com> wrote: > > This patch prints the enum's name if there is one found in > the array of btf_enum. > > The commit 9eea98497951 ("bpf: fix BTF verification of enums") > has details about an enum could have any power-of-2 size (up to 8 bytes). > This patch also takes this chance to accommodate these non 4 byte > enums. > > Acked-by: Quentin Monnet <quentin@isovalent.com> > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > tools/bpf/bpftool/btf_dumper.c | 39 +++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c > index 01cc52b834fa..079f9171b1a3 100644 > --- a/tools/bpf/bpftool/btf_dumper.c > +++ b/tools/bpf/bpftool/btf_dumper.c > @@ -43,9 +43,42 @@ static int btf_dumper_modifier(const struct btf_dumper *d, __u32 type_id, > return btf_dumper_do_type(d, actual_type_id, bit_offset, data); > } > > -static void btf_dumper_enum(const void *data, json_writer_t *jw) > +static void btf_dumper_enum(const struct btf_dumper *d, > + const struct btf_type *t, > + const void *data) > { > - jsonw_printf(jw, "%d", *(int *)data); > + const struct btf_enum *enums = btf_enum(t); > + __s64 value; > + __u16 i; > + > + switch (t->size) { > + case 8: > + value = *(__s64 *)data; > + break; > + case 4: > + value = *(__s32 *)data; > + break; > + case 2: > + value = *(__s16 *)data; > + break; > + case 1: > + value = *(__s8 *)data; > + break; > + default: > + jsonw_string(d->jw, "<invalid_enum_size>"); Why not return error and let it propagate, similar to how btf_dumper_array() can return an error? BTF is malformed if this happened, so there is no point in continuing dumping, it's most probably going to be a garbage. > + return; > + } > + > + for (i = 0; i < btf_vlen(t); i++) { > + if (value == enums[i].val) { > + jsonw_string(d->jw, > + btf__name_by_offset(d->btf, > + enums[i].name_off)); nit: local variable will make it cleaner > + return; > + } > + } > + > + jsonw_int(d->jw, value); > } > > static int btf_dumper_array(const struct btf_dumper *d, __u32 type_id, > @@ -366,7 +399,7 @@ static int btf_dumper_do_type(const struct btf_dumper *d, __u32 type_id, > case BTF_KIND_ARRAY: > return btf_dumper_array(d, type_id, data); > case BTF_KIND_ENUM: > - btf_dumper_enum(data, d->jw); > + btf_dumper_enum(d, t, data); > return 0; > case BTF_KIND_PTR: > btf_dumper_ptr(data, d->jw, d->is_plain_text); > -- > 2.17.1 >
On Tue, Mar 17, 2020 at 11:09:24PM -0700, Andrii Nakryiko wrote: > On Tue, Mar 17, 2020 at 8:15 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > This patch prints the enum's name if there is one found in > > the array of btf_enum. > > > > The commit 9eea98497951 ("bpf: fix BTF verification of enums") > > has details about an enum could have any power-of-2 size (up to 8 bytes). > > This patch also takes this chance to accommodate these non 4 byte > > enums. > > > > Acked-by: Quentin Monnet <quentin@isovalent.com> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > tools/bpf/bpftool/btf_dumper.c | 39 +++++++++++++++++++++++++++++++--- > > 1 file changed, 36 insertions(+), 3 deletions(-) > > > > diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c > > index 01cc52b834fa..079f9171b1a3 100644 > > --- a/tools/bpf/bpftool/btf_dumper.c > > +++ b/tools/bpf/bpftool/btf_dumper.c > > @@ -43,9 +43,42 @@ static int btf_dumper_modifier(const struct btf_dumper *d, __u32 type_id, > > return btf_dumper_do_type(d, actual_type_id, bit_offset, data); > > } > > > > -static void btf_dumper_enum(const void *data, json_writer_t *jw) > > +static void btf_dumper_enum(const struct btf_dumper *d, > > + const struct btf_type *t, > > + const void *data) > > { > > - jsonw_printf(jw, "%d", *(int *)data); > > + const struct btf_enum *enums = btf_enum(t); > > + __s64 value; > > + __u16 i; > > + > > + switch (t->size) { > > + case 8: > > + value = *(__s64 *)data; > > + break; > > + case 4: > > + value = *(__s32 *)data; > > + break; > > + case 2: > > + value = *(__s16 *)data; > > + break; > > + case 1: > > + value = *(__s8 *)data; > > + break; > > + default: > > + jsonw_string(d->jw, "<invalid_enum_size>"); > > Why not return error and let it propagate, similar to how > btf_dumper_array() can return an error? BTF is malformed if this > happened, so there is no point in continuing dumping, it's most > probably going to be a garbage. I can send v4 to return -EINVAL here. However, the caller of btf_dump*() is pretty loose on checking it. It won't be difficult to find other existing codes that will continue on btf_type's related error cases. I also don't think fixing all these error checking/returning is the right answer here The proper place to check malformed BTF is in btf__new(). Check it once there like how the kernel does. [ btw, the data and btf here are obtained from the kernel which has verified it ]. > > > + return; > > + } > > + > > + for (i = 0; i < btf_vlen(t); i++) { > > + if (value == enums[i].val) { > > + jsonw_string(d->jw, > > + btf__name_by_offset(d->btf, > > + enums[i].name_off)); > > nit: local variable will make it cleaner I prefer to keep it as is. There are many other uses like this. > > > + return; > > + } > > + } > > + > > + jsonw_int(d->jw, value); > > } > > > > static int btf_dumper_array(const struct btf_dumper *d, __u32 type_id, > > @@ -366,7 +399,7 @@ static int btf_dumper_do_type(const struct btf_dumper *d, __u32 type_id, > > case BTF_KIND_ARRAY: > > return btf_dumper_array(d, type_id, data); > > case BTF_KIND_ENUM: > > - btf_dumper_enum(data, d->jw); > > + btf_dumper_enum(d, t, data); > > return 0; > > case BTF_KIND_PTR: > > btf_dumper_ptr(data, d->jw, d->is_plain_text); > > -- > > 2.17.1 > >
On Wed, Mar 18, 2020 at 10:10 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Tue, Mar 17, 2020 at 11:09:24PM -0700, Andrii Nakryiko wrote: > > On Tue, Mar 17, 2020 at 8:15 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > This patch prints the enum's name if there is one found in > > > the array of btf_enum. > > > > > > The commit 9eea98497951 ("bpf: fix BTF verification of enums") > > > has details about an enum could have any power-of-2 size (up to 8 bytes). > > > This patch also takes this chance to accommodate these non 4 byte > > > enums. > > > > > > Acked-by: Quentin Monnet <quentin@isovalent.com> > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > --- > > > tools/bpf/bpftool/btf_dumper.c | 39 +++++++++++++++++++++++++++++++--- > > > 1 file changed, 36 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c > > > index 01cc52b834fa..079f9171b1a3 100644 > > > --- a/tools/bpf/bpftool/btf_dumper.c > > > +++ b/tools/bpf/bpftool/btf_dumper.c > > > @@ -43,9 +43,42 @@ static int btf_dumper_modifier(const struct btf_dumper *d, __u32 type_id, > > > return btf_dumper_do_type(d, actual_type_id, bit_offset, data); > > > } > > > > > > -static void btf_dumper_enum(const void *data, json_writer_t *jw) > > > +static void btf_dumper_enum(const struct btf_dumper *d, > > > + const struct btf_type *t, > > > + const void *data) > > > { > > > - jsonw_printf(jw, "%d", *(int *)data); > > > + const struct btf_enum *enums = btf_enum(t); > > > + __s64 value; > > > + __u16 i; > > > + > > > + switch (t->size) { > > > + case 8: > > > + value = *(__s64 *)data; > > > + break; > > > + case 4: > > > + value = *(__s32 *)data; > > > + break; > > > + case 2: > > > + value = *(__s16 *)data; > > > + break; > > > + case 1: > > > + value = *(__s8 *)data; > > > + break; > > > + default: > > > + jsonw_string(d->jw, "<invalid_enum_size>"); > > > > Why not return error and let it propagate, similar to how > > btf_dumper_array() can return an error? BTF is malformed if this > > happened, so there is no point in continuing dumping, it's most > > probably going to be a garbage. > I can send v4 to return -EINVAL here. > > However, the caller of btf_dump*() is pretty loose on checking it. > It won't be difficult to find other existing codes that will > continue on btf_type's related error cases. I also don't > think fixing all these error checking/returning is the > right answer here Disagree about fixing error checking in general, but I don't insist either. > > The proper place to check malformed BTF is in btf__new(). I agree. > Check it once there like how the kernel does. > [ btw, the data and btf here are obtained from the kernel > which has verified it ]. > > > > > > + return; > > > + } > > > + > > > + for (i = 0; i < btf_vlen(t); i++) { > > > + if (value == enums[i].val) { > > > + jsonw_string(d->jw, > > > + btf__name_by_offset(d->btf, > > > + enums[i].name_off)); > > > > nit: local variable will make it cleaner > I prefer to keep it as is. There are many other uses like this. Ok. > > > > > > + return; > > > + } > > > + } > > > + > > > + jsonw_int(d->jw, value); > > > } > > > > > > static int btf_dumper_array(const struct btf_dumper *d, __u32 type_id, > > > @@ -366,7 +399,7 @@ static int btf_dumper_do_type(const struct btf_dumper *d, __u32 type_id, > > > case BTF_KIND_ARRAY: > > > return btf_dumper_array(d, type_id, data); > > > case BTF_KIND_ENUM: > > > - btf_dumper_enum(data, d->jw); > > > + btf_dumper_enum(d, t, data); > > > return 0; > > > case BTF_KIND_PTR: > > > btf_dumper_ptr(data, d->jw, d->is_plain_text); > > > -- > > > 2.17.1 > > >
diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c index 01cc52b834fa..079f9171b1a3 100644 --- a/tools/bpf/bpftool/btf_dumper.c +++ b/tools/bpf/bpftool/btf_dumper.c @@ -43,9 +43,42 @@ static int btf_dumper_modifier(const struct btf_dumper *d, __u32 type_id, return btf_dumper_do_type(d, actual_type_id, bit_offset, data); } -static void btf_dumper_enum(const void *data, json_writer_t *jw) +static void btf_dumper_enum(const struct btf_dumper *d, + const struct btf_type *t, + const void *data) { - jsonw_printf(jw, "%d", *(int *)data); + const struct btf_enum *enums = btf_enum(t); + __s64 value; + __u16 i; + + switch (t->size) { + case 8: + value = *(__s64 *)data; + break; + case 4: + value = *(__s32 *)data; + break; + case 2: + value = *(__s16 *)data; + break; + case 1: + value = *(__s8 *)data; + break; + default: + jsonw_string(d->jw, "<invalid_enum_size>"); + return; + } + + for (i = 0; i < btf_vlen(t); i++) { + if (value == enums[i].val) { + jsonw_string(d->jw, + btf__name_by_offset(d->btf, + enums[i].name_off)); + return; + } + } + + jsonw_int(d->jw, value); } static int btf_dumper_array(const struct btf_dumper *d, __u32 type_id, @@ -366,7 +399,7 @@ static int btf_dumper_do_type(const struct btf_dumper *d, __u32 type_id, case BTF_KIND_ARRAY: return btf_dumper_array(d, type_id, data); case BTF_KIND_ENUM: - btf_dumper_enum(data, d->jw); + btf_dumper_enum(d, t, data); return 0; case BTF_KIND_PTR: btf_dumper_ptr(data, d->jw, d->is_plain_text);
This patch prints the enum's name if there is one found in the array of btf_enum. The commit 9eea98497951 ("bpf: fix BTF verification of enums") has details about an enum could have any power-of-2 size (up to 8 bytes). This patch also takes this chance to accommodate these non 4 byte enums. Acked-by: Quentin Monnet <quentin@isovalent.com> Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- tools/bpf/bpftool/btf_dumper.c | 39 +++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)