On Tue, May 12, 2020 at 05:05:12PM +0200, Daniel Borkmann wrote:
> > - env->allow_ptr_leaks = is_priv;
> > + env->allow_ptr_leaks = perfmon_capable();
> > + env->bpf_capable = bpf_capable();
>
> Probably more of a detail, but it feels weird to tie perfmon_capable() into the BPF
> core and use it in various places there. I would rather make this a proper bpf_*
> prefixed helper and add a more descriptive name (what does it have to do with perf
> or monitoring directly?). For example, all the main functionality could be under
> `bpf_base_capable()` and everything with potential to leak pointers or mem to user
> space as `bpf_leak_capable()`. Then inside include/linux/capability.h this can still
> resolve under the hood to something like:
>
> static inline bool bpf_base_capable(void)
> {
> return capable(CAP_BPF) || capable(CAP_SYS_ADMIN);
> }
I don't like the 'base' in the name, since 'base' implies common subset,
but it's not the case. Also 'base' implies that something else is additive,
but it's not the case either. The real base is unpriv. cap_bpf adds to it.
So bpf_capable() in capability.h is the most appropriate.
It also matches perfmon_capable() and other *_capable()
> static inline bool bpf_leak_capable(void)
> {
> return perfmon_capable();
> }
This is ok, but not in capability.h. I can put it into bpf_verifier.h
