[bpf-next,0/6] bpftool: Allow to select sections and filter probes

This patch series extend the "bpftool feature" subcommand with the
new positional arguments:

- "section", which allows to select a specific section of probes (i.e.
  "system_config", "program_types", "map_types");
- "filter_in", which allows to select only probes which matches the
  given regex pattern;
- "filter_out", which allows to filter out probes which do not match the
  given regex pattern.

The main motivation behind those changes is ability the fact that some
probes (for example those related to "trace" or "write_user" helpers)
emit dmesg messages which might be confusing for people who are running
on production environments. For details see the Cilium issue[0].

[0] https://github.com/cilium/cilium/issues/10048

Michal Rostecki (6):
  bpftool: Move out sections to separate functions
  bpftool: Allow to select a specific section to probe
  bpftool: Add arguments for filtering in and filtering out probes
  bpftool: Update documentation of "bpftool feature" command
  bpftool: Update bash completion for "bpftool feature" command
  selftests/bpf: Add test for "bpftool feature" command

 .../bpftool/Documentation/bpftool-feature.rst |  37 +-
 tools/bpf/bpftool/bash-completion/bpftool     |  32 +-
 tools/bpf/bpftool/feature.c                   | 592 +++++++++++++-----
 tools/testing/selftests/.gitignore            |   5 +-
 tools/testing/selftests/bpf/Makefile          |   3 +-
 tools/testing/selftests/bpf/test_bpftool.py   | 294 +++++++++
 tools/testing/selftests/bpf/test_bpftool.sh   |   5 +
 7 files changed, 811 insertions(+), 157 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/test_bpftool.py
 create mode 100755 tools/testing/selftests/bpf/test_bpftool.sh

On Tue, Feb 18, 2020 at 11:02 AM Michal Rostecki <mrostecki@opensuse.org> wrote:
>
> This patch series extend the "bpftool feature" subcommand with the
> new positional arguments:
>
> - "section", which allows to select a specific section of probes (i.e.
>   "system_config", "program_types", "map_types");
> - "filter_in", which allows to select only probes which matches the
>   given regex pattern;
> - "filter_out", which allows to filter out probes which do not match the
>   given regex pattern.
>
> The main motivation behind those changes is ability the fact that some
> probes (for example those related to "trace" or "write_user" helpers)
> emit dmesg messages which might be confusing for people who are running
> on production environments. For details see the Cilium issue[0].
>
> [0] https://github.com/cilium/cilium/issues/10048

The motivation is clear, but I think the users shouldn't be made
aware of such implementation details. I think instead of filter_in/out
it's better to do 'full or safe' mode of probing.
By default it can do all the probing that doesn't cause
extra dmesgs and in 'full' mode it can probe everything.

On 2/19/20 4:02 AM, Alexei Starovoitov wrote:
> The motivation is clear, but I think the users shouldn't be made
> aware of such implementation details. I think instead of filter_in/out
> it's better to do 'full or safe' mode of probing.
> By default it can do all the probing that doesn't cause
> extra dmesgs and in 'full' mode it can probe everything.

Alright, then I will send later v2 where the "internal" implementation
(filtering out based on regex) stays similar (filter_out will stay in
the code without being exposed to users, filter_in will be removed). And
the exposed option of "safe" probing will just apply the
"(trace|write_user)" filter_out pattern. Does it sound good?

On Wed, Feb 19, 2020 at 4:33 AM Michal Rostecki <mrostecki@opensuse.org> wrote:
>
> On 2/19/20 4:02 AM, Alexei Starovoitov wrote:
> > The motivation is clear, but I think the users shouldn't be made
> > aware of such implementation details. I think instead of filter_in/out
> > it's better to do 'full or safe' mode of probing.
> > By default it can do all the probing that doesn't cause
> > extra dmesgs and in 'full' mode it can probe everything.
>
> Alright, then I will send later v2 where the "internal" implementation
> (filtering out based on regex) stays similar (filter_out will stay in
> the code without being exposed to users, filter_in will be removed). And
> the exposed option of "safe" probing will just apply the
> "(trace|write_user)" filter_out pattern. Does it sound good?

yes. If implementation is doing filter_in and applying 'trace_printk|write_user'
strings hidden within bpftool than I think it should be good.
What do you think the default should be?
It feels to me that the default should not be causing dmesg prints.
So only addition flag for bpftool command line will be 'bpftool
feature probe full'

On 2/19/20 5:37 PM, Alexei Starovoitov wrote:
> On Wed, Feb 19, 2020 at 4:33 AM Michal Rostecki <mrostecki@opensuse.org> wrote:
>>
>> On 2/19/20 4:02 AM, Alexei Starovoitov wrote:
>>> The motivation is clear, but I think the users shouldn't be made
>>> aware of such implementation details. I think instead of filter_in/out
>>> it's better to do 'full or safe' mode of probing.
>>> By default it can do all the probing that doesn't cause
>>> extra dmesgs and in 'full' mode it can probe everything.
>>
>> Alright, then I will send later v2 where the "internal" implementation
>> (filtering out based on regex) stays similar (filter_out will stay in
>> the code without being exposed to users, filter_in will be removed). And
>> the exposed option of "safe" probing will just apply the
>> "(trace|write_user)" filter_out pattern. Does it sound good?
> 
> yes. If implementation is doing filter_in and applying 'trace_printk|write_user'
> strings hidden within bpftool than I think it should be good.
> What do you think the default should be?
> It feels to me that the default should not be causing dmesg prints.
> So only addition flag for bpftool command line will be 'bpftool
> feature probe full'

Agree, that makes sense to me.

On 2/19/20 5:52 PM, Daniel Borkmann wrote:
> On 2/19/20 5:37 PM, Alexei Starovoitov wrote:
>> On Wed, Feb 19, 2020 at 4:33 AM Michal Rostecki
>> <mrostecki@opensuse.org> wrote:
>>>
>>> On 2/19/20 4:02 AM, Alexei Starovoitov wrote:
>>>> The motivation is clear, but I think the users shouldn't be made
>>>> aware of such implementation details. I think instead of filter_in/out
>>>> it's better to do 'full or safe' mode of probing.
>>>> By default it can do all the probing that doesn't cause
>>>> extra dmesgs and in 'full' mode it can probe everything.
>>>
>>> Alright, then I will send later v2 where the "internal" implementation
>>> (filtering out based on regex) stays similar (filter_out will stay in
>>> the code without being exposed to users, filter_in will be removed). And
>>> the exposed option of "safe" probing will just apply the
>>> "(trace|write_user)" filter_out pattern. Does it sound good?
>>
>> yes. If implementation is doing filter_in and applying
>> 'trace_printk|write_user'
>> strings hidden within bpftool than I think it should be good.
>> What do you think the default should be?
>> It feels to me that the default should not be causing dmesg prints.
>> So only addition flag for bpftool command line will be 'bpftool
>> feature probe full'
> 
> Agree, that makes sense to me.

Makes sense to me as well. I will do that in v2.