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: <6fb405e9-fe02-17c6-90e6-cf9c73ca7693@lab.ntt.co.jp>
Date:   Thu, 20 Sep 2018 14:04:19 +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?
> 
>> 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?

These are used only once. These can be used in do_lookup(). Currently 
do_lookup() prints strerror(errno) when lookup is failed. Shall I change 
that do_lookup() output?

-Prashant