Re: [PATCH] bpftool: fix a wrong type cast in btf_dumper_int

[John Fastabend <john.fastabend@gmail.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>