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@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>
