[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <783eb0b0-adce-ba31-0b2a-dbc93ea86b23@linux.dev>
Date: Mon, 19 Sep 2022 10:28:25 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Pu Lehui <pulehui@...weicloud.com>,
Stanislav Fomichev <sdf@...gle.com>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Quentin Monnet <quentin@...valent.com>,
Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>, bpf@...r.kernel.org,
linux-kernel@...r.kernel.org, Pu Lehui <pulehui@...wei.com>
Subject: Re: [PATCH bpf-next v3 1/2] bpf, cgroup: Don't populate
prog_attach_flags array when effective query
On 9/19/22 6:32 AM, Pu Lehui wrote:
>
>
> On 2022/9/17 8:03, Martin KaFai Lau wrote:
>> On 9/14/22 9:17 AM, Pu Lehui wrote:
>>> From: Pu Lehui <pulehui@...wei.com>
>>>
>>> Attach flags is only valid for attached progs of this layer cgroup,
>>> but not for effective progs. For querying with EFFECTIVE flags,
>>> exporting attach flags does not make sense. so we don't need to
>>> populate prog_attach_flags array when effective query.
>>
>> prog_attach_flags has been added to 6.0 which is in rc5. It is still
>> doable (and cleaner) to reject prog_attach_flags when it is an
>> effective_query. This should be done regardless of 'type ==
>> BPF_LSM_CGROUP' or not. Something like:
>>
>> if (effective_query && prog_attach_flags)
>> return -EINVAL;
>>
>> Otherwise, the whole prog_attach_flags needs to be set to 0 during
>> effective_query. Please target the change to the bpf tree instead of
>> bpf-next such that this uapi bit can be fixed before 6.0.
>>
>
> Okay, will handle in next version.
Thanks. It will also be useful to comment the uapi's bpf.h and mention
prog_attach_flags should not be set during effective_query.
>
>> Also, the effective_query issue is not limited to the
>> prog_attach_flags? For the older uattr->query.attach_flags, it should
>> be set to 0 also when it is an effective_query, right?
>
> For output uattr->query.attach_flags, we certainly don't need to copy it
> to userspace when effective query. Since we do not utilize
> uattr->query.attach_flags in the cgroup query function, should we need
> to take it as input and reject when it is non-zero in effective query?
> Something like:
> if (effective_query && (prog_attach_flags || attr->query.attach_flags))
No. I don't think the zero attr->query.attach_flags can be enforced
now. It is used as an output value only and its input value has never
been checked. Although the bpftool always sets it to 0 before the
query, checking zero now does not gain much while there is a slight
chance of breaking other users.
Only need to set/output uattr->query.attach_flags as 0 during
effective_query.
>
> For both output and input scenarios, we are faced with the problem that
> there is a ambiguity in attach_flags being 0. When we do not copy to the
> userspace, libbpf will set it to 0 by default, and 0 can mean NONE flag
> attach, or no attach prog. The same is true for input scenarios.
>
> So should we need to define NONE attach flag and redefine the others?
> Such as follow:
> #define BPF_F_ALLOW_NONE (1U << 0)
I would not change the uapi for this. 0 implicitly means no flags or
none. Regardless, this change does not belong to the bpf tree where
this fix will be landing.
> #define BPF_F_ALLOW_OVERRIDE (1U << 1)
> #define BPF_F_ALLOW_MULTI (1U << 2)
> #define BPF_F_REPLACE (1U << 3)
>
> And then attach flags being 0 certainly means no attach any prog.
>
Powered by blists - more mailing lists