Yonghong Song wrote:
>
>
> On 5/13/20 12:24 PM, John Fastabend wrote:
> > Often it is useful when applying policy to know something about the
> > task. If the administrator has CAP_SYS_ADMIN rights then they can
> > use kprobe + sk_msg and link the two programs together to accomplish
> > this. However, this is a bit clunky and also means we have to call
> > sk_msg program and kprobe program when we could just use a single
> > program and avoid passing metadata through sk_msg/skb, socket, etc.
> >
> > To accomplish this add probe_* helpers to sk_msg programs guarded
> > by a CAP_SYS_ADMIN check. New supported helpers are the following,
> >
> > BPF_FUNC_get_current_task
> > BPF_FUNC_current_task_under_cgroup
> > BPF_FUNC_probe_read_user
> > BPF_FUNC_probe_read_kernel
> > BPF_FUNC_probe_read
> > BPF_FUNC_probe_read_user_str
> > BPF_FUNC_probe_read_kernel_str
> > BPF_FUNC_probe_read_str
>
> I think this is a good idea. But this will require bpf program
> to be GPLed, probably it will be okay. Currently, for capabilities,
> it is CAP_SYS_ADMIN now, in the future, it may be CAP_PERFMON.
Right.
>
> Also, do we want to remove BPF_FUNC_probe_read and
> BPF_FUNC_probe_read_str from the list? Since we
> introduce helpers to new program types, we can deprecate
> these two helpers right away.
Removed, Daniel had the same comment.
>
> The new helpers will be subject to new security lockdown
> rules which may have impact on networking bpf programs
> on particular setup.
But only if these helpers are used. If not everything should
be the same I think.
>
> >
> > Signed-off-by: John Fastabend <john.fastabend@xxxxxxxxx>
> > ---
[...]
> > @@ -6397,6 +6406,31 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_get_cgroup_classid_curr_proto;
> > #endif
> > default:
> > + break;
> > + }
> > +
> > + if (!capable(CAP_SYS_ADMIN))
> > + return bpf_base_func_proto(func_id);
> > +
> > + /* All helpers below are for CAP_SYS_ADMIN only */
> > + switch (func_id) {
> > + case BPF_FUNC_get_current_task:
> > + return &bpf_get_current_task_proto;
> > + case BPF_FUNC_current_task_under_cgroup:
> > + return &bpf_current_task_under_cgroup_proto;
> > + case BPF_FUNC_probe_read_user:
> > + return &bpf_probe_read_user_proto;
> > + case BPF_FUNC_probe_read_kernel:
> > + return &bpf_probe_read_kernel_proto;
> > + case BPF_FUNC_probe_read:
> > + return &bpf_probe_read_compat_proto;
> > + case BPF_FUNC_probe_read_user_str:
> > + return &bpf_probe_read_user_str_proto;
> > + case BPF_FUNC_probe_read_kernel_str:
> > + return &bpf_probe_read_kernel_str_proto;
> > + case BPF_FUNC_probe_read_str:
> > + return &bpf_probe_read_compat_str_proto;
> > + default:
> > return bpf_base_func_proto(func_id);
>
> If we can get a consensus here, I think we can even folding all
> these bpf helpers (get_current_task, ..., probe_read_kernel_str)
> to bpf_base_func_proto, so any bpf program types including
> other networking types can use them.
> Any concerns?
>
Nothing comes to mind. I'm OK to move them into base if folks
agree its useful there. I was putting them where I have a known
use case at the moment but doesn't bother me to make them more
widely available.
