Re: [PATCH bpf-next 6/7] bpf: enable retrival of pid/tgid/comm from bpf cgroup hooks

[Daniel Borkmann <daniel@iogearbox.net>]

On 3/28/20 1:49 AM, Andrii Nakryiko wrote:
> On Fri, Mar 27, 2020 at 8:59 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> We already have the bpf_get_current_uid_gid() helper enabled, and
>> given we now have perf event RB output available for connect(),
>> sendmsg(), recvmsg() and bind-related hooks, add a trivial change
>> to enable bpf_get_current_pid_tgid() and bpf_get_current_comm()
>> as well.
>>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
> 
> LGTM, there was probably never a good reason this wasn't available
> from the very beginning :)

Right. :-)

> Might as well add bpf_get_current_uid_gid() if it's not there yet.

It's already there. ;-)

> Acked-by: Andrii Nakryiko <andriin@fb.com>
> 
>>   net/core/filter.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 5cec3ac9e3dd..bb4a196c8809 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -6018,6 +6018,10 @@ sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>>                  return &bpf_get_netns_cookie_sock_proto;
>>          case BPF_FUNC_perf_event_output:
>>                  return &bpf_event_output_data_proto;
>> +       case BPF_FUNC_get_current_pid_tgid:
>> +               return &bpf_get_current_pid_tgid_proto;
>> +       case BPF_FUNC_get_current_comm:
>> +               return &bpf_get_current_comm_proto;
> 
> So you are not adding it to bpf_base_func_proto() instead, because
> that one can be used in BPF programs that don't have a valid current,
> is that right? If yes, would it make sense to have a common
> bpf_base_process_ctx_func_proto() function for cases where there is a
> valid current and add all the functions there (including uid_gid and
> whatever else makes sense?)

I didn't add it to bpf_base_func_proto() since we might not always have
a useful 'current' available. My focus in this series was on sock_addr
and sock helpers for our LB where 'current' is always the application
doing the syscall. A common base_func_proto() for both could be useful
though this only works with helpers that do not rely on the input context
since both are different. Tbh, the whole net/core/filter.c feels quite
convoluted these days where it's getting hard to follow which func_proto
and access checker belongs to which program type. I can check if we could
get some more order in there in general through some larger refactoring
to make it easier for people to extend, though not sure if more
bpf_base_func_proto()-like helpers would add much, I think it needs a
larger revamp.

>>   #ifdef CONFIG_CGROUPS
>>          case BPF_FUNC_get_current_cgroup_id:
>>                  return &bpf_get_current_cgroup_id_proto;
>> @@ -6058,6 +6062,10 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>>                  return &bpf_get_local_storage_proto;
>>          case BPF_FUNC_perf_event_output:
>>                  return &bpf_event_output_data_proto;
>> +       case BPF_FUNC_get_current_pid_tgid:
>> +               return &bpf_get_current_pid_tgid_proto;
>> +       case BPF_FUNC_get_current_comm:
>> +               return &bpf_get_current_comm_proto;
>>   #ifdef CONFIG_CGROUPS
>>          case BPF_FUNC_get_current_cgroup_id:
>>                  return &bpf_get_current_cgroup_id_proto;
>> --
>> 2.21.0
>>