Re: [PATCH bpf-next 1/2] bpf, cgroup: Fix attach flags being assigned to effective progs

[Pu Lehui <pulehui@huawei.com>]

On 2022/9/8 11:07, Pu Lehui wrote:
> 
> 
> On 2022/8/24 5:22, John Fastabend wrote:
>> Pu Lehui wrote:
>>> Attach flags is only valid for attached progs of this layer cgroup,
>>> but not for effective progs. We know that the attached progs is at
>>> the beginning of the effective progs array, so we can just populate
>>> the elements in front of the prog_attach_flags array.
>>>
>>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
>>
>> Trying to parse above, could you add a bit more detail on why this is
>> problem so readers don't need to track it down.
>>
>>> ---
>>>   kernel/bpf/cgroup.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>>> index 59b7eb60d5b4..9adf72e99907 100644
>>> --- a/kernel/bpf/cgroup.c
>>> +++ b/kernel/bpf/cgroup.c
>>> @@ -1091,11 +1091,14 @@ static int __cgroup_bpf_query(struct cgroup 
>>> *cgrp, const union bpf_attr *attr,
>>>           }
>>
>> Because we are looking at it let me try to understand. There are two
>> paths that set cnt relative bits here,
>>
> 
> Hi John,
> 
> Thanks for your review. The reason is that:
> For the following cgroup tree:
>     root
>      |
>     cg1
>      |
>     cg2
> 
> I attached prog1 and prog2 to root cgroup with MULTI attach type, 
sorry, typo, attach type -> attach flag
> attached prog3 to cg1 with OVERRIDE attach type, and then used bpftool
> to show this cgroup tree with effective query flag:
> 
> $ bpftool cgroup tree /sys/fs/cgroup effective
> CgroupPath
> ID       AttachType      AttachFlags     Name
> /sys/fs/cgroup
> 1        sysctl          multi           sysctl_tcp_mem
> 2        sysctl          multi           sysctl_tcp_mem
> /sys/fs/cgroup/cg1
> 3        sysctl          override        sysctl_tcp_mem
> 1        sysctl          override        sysctl_tcp_mem <- wrong
> 2        sysctl          override        sysctl_tcp_mem <- wrong
> /sys/fs/cgroup/cg1/cg2
> 3        sysctl                          sysctl_tcp_mem
> 1        sysctl                          sysctl_tcp_mem
> 2        sysctl                          sysctl_tcp_mem
> 
> For cg1, obviously, the attach flags of prog1 and prog2 can not be 
> OVERRIDE, and the attach flags of prog1 and prog2 is meaningless for 
> cg1. We only need to care the attach flags of prog which attached to 
> cg1, other progs attach flags should be omit.
> 
>>    if (attr->query.query_flags & BPF_F_QUERY_EFFECTIVE) {
>>        ...
>>        cnt = min_t(int, bpf_prog_array_length(effective), total_cnt);
>>        ...
>>    } else {
>>       ...
>>       progs = &cgrp->bpf.progs[atype];
>>       cnt = min_t(int, prog_list_length(progs), total_cnt);
>>       ...
>>    }
>>
>> And the docs claim
>>
>>   *              **BPF_F_QUERY_EFFECTIVE**
>>   *                      Only return information regarding programs 
>> which are
>>   *                      currently effective at the specified 
>> *target_fd*.
>>
>> so in the EFFECTIVE case should we be reporting flags at all if the
>> commit message says "attach flags is only valid for attached progs
>> of this layer cgroup, but not for effective progs."
>>
>> And then in the else branch the change is what you have in the diff 
>> anyways correct?
>>
>>>           if (prog_attach_flags) {
>>> +            int progs_cnt = prog_list_length(&cgrp->bpf.progs[atype]);
>>>               flags = cgrp->bpf.flags[atype];
>>> -            for (i = 0; i < cnt; i++)
>>
>> Do we need to min with total_cnt here so we don't walk off a short 
>> user list?
>>
> 
> We should limit it, will fix it in v2. For query without effective flag, 
> progs_cnt will equal to cnt, and for effective flag situation, progs_cnt 
> only calculate prog count which attached to this cgroup layer.
> 
>>> +            /* attach flags only for attached progs, but not 
>>> effective progs */
>>> +            for (i = 0; i < progs_cnt; i++)
>>>                   if (copy_to_user(prog_attach_flags + i, &flags, 
>>> sizeof(flags)))
>>>                       return -EFAULT;
>>> +
>>>               prog_attach_flags += cnt;
>>>           }
>>> -- 
>>> 2.25.1
>>>
>> .
>>
> .