netdev - Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54933018-813c-da62-7c8c-1eb9eca173f5@lab.ntt.co.jp>
Date:   Thu, 20 Sep 2018 11:58:39 +0900
From:   Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Quentin Monnet <quentin.monnet@...ronome.com>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup
 is failed



On 9/20/2018 12:29 AM, Jakub Kicinski wrote:
> On Wed, 19 Sep 2018 16:51:43 +0900, Prashant Bhole wrote:
>> Let's add a check for EOPNOTSUPP error when map lookup is failed.
>> Also in case map doesn't support lookup, the output of map dump is
>> changed from "can't lookup element" to "lookup not supported for
>> this map".
>>
>> Patch adds function print_entry_error() function to print the error
>> value.
>>
>> Following example dumps a map which does not support lookup.
>>
>> Output before:
>> root# bpftool map -jp dump id 40
>> [
>>      "key": ["0x0a","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "can\'t lookup element"
>>      },
>>      "key": ["0x0b","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "can\'t lookup element"
>>      }
>> ]
>>
>> root# bpftool map dump id 40
>> can't lookup element with key:
>> 0a 00 00 00
>> can't lookup element with key:
>> 0b 00 00 00
>> Found 0 elements
>>
>> Output after changes:
>> root# bpftool map dump -jp  id 45
>> [
>>      "key": ["0x0a","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "lookup not supported for this map"
>>      },
>>      "key": ["0x0b","0x00","0x00","0x00"
>>      ],
>>      "value": {
>>          "error": "lookup not supported for this map"
>>      }
>> ]
>>
>> root# bpftool map dump id 45
>> key:
>> 0a 00 00 00
>> value:
>> lookup not supported for this map
>> key:
>> 0b 00 00 00
>> value:
>> lookup not supported for this map
>> Found 0 elements
> 
> Nice improvement, thanks for the changes!  I wonder what your thoughts
> would be on just printing some form of "lookup not supported for this
> map" only once?  It seems slightly like repeated information - if
> lookup is not supported for one key it likely won't be for other keys
> too, so we could shorten the output.  Would that make sense?

I too thought that the message is repeated information. One idea was as 
you said, stop iterating once we get EOPNOTSUPP. But only reason for 
keeping this way is that user will at least see dump of keys along with 
it. Also it will be consistent with Json output. What is your opinion on it?

I will fix other things that you pointed out. Thanks!

-Prashant

> 
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
>> ---
>>   tools/bpf/bpftool/main.h |  5 +++++
>>   tools/bpf/bpftool/map.c  | 35 ++++++++++++++++++++++++++++++-----
>>   2 files changed, 35 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>> index 40492cdc4e53..1a8c683f949b 100644
>> --- a/tools/bpf/bpftool/main.h
>> +++ b/tools/bpf/bpftool/main.h
>> @@ -46,6 +46,11 @@
>>   
>>   #include "json_writer.h"
>>   
>> +#define ERR_CANNOT_LOOKUP \
>> +	"can't lookup element"
>> +#define ERR_LOOKUP_NOT_SUPPORTED \
>> +	"lookup not supported for this map"
> 
> Do we need these?  Are we going to reused them in more parts of the
> code?
> 
>>   #define ptr_to_u64(ptr)	((__u64)(unsigned long)(ptr))
>>   
>>   #define NEXT_ARG()	({ argc--; argv++; if (argc < 0) usage(); })
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index 284e12a289c0..2faccd2098c9 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -333,6 +333,25 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>>   	jsonw_end_object(json_wtr);
>>   }
>>   
>> +static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
>> +			      char *value)
>> +{
>> +	bool single_line, break_names;
>> +	int value_size = strlen(value);
> 
> nit: order variables declaration lines to shortest, please.
> 
>> +
>> +	break_names = info->key_size > 16 || value_size > 16;
>> +	single_line = info->key_size + value_size <= 24 && !break_names;
>> +
>> +	printf("key:%c", break_names ? '\n' : ' ');
>> +	fprint_hex(stdout, key, info->key_size, " ");
>> +
>> +	printf(single_line ? "  " : "\n");
>> +
>> +	printf("value:%c%s", break_names ? '\n' : ' ', value);
>> +
>> +	printf("\n");
>> +}
>> +
>>   static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
>>   			      unsigned char *value)
>>   {
>> @@ -660,6 +679,8 @@ static int dump_map_elem(int fd, void *key, void *value,
>>   			 json_writer_t *btf_wtr)
>>   {
>>   	int num_elems = 0;
>> +	int lookup_errno;
>> +	char *errstr;
> 
> nit: const char?
> 
>>   
>>   	if (!bpf_map_lookup_elem(fd, key, value)) {
>>   		if (json_output) {
> 
> 
>