Re: [PATCH net-next v4 2/3] bpf: Add bpf_current_task_under_cgroup helper

[Daniel Borkmann <daniel@iogearbox.net>]

On 08/12/2016 09:22 AM, Sargun Dhillon wrote:
> On Fri, Aug 12, 2016 at 09:16:07AM +0200, Daniel Borkmann wrote:
>> On 08/12/2016 06:50 AM, Sargun Dhillon wrote:
>>> I realize that in_cgroup is more consistent, but under_cgroup makes
>>> far more sense to me. I think it's more intuitive.
>>>
>>> On Thu, Aug 11, 2016 at 9:48 PM, Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>> On Thu, Aug 11, 2016 at 08:14:56PM -0700, Sargun Dhillon wrote:
>>>>> This adds a bpf helper that's similar to the skb_in_cgroup helper to check
>>>>> whether the probe is currently executing in the context of a specific
>>>>> subset of the cgroupsv2 hierarchy. It does this based on membership test
>>>>> for a cgroup arraymap. It is invalid to call this in an interrupt, and
>>>>> it'll return an error. The helper is primarily to be used in debugging
>>>>> activities for containers, where you may have multiple programs running in
>>>>> a given top-level "container".
>>>>>
>>>>> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>>>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>>>> Cc: Tejun Heo <tj@kernel.org>
>>>>> ---
>>>>> +     /**
>>>>> +      * bpf_current_task_under_cgroup(map, index) - Check cgroup2 membership of current task
>>>>> +      * @map: pointer to bpf_map in BPF_MAP_TYPE_CGROUP_ARRAY type
>>>>> +      * @index: index of the cgroup in the bpf_map
>>>>> +      * Return:
>>>>> +      *   == 0 current failed the cgroup2 descendant test
>>>>> +      *   == 1 current succeeded the cgroup2 descendant test
>>>>> +      *    < 0 error
>>>>> +      */
>>>>> +     BPF_FUNC_current_task_under_cgroup,
>>>> ..
>>>>>        case BPF_MAP_TYPE_CGROUP_ARRAY:
>>>>> -             if (func_id != BPF_FUNC_skb_in_cgroup)
>>>>> +             if (func_id != BPF_FUNC_skb_in_cgroup &&
>>>>> +                 func_id != BPF_FUNC_current_task_under_cgroup)
>>>>>                        goto error;
>>>> ...
>>>>> +     case BPF_FUNC_current_task_under_cgroup:
>>>>>        case BPF_FUNC_skb_in_cgroup:
>>>>
>>>> Tejun,
>>>> do you feel strongly about 'under' ?
>>>> It just looks inconsistent vs existing skb_in_cgroup...
>>>> "in cgroup" - 4k google hits
>>>> "under cgroup" - 2k google hits
>>
>> Alternative could be that we take "BPF_FUNC_current_in_cgroup" as a
>> helper enum to keep consistency with what we have wrt skb helper, but
>> for the cgroup header have the suggested task_under_cgroup_hierarchy()
>> name.
>
> I actually wish we could rename skb_in_cgroup to skb_under_cgroup. If we ever
> introduced a check for absolute membership versus ancestral membership, what
> would we call that?

That option is, by the way, still on the table for -net tree, since 4.8 is not
released yet, so it could still be renamed into BPF_FUNC_skb_under_cgroup.

Then you could make this one here for -net-next as "BPF_FUNC_current_under_cgroup".

Tejun, Alexei?