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] [day] [month] [year] [list]

Message-ID: <7d104e2a-2292-3fc3-b31a-2f03e987aa5e@lab.ntt.co.jp>
Date:   Tue, 2 Oct 2018 14:33:40 +0900
From:   Prashant Bhole <bhole_prashant_q7@....ntt.co.jp>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Alexei Starovoitov <ast@...nel.org>
Cc:     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/21/2018 12:59 AM, Jakub Kicinski wrote:
> On Thu, 20 Sep 2018 14:04:19 +0900, Prashant Bhole wrote:
>> 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?
> 
> I actually prefer to stick to strerror(), the standard errors more
> clearly correlate with what happened in my mind (i.e. "Operation not
> supported" == kernel sent EOPNOTSUPP).   strerror() may also print in
> local language if translation/localization matters.
> 
> We could even use strerr() in dump_map_elem() but up to you.  The one
> in do_lookup() I'd prefer to leave be ;)

Sorry for the late reply.
In v2 I have removed the error strings altogether. As you suggested 
output will be strerror(). Also added verifier tests as Alexei 
suggested. I am sending the RFC-v2 series soon.
Thanks.

-Prashant