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

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

[Lam Thai]

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(-)

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

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

[John Fastabend]

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
> 

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

[Quentin Monnet]

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>

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

[John Fastabend]

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>

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

[Andrii Nakryiko]

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>

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html