| Message ID | 20220824225859.9038-1-lamthai@arista.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7184aef9c0f7a81db8fd18d183ee42481d89bf35 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpftool: fix a wrong type cast in btf_dumper_int | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
Lam Thai wrote: > When `data` points to a boolean value, casting it to `int *` is problematic > and could lead to a wrong value being passed to `jsonw_bool`. Change the > cast to `bool *` instead. How is it problematic? Its from BTF_KIND_INT by my quick reading. > > Fixes: b12d6ec09730 ("bpf: btf: add btf print functionality") > Signed-off-by: Lam Thai <lamthai@arista.com> > --- for bpf-next looks like a nice cleanup, I don't think its needed for bpf tree? > tools/bpf/bpftool/btf_dumper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c > index 125798b0bc5d..19924b6ce796 100644 > --- a/tools/bpf/bpftool/btf_dumper.c > +++ b/tools/bpf/bpftool/btf_dumper.c > @@ -452,7 +452,7 @@ static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset, > *(char *)data); > break; > case BTF_INT_BOOL: > - jsonw_bool(jw, *(int *)data); > + jsonw_bool(jw, *(bool *)data); > break; > default: > /* shouldn't happen */ > > base-commit: 6fc2838b148f8fe6aa14fc435e666984a0505018 > -- > 2.37.0 >
On 25/08/2022 07:29, John Fastabend wrote: > Lam Thai wrote: >> When `data` points to a boolean value, casting it to `int *` is problematic >> and could lead to a wrong value being passed to `jsonw_bool`. Change the >> cast to `bool *` instead. > > How is it problematic? Its from BTF_KIND_INT by my quick reading. Hi John, it's an INT but it also has a size of 1: struct map_value { int a; int b; short c; bool d; }; # bpftool btf dump id 1107 [...] [2] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED [...] [12] STRUCT 'map_value' size=12 vlen=4 'a' type_id=2 bits_offset=0 'b' type_id=2 bits_offset=32 'c' type_id=13 bits_offset=64 'd' type_id=14 bits_offset=80 [13] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED [14] INT '_Bool' size=1 bits_offset=0 nr_bits=8 encoding=BOOL [...] And Lam reported [0] that the pretty-print for the map does not display the correct boolean value, because it reads too many bytes from this *(int *)data. # bpftool map dump name my_map --pretty [{ "key": ["0x00","0x00","0x00","0x00" ], "value": ["0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00" ], "formatted": { "key": 0, "value": { "a": 0, "b": 0, "c": 0, "d": true } } } ] The above is before the map gets any update. The bytes in "value" look correct, but "d" says "true" when it should be "false". So bpf tree would make sense to me. [0] https://github.com/libbpf/bpftool/issues/38 > >> >> Fixes: b12d6ec09730 ("bpf: btf: add btf print functionality") >> Signed-off-by: Lam Thai <lamthai@arista.com> >> --- > > for bpf-next looks like a nice cleanup, I don't think its needed for bpf > tree? > >> tools/bpf/bpftool/btf_dumper.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c >> index 125798b0bc5d..19924b6ce796 100644 >> --- a/tools/bpf/bpftool/btf_dumper.c >> +++ b/tools/bpf/bpftool/btf_dumper.c >> @@ -452,7 +452,7 @@ static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset, >> *(char *)data); >> break; >> case BTF_INT_BOOL: >> - jsonw_bool(jw, *(int *)data); >> + jsonw_bool(jw, *(bool *)data); Looks good, thanks Reviewed-by: Quentin Monnet <quentin@isovalent.com>
Quentin Monnet wrote: > On 25/08/2022 07:29, John Fastabend wrote: > > Lam Thai wrote: > >> When `data` points to a boolean value, casting it to `int *` is problematic > >> and could lead to a wrong value being passed to `jsonw_bool`. Change the > >> cast to `bool *` instead. > > > > How is it problematic? Its from BTF_KIND_INT by my quick reading. > > Hi John, it's an INT but it also has a size of 1: > > struct map_value { > int a; > int b; > short c; > bool d; > }; > > # bpftool btf dump id 1107 > [...] > [2] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED > [...] > [12] STRUCT 'map_value' size=12 vlen=4 > 'a' type_id=2 bits_offset=0 > 'b' type_id=2 bits_offset=32 > 'c' type_id=13 bits_offset=64 > 'd' type_id=14 bits_offset=80 > [13] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED > [14] INT '_Bool' size=1 bits_offset=0 nr_bits=8 encoding=BOOL > [...] > > And Lam reported [0] that the pretty-print for the map does not display > the correct boolean value, because it reads too many bytes from this > *(int *)data. > > # bpftool map dump name my_map --pretty > [{ > "key": ["0x00","0x00","0x00","0x00" > ], > "value": > ["0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00" > ], > "formatted": { > "key": 0, > "value": { > "a": 0, > "b": 0, > "c": 0, > "d": true > } > } > } > ] > > The above is before the map gets any update. The bytes in "value" look > correct, but "d" says "true" when it should be "false". So bpf tree > would make sense to me. > > [0] https://github.com/libbpf/bpftool/issues/38 Thanks for the explanation. It would be nice to add the above in the commit message. Otherwise though. Acked-by: John Fastabend <john.fastabend@gmail.com> > > > > >> > >> Fixes: b12d6ec09730 ("bpf: btf: add btf print functionality") > >> Signed-off-by: Lam Thai <lamthai@arista.com> > >> --- > > > > for bpf-next looks like a nice cleanup, I don't think its needed for bpf > > tree? > > > >> tools/bpf/bpftool/btf_dumper.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c > >> index 125798b0bc5d..19924b6ce796 100644 > >> --- a/tools/bpf/bpftool/btf_dumper.c > >> +++ b/tools/bpf/bpftool/btf_dumper.c > >> @@ -452,7 +452,7 @@ static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset, > >> *(char *)data); > >> break; > >> case BTF_INT_BOOL: > >> - jsonw_bool(jw, *(int *)data); > >> + jsonw_bool(jw, *(bool *)data); > > Looks good, thanks > Reviewed-by: Quentin Monnet <quentin@isovalent.com>
On Thu, Aug 25, 2022 at 1:57 AM Quentin Monnet <quentin@isovalent.com> wrote: > > On 25/08/2022 07:29, John Fastabend wrote: > > Lam Thai wrote: > >> When `data` points to a boolean value, casting it to `int *` is problematic > >> and could lead to a wrong value being passed to `jsonw_bool`. Change the > >> cast to `bool *` instead. > > > > How is it problematic? Its from BTF_KIND_INT by my quick reading. > > Hi John, it's an INT but it also has a size of 1: > > struct map_value { > int a; > int b; > short c; > bool d; > }; > > # bpftool btf dump id 1107 > [...] > [2] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED > [...] > [12] STRUCT 'map_value' size=12 vlen=4 > 'a' type_id=2 bits_offset=0 > 'b' type_id=2 bits_offset=32 > 'c' type_id=13 bits_offset=64 > 'd' type_id=14 bits_offset=80 > [13] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED > [14] INT '_Bool' size=1 bits_offset=0 nr_bits=8 encoding=BOOL > [...] > > And Lam reported [0] that the pretty-print for the map does not display > the correct boolean value, because it reads too many bytes from this > *(int *)data. > > # bpftool map dump name my_map --pretty > [{ > "key": ["0x00","0x00","0x00","0x00" > ], > "value": > ["0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00","0x00" > ], > "formatted": { > "key": 0, > "value": { > "a": 0, > "b": 0, > "c": 0, > "d": true > } > } > } > ] > > The above is before the map gets any update. The bytes in "value" look > correct, but "d" says "true" when it should be "false". So bpf tree > would make sense to me. That code is back from 2018. Alexei recently clarified that bpf is for hi-pri and urgent fixes. I don't think this one classifies as such. Plus bpftool itself should be packaged from github mirror, so this fix will make it there fast. Applied to bpf-next. > > [0] https://github.com/libbpf/bpftool/issues/38 > > > > >> > >> Fixes: b12d6ec09730 ("bpf: btf: add btf print functionality") > >> Signed-off-by: Lam Thai <lamthai@arista.com> > >> --- > > > > for bpf-next looks like a nice cleanup, I don't think its needed for bpf > > tree? > > > >> tools/bpf/bpftool/btf_dumper.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c > >> index 125798b0bc5d..19924b6ce796 100644 > >> --- a/tools/bpf/bpftool/btf_dumper.c > >> +++ b/tools/bpf/bpftool/btf_dumper.c > >> @@ -452,7 +452,7 @@ static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset, > >> *(char *)data); > >> break; > >> case BTF_INT_BOOL: > >> - jsonw_bool(jw, *(int *)data); > >> + jsonw_bool(jw, *(bool *)data); > > Looks good, thanks > Reviewed-by: Quentin Monnet <quentin@isovalent.com>
Hello: This patch was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Wed, 24 Aug 2022 15:59:00 -0700 you wrote: > When `data` points to a boolean value, casting it to `int *` is problematic > and could lead to a wrong value being passed to `jsonw_bool`. Change the > cast to `bool *` instead. > > Fixes: b12d6ec09730 ("bpf: btf: add btf print functionality") > Signed-off-by: Lam Thai <lamthai@arista.com> > > [...] Here is the summary with links: - bpftool: fix a wrong type cast in btf_dumper_int https://git.kernel.org/bpf/bpf-next/c/7184aef9c0f7 You are awesome, thank you!
diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c index 125798b0bc5d..19924b6ce796 100644 --- a/tools/bpf/bpftool/btf_dumper.c +++ b/tools/bpf/bpftool/btf_dumper.c @@ -452,7 +452,7 @@ static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset, *(char *)data); break; case BTF_INT_BOOL: - jsonw_bool(jw, *(int *)data); + jsonw_bool(jw, *(bool *)data); break; default: /* shouldn't happen */
When `data` points to a boolean value, casting it to `int *` is problematic and could lead to a wrong value being passed to `jsonw_bool`. Change the cast to `bool *` instead. Fixes: b12d6ec09730 ("bpf: btf: add btf print functionality") Signed-off-by: Lam Thai <lamthai@arista.com> --- tools/bpf/bpftool/btf_dumper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 6fc2838b148f8fe6aa14fc435e666984a0505018