| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20230425095221.20805-1-kuro@kuroa.me>
Date: Tue, 25 Apr 2023 17:52:21 +0800
From: Xueming Feng <kuro@...oa.me>
To: quentin@...valent.com
Cc: andrii@...nel.org, ast@...nel.org, bpf@...r.kernel.org,
daniel@...earbox.net, haoluo@...gle.com, john.fastabend@...il.com,
jolsa@...nel.org, kpsingh@...nel.org, kuro@...oa.me,
linux-kernel@...r.kernel.org, martin.lau@...ux.dev, sdf@...gle.com,
song@...nel.org, yhs@...com, yhs@...a.com
Subject: Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types
>2023-04-25 14:37 UTC+0800 ~ Xueming Feng <kuro@...oa.me>
>>> On 4/24/23 9:10 PM, Xueming Feng wrote:
>>>>> On 4/24/23 2:09 AM, Xueming Feng wrote:
>>>>>> When using `bpftool map dump` in plain format, it is usually
>>>>>> more convenient to show the inner map id instead of raw value.
>>>>>> Changing this behavior would help with quick debugging with
>>>>>> `bpftool`, without disrupting scripted behavior. Since user
>>>>>> could dump the inner map with id, and need to convert value.
>>>>>>
>>>>>> Signed-off-by: Xueming Feng <kuro@...oa.me>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> - Fix commit message grammar.
>>>>>> - Change `print_uint` to only print to stdout, make `arg` const, and rename
>>>>>> `n` to `arg_size`.
>>>>>> - Make `print_uint` able to take any size of argument up to `unsigned long`,
>>>>>> and print it as unsigned decimal.
>>>>>>
>>>>>> Thanks for the review and suggestions! I have changed my patch accordingly.
>>>>>> There is a possibility that `arg_size` is larger than `unsigned long`,
>>>>>> but previous review suggested that it should be up to the caller function to
>>>>>> set `arg_size` correctly. So I didn't add check for that, should I?
>>>>>>
>>>>>> tools/bpf/bpftool/main.c | 15 +++++++++++++++
>>>>>> tools/bpf/bpftool/main.h | 1 +
>>>>>> tools/bpf/bpftool/map.c | 9 +++++++--
>>>>>> 3 files changed, 23 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
>>>>>> index 08d0ac543c67..810c0dc10ecb 100644
>>>>>> --- a/tools/bpf/bpftool/main.c
>>>>>> +++ b/tools/bpf/bpftool/main.c
>>>>>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +void print_uint(const void *arg, unsigned int arg_size)
>>>>>> +{
>>>>>> + const unsigned char *data = arg;
>>>>>> + unsigned long val = 0ul;
>>>>>> +
>>>>>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>>> + memcpy(&val, data, arg_size);
>>>>>> + #else
>>>>>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size,
>>>>>> + data, arg_size);
>>>>>> + #endif
>>>>>> +
>>>>>> + fprintf(stdout, "%lu", val);
>>>>>> +}
>>>>>> +
>>>>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
>>>>>> {
>>>>>> unsigned char *data = arg;
>>>>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>>>>> index 0ef373cef4c7..0de671423431 100644
>>>>>> --- a/tools/bpf/bpftool/main.h
>>>>>> +++ b/tools/bpf/bpftool/main.h
>>>>>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...);
>>>>>>
>>>>>> bool is_prefix(const char *pfx, const char *str);
>>>>>> int detect_common_prefix(const char *arg, ...);
>>>>>> +void print_uint(const void *arg, unsigned int arg_size);
>>>>>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
>>>>>> void usage(void) __noreturn;
>>>>>>
>>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>>>> index aaeb8939e137..f5be4c0564cf 100644
>>>>>> --- a/tools/bpf/bpftool/map.c
>>>>>> +++ b/tools/bpf/bpftool/map.c
>>>>>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>>>>>> }
>>>>>>
>>>>>> if (info->value_size) {
>>>>>> - printf("value:%c", break_names ? '\n' : ' ');
>>>>>> - fprint_hex(stdout, value, info->value_size, " ");
>>>>>> + if (map_is_map_of_maps(info->type)) {
>>>>>> + printf("id:%c", break_names ? '\n' : ' ');
>>>>> 1> + print_uint(value, info->value_size);
>>>>
>>>> On Mon, 24 Apr 2023 18:07:27 -0700, Yonghong Song wrote:
>>>>> For all map_in_map types, the inner map value size is 32bit int which
>>>>> represents a fd (for map creation) and a id (for map info), e.g., in
>>>>> show_prog_maps() in prog.c. So maybe we can simplify the code as below:
>>>>> printf("id: %u", *(unsigned int *)value);
>>>>
>>>> That is true, maybe the "id" could also be changed to "map_id" to follow the
>>>> convention. Do you think that `print_uint` could be useful in the future?
>>>> If that is the case, should I keep using it here as an example usage, and to
>>>> avoid dead code? Or should I just remove it?
On Tue, 25 Apr 2023 09:57:17 +0100, Quentin Monnet wrote:
> This makes me think we could also have something similar for prog_array
> maps (but not necessarily as part of your patchset).
Good idea, will take a look at that after finishing v3 patch, and possibly
make a separate patch for it. (This is my first time contributing, better keep
it simple).
>> On Mon, 24 Apr 2023 22:58:10 -0700, Yonghong Song wrote:
>>> Maybe, "inner_map_id" is a better choice. For array of maps, some array
>>> element value could be 0, implying "inner_map_id 0", but I think it is
>>> okay, people should know a real inner_map_id (or any map_id) should
>>> never be 0.
>>>
>>> Function "print_uint" is not needed any more. Please remove it.
>>
>> Will reflect this in v3.
>>
>>>
>>> Please add the command line to dump map values triggering the above
>>> change, also the actual dumps with and without this patch.
>>
>> $ bpftool map dump id 138
>> Without patch:
>> ```
>> key:
>> fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05
>> 27 16 06 00
>> value:
>> 8b 00 00 00
>> Found 1 element
>> ```
>> With patch:
>> ```
>> key:
>> fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05
>> 27 16 06 00
>> inner_map_id:
>> 139
>> Found 1 element
>> ```
> Thanks! Please add those sample outputs to the commit description for
> v3. Can you please also add an example with JSON ("-p")?
Sure, will add them in v3! About json output, they are currently not touched
to avoid breaking scripts. I will add inner_map_id as a new field in v3 like the
following patch.
https://lore.kernel.org/bpf/20230419003651.988865-1-kuifeng@meta.com/
> Quentin
Powered by blists - more mailing lists