On Thu, Aug 25, 2022 at 1:57 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >
> > 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@xxxxxxxxxxxxx>
