Re: [PATCH bpf-next 2/8] tools: bpftool: add probes for /proc/ eBPF parameters

2018-12-13 18:58 UTC-0800 ~ Stanislav Fomichev [off-list ref]

On 12/13, Quentin Monnet wrote:

quoted

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

[..]

quoted

    # 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

This looks a bit complicated. For example, why not simply define:

#define UNPRIVILEGED_BPF_DISABLED 1 /* when it's explicitly disabled */
#define UNPRIVILEGED_BPF_ENABLED  1 /* when it's explicitly enabled */
#define UNPRIVILEGED_BPF_UNKNOWN  1 /* when unknown - maybe even skip
this altogether and treat unknown == disabled (worst case) */

Then, I, as a potential user, can do:

#if defined(UNPRIVILEGED_BPF_ENABLED)
/* do something useful */
#elif defined(UNPRIVILEGED_BPF_DISABLED)
/* print an error asking to use root */
#else
/* try anyway, fallback to error ? */
#endif

IMO, if don't want to do stuff like:

#if UNPRIVILEGED_BPF_DISABLED == UNPRIVILEGED_BPF_DISABLED_OFF
#elif UNPRIVILEGED_BPF_DISABLED == UNPRIVILEGED_BPF_DISABLED_ON
#else
#endif

I live in my mental model if ifdefs, not complicated cpp #if
comparisons.

Just a suggestion, I feel like we can keep it simple.

It's true that it make things look simpler. We loose the direct
correspondence between macro name and procfs file name, but I suppose we
can live with it.

I'm more concerned about the loss of information for the "unknown" case,
however. If we successfully retrieved a value from the procfs, I find it
harsh not to give it to the user, even if we cannot interpret it :/. And
I don't see a way of doing that without having a UNPRIVILEGED_BPF_VALUE
macro of some kind.