Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6353e12d-6fe6-f42b-4277-b32e2b2268a8@gmail.com>
Date:   Mon, 24 Apr 2023 22:19:52 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for
 map_of_maps types
Content-Language: en-US
To:     Xueming Feng <kuro@kuroa.me>
Cc:     andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org,
        daniel@iogearbox.net, haoluo@google.com, john.fastabend@gmail.com,
        jolsa@kernel.org, kpsingh@kernel.org, linux-kernel@vger.kernel.org,
        martin.lau@linux.dev, quentin@isovalent.com, sdf@google.com,
        song@kernel.org, yhs@fb.com
References: <f804fd67-87a9-4762-7e31-23abacdf6086@gmail.com>
 <20230425035803.49919-1-kuro@kuroa.me>
From:   Kui-Feng Lee <sinquersw@gmail.com>
In-Reply-To: <20230425035803.49919-1-kuro@kuroa.me>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 4/24/23 20:58, Xueming Feng wrote:
>> On 4/24/23 02:09, 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@kuroa.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
> 
> On Mon, 24 Apr 2023 09:44:18 -0700, Kui-Feng Lee wrote:
>> Is it possible that arg_size is bigger than sizeof(val)?
> 
> Yes it is possible, I had the thought of adding a check. But as I mentioned
> before the diff section, previous review
> https://lore.kernel.org/bpf/20230421101154.23690-1-kuro@kuroa.me/ suggested that
> I should leave it to the caller function to behave. If I were to add a check,
> what action do you recommend if the check fails? Print a '-1', do nothing,
> or just use the first sizeof(val) bytes?

In the previous patch, it may have integer overflow, but it is never 
buffer underrun.  This version uses memcpy and may cause buffer underrun 
if arg_size is bigger than sizeof(val).  I would say that at least 
prevent buffer underrun from happening.

> 
>>> +
>>> +	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' : ' ');
>>> +				print_uint(value, info->value_size);
>>> +			} else {
>>> +				printf("value:%c", break_names ? '\n' : ' ');
>>> +				fprint_hex(stdout, value, info->value_size, " ");
>>> +			}
>>>    		}
>>>    
>>>    		printf("\n");