Message-ID: <a0b8a249-b62d-08a9-b559-43b62d5e7896@netronome.com>
Date:   Mon, 17 Dec 2018 10:44:45 +0000
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>
Cc:     netdev@...r.kernel.org, oss-drivers@...ronome.com,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH bpf-next 2/8] tools: bpftool: add probes for /proc/ eBPF
 parameters

2018-12-16 01:14 UTC+0100 ~ Daniel Borkmann <daniel@...earbox.net>
> On 12/15/2018 04:31 AM, Quentin Monnet wrote:
>> 2018-12-15 00:40 UTC+0100 ~ Daniel Borkmann <daniel@...earbox.net>
>>> On 12/13/2018 01:19 PM, Quentin Monnet wrote:
>>>> Add a set of probes to dump the eBPF-related parameters available from
>>>> /proc/: availability of bpf() syscall for unprivileged users,
>>>> JIT compiler status and hardening status, kallsyms exports status.
>>>>
>>>> Sample output:
>>>>
>>>>      # bpftool feature probe kernel
>>>>      Scanning system configuration...
>>>>      bpf() syscall for unprivileged users is enabled
>>>>      JIT compiler is disabled
>>>>      JIT compiler hardening is disabled
>>>>      JIT compiler kallsyms exports are disabled
>>>>      ...
>>>>
>>>>      # bpftool --json --pretty feature probe kernel
>>>>      {
>>>>          "system_config": {
>>>>              "unprivileged_bpf_disabled": 0,
>>>>              "bpf_jit_enable": 0,
>>>>              "bpf_jit_harden": 0,
>>>>              "bpf_jit_kallsyms": 0
>>>>          },
>>>>          ...
>>>>      }
>>>>
>>>>      # bpftool feature probe kernel macros prefix BPFTOOL_
>>>>      #define UNPRIVILEGED_BPF_DISABLED UNPRIVILEGED_BPF_DISABLED_OFF
>>>>      #define  UNPRIVILEGED_BPF_DISABLED_OFF 0
>>>>      #define  UNPRIVILEGED_BPF_DISABLED_ON 1
>>>>      #define  UNPRIVILEGED_BPF_DISABLED_UNKNOWN -1
>>>>      #define JIT_COMPILER_ENABLE JIT_COMPILER_ENABLE_OFF
>>>>      #define  JIT_COMPILER_ENABLE_OFF 0
>>>>      #define  JIT_COMPILER_ENABLE_ON 1
>>>>      #define  JIT_COMPILER_ENABLE_ON_WITH_DEBUG 2
>>>>      #define  JIT_COMPILER_ENABLE_UNKNOWN -1
>>>>      #define JIT_COMPILER_HARDEN JIT_COMPILER_HARDEN_OFF
>>>>      #define  JIT_COMPILER_HARDEN_OFF 0
>>>>      #define  JIT_COMPILER_HARDEN_FOR_UNPRIVILEGED 1
>>>>      #define  JIT_COMPILER_HARDEN_FOR_ALL_USERS 2
>>>>      #define  JIT_COMPILER_HARDEN_UNKNOWN -1
>>>>      #define JIT_COMPILER_KALLSYMS JIT_COMPILER_KALLSYMS_OFF
>>>>      #define  JIT_COMPILER_KALLSYMS_OFF 0
>>>>      #define  JIT_COMPILER_KALLSYMS_FOR_ROOT 1
>>>>      #define  JIT_COMPILER_KALLSYMS_UNKNOWN -1
>>>>      ...
>>>
>>> Hm, given these knobs may change at any point in time, what would
>>> be a use case in an application for these if they cannot be relied
>>> upon? (At least the jit_enable and jit_harden are transparent to
>>> the user.)
>>
>> Granted, for those parameters it's a snapshot of the system at the time
>> the probes are run. It can be useful, I suppose, if a server is not
>> expected to change them often... And the plain output might be useful to
>> a sysadmin who wants to have a quick look at BPF-related parameters, maybe?
> 
> Hmm, but wouldn't the main purpose of this header file be to include it
> into a BPF program to selectively enable / disable features (e.g. LPM
> map vs hashtab when kernel does not support LPM type as one example)?
> What would a use-case be for the above defines used inside such BPF prog?
> (Similarly for the kernel config defines in the other patch, how would
> a BPF prog use them?)
> 
> I think perhaps the 'issue' is that the C-style header generation and json
> dump are dumping the /exact/ same information. Is this a requirement?
> Wouldn't it be better to evolve the two /independently/?
> 
> E.g. the system_config bits from the json dump and BPF-related kernel
> config, perhaps also a listing of available maps, progs with supported
> helpers for a prog would be useful for the json dump for an admin or
> orchestration daemon to adapt to the underlying kernel where it could
> just parse the json and doesn't have to do the queries by itself.
> 
> But for the header generation, I would only place defines in there that
> are strictly relevant for the BPF program author. Available maps, progs
> and helpers is a good start there, later we could also put others in there
> such as [0] and similar specifics or quirks to verifier behavior that
> would be relevant in terms of work-arounds for supporting different kernel
> versions; but on a case by case basis. There things might potentially be
> less interesting for a json dump (though the json dump could overall be
> a superset of the info from the header file).
> 
>    [0] https://github.com/cilium/cilium/blob/master/bpf/probes/raw_mark_map_val.t
> 

For the use case about kernel config options, I was thinking about 
Cilium which collects some of them as well [0], but maybe it's not worth 
having it in the C-style header for now. For procfs parameters, maybe 
it's not so relevant indeed to have them at all in this output.

So you have a point, I suppose. I do not have any hard requirement about 
having the #define and the JSON similar; I argued with Stanislav that I 
didn't want to introduce small losses of information between the two, 
but if we consider them entirely different from the start it is not the 
same thing... So maybe I should just stick to the basics for the #define 
output, as you suggest.

I've seen the other probes used by Cilium, but I intentionally left the 
most specific one aside for now, there's enough to do with the current 
probes :). But yeah, it would make sense to have them added in the 
future. And for the record, I like the idea of keeping JSON a superset 
of the available information indeed.

I'll go with just prog/map types and helpers for the #define in my next 
version. This should also settle the discussion on the format of the 
macros used in this first version for the procfs parameters.

Thanks!
Quentin

[0] https://github.com/cilium/cilium/blob/master/bpf/run_probes.sh#L37

Powered by blists - more mailing lists