netdev - Re: [PATCH bpf-next 09/13] tools/bpftool: add bpftool support for bpf map element iterator

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: <c70ebb0a-538c-a84f-f606-1d08af426fde@fb.com>
Date:   Thu, 16 Jul 2020 10:42:13 -0700
From:   Yonghong Song <yhs@...com>
To:     Quentin Monnet <quentin@...valent.com>, <bpf@...r.kernel.org>,
        <netdev@...r.kernel.org>
CC:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, <kernel-team@...com>,
        Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf-next 09/13] tools/bpftool: add bpftool support for bpf
 map element iterator



On 7/16/20 9:39 AM, Quentin Monnet wrote:
> 2020-07-13 09:17 UTC-0700 ~ Yonghong Song <yhs@...com>
>> The optional parameter "map MAP" can be added to "bpftool iter"
>> command to create a bpf iterator for map elements. For example,
>>    bpftool iter pin ./prog.o /sys/fs/bpf/p1 map id 333
>>
>> For map element bpf iterator "map MAP" parameter is required.
>> Otherwise, bpf link creation will return an error.
>>
>> Signed-off-by: Yonghong Song <yhs@...com>
>> ---
>>   .../bpftool/Documentation/bpftool-iter.rst    | 16 ++++++++--
>>   tools/bpf/bpftool/iter.c                      | 32 ++++++++++++++++---
>>   2 files changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-iter.rst b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
>> index 8dce698eab79..53ee4fb188b4 100644
>> --- a/tools/bpf/bpftool/Documentation/bpftool-iter.rst
>> +++ b/tools/bpf/bpftool/Documentation/bpftool-iter.rst
>> @@ -17,14 +17,15 @@ SYNOPSIS
>>   ITER COMMANDS
>>   ===================
>>   
>> -|	**bpftool** **iter pin** *OBJ* *PATH*
>> +|	**bpftool** **iter pin** *OBJ* *PATH* [**map** *MAP*]
>>   |	**bpftool** **iter help**
>>   |
>>   |	*OBJ* := /a/file/of/bpf_iter_target.o
>> +|       *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> 
> Please don't change the indentation style (other lines have a tab).

Will fix.

> 
>>   
>>   DESCRIPTION
>>   ===========
>> -	**bpftool iter pin** *OBJ* *PATH*
>> +	**bpftool iter pin** *OBJ* *PATH* [**map** *MAP*]
>>   		  A bpf iterator combines a kernel iterating of
>>   		  particular kernel data (e.g., tasks, bpf_maps, etc.)
>>   		  and a bpf program called for each kernel data object
>> @@ -37,6 +38,10 @@ DESCRIPTION
>>   		  character ('.'), which is reserved for future extensions
>>   		  of *bpffs*.
>>   
>> +                  Map element bpf iterator requires an additional parameter
>> +                  *MAP* so bpf program can iterate over map elements for
>> +                  that map.
>> +
> 
> Same note on indentation.
> 
> Could you please also explain in a few words what the "Map element bpf
> iterator" is? Reusing part of your cover letter (see below) could do,
> it's just so that users not familiar with the concept can get an idea of
> what it does.

Will do.

> 
> ---
> User can have a bpf program in kernel to run with each map element,
> do checking, filtering, aggregation, etc. without copying data
> to user space.
> ---
> 
>>   		  User can then *cat PATH* to see the bpf iterator output.
>>   
>>   	**bpftool iter help**
> 
> [...]
> 
>> @@ -62,13 +83,16 @@ static int do_pin(int argc, char **argv)
>>   	bpf_link__destroy(link);
>>   close_obj:
>>   	bpf_object__close(obj);
>> +close_map_fd:
>> +	if (map_fd >= 0)
>> +		close(map_fd);
>>   	return err;
>>   }
>>   
>>   static int do_help(int argc, char **argv)
>>   {
>>   	fprintf(stderr,
>> -		"Usage: %1$s %2$s pin OBJ PATH\n"
>> +		"Usage: %1$s %2$s pin OBJ PATH [map MAP]\n"
> 
> You probably want to add HELP_SPEC_MAP (as in map.c) to tell the user
> what MAP should be.

Good suggestion.

> 
> Could you please also update the bash completion?

This is always my hardest part! In this case it is
   bpftool iter pin <filedir> <filedir> [map MAP]

Any particular existing bpftool implementation I can imitate?

> 
> Thanks,
> Quentin
>