Date:   Mon, 17 Dec 2018 12:11:12 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Quentin Monnet <quentin.monnet@...ronome.com>,
        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

On 12/17/2018 11:44 AM, Quentin Monnet wrote:
> 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.

Sounds good to me, I also think it's better to keep the json output a superset;
definitely allows for more flexibility and if there's a real world use case to
have some of that additional information also in the header output as defines,
we can add that at a later point in time.

> 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.

Yes makes sense, this would be great as a start, and we can extend it as needed.

Thanks,
Daniel

Powered by blists - more mailing lists