| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7d01b12e-7d94-534d-8582-d3c1485779d0@fb.com>
Date: Mon, 4 Jun 2018 08:11:04 -0700
From: Yonghong Song <yhs@...com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>,
Daniel Borkmann <daniel@...earbox.net>
CC: <ast@...com>, <netdev@...r.kernel.org>, <kernel-team@...com>
Subject: Re: [PATCH net-next v2 1/3] bpf: implement
bpf_get_current_cgroup_id() helper
On 6/4/18 7:58 AM, Alexei Starovoitov wrote:
> On Mon, Jun 04, 2018 at 11:08:35AM +0200, Daniel Borkmann wrote:
>> On 06/04/2018 12:59 AM, Yonghong Song wrote:
>>> bpf has been used extensively for tracing. For example, bcc
>>> contains an almost full set of bpf-based tools to trace kernel
>>> and user functions/events. Most tracing tools are currently
>>> either filtered based on pid or system-wide.
>>>
>>> Containers have been used quite extensively in industry and
>>> cgroup is often used together to provide resource isolation
>>> and protection. Several processes may run inside the same
>>> container. It is often desirable to get container-level tracing
>>> results as well, e.g. syscall count, function count, I/O
>>> activity, etc.
>>>
>>> This patch implements a new helper, bpf_get_current_cgroup_id(),
>>> which will return cgroup id based on the cgroup within which
>>> the current task is running.
>>>
>>> The later patch will provide an example to show that
>>> userspace can get the same cgroup id so it could
>>> configure a filter or policy in the bpf program based on
>>> task cgroup id.
>>>
>>> The helper is currently implemented for tracing. It can
>>> be added to other program types as well when needed.
>>>
>>> Acked-by: Alexei Starovoitov <ast@...nel.org>
>>> Signed-off-by: Yonghong Song <yhs@...com>
>>> ---
>>> include/linux/bpf.h | 1 +
>>> include/uapi/linux/bpf.h | 8 +++++++-
>>> kernel/bpf/core.c | 1 +
>>> kernel/bpf/helpers.c | 15 +++++++++++++++
>>> kernel/trace/bpf_trace.c | 2 ++
>>> 5 files changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index bbe2974..995c3b1 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -746,6 +746,7 @@ extern const struct bpf_func_proto bpf_get_stackid_proto;
>>> extern const struct bpf_func_proto bpf_get_stack_proto;
>>> extern const struct bpf_func_proto bpf_sock_map_update_proto;
>>> extern const struct bpf_func_proto bpf_sock_hash_update_proto;
>>> +extern const struct bpf_func_proto bpf_get_current_cgroup_id_proto;
>>>
>>> /* Shared helpers among cBPF and eBPF. */
>>> void bpf_user_rnd_init_once(void);
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index f0b6608..18712b0 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -2070,6 +2070,11 @@ union bpf_attr {
>>> * **CONFIG_SOCK_CGROUP_DATA** configuration option.
>>> * Return
>>> * The id is returned or 0 in case the id could not be retrieved.
>>> + *
>>> + * u64 bpf_get_current_cgroup_id(void)
>>> + * Return
>>> + * A 64-bit integer containing the current cgroup id based
>>> + * on the cgroup within which the current task is running.
>>> */
>>> #define __BPF_FUNC_MAPPER(FN) \
>>> FN(unspec), \
>>> @@ -2151,7 +2156,8 @@ union bpf_attr {
>>> FN(lwt_seg6_action), \
>>> FN(rc_repeat), \
>>> FN(rc_keydown), \
>>> - FN(skb_cgroup_id),
>>> + FN(skb_cgroup_id), \
>>> + FN(get_current_cgroup_id),
>>>
>>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>>> * function eBPF program intends to call
>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>>> index 527587d..9f14937 100644
>>> --- a/kernel/bpf/core.c
>>> +++ b/kernel/bpf/core.c
>>> @@ -1765,6 +1765,7 @@ const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
>>> const struct bpf_func_proto bpf_get_current_comm_proto __weak;
>>> const struct bpf_func_proto bpf_sock_map_update_proto __weak;
>>> const struct bpf_func_proto bpf_sock_hash_update_proto __weak;
>>> +const struct bpf_func_proto bpf_get_current_cgroup_id_proto __weak;
>>>
>>> const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void)
>>> {
>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>>> index 3d24e23..73065e2 100644
>>> --- a/kernel/bpf/helpers.c
>>> +++ b/kernel/bpf/helpers.c
>>> @@ -179,3 +179,18 @@ const struct bpf_func_proto bpf_get_current_comm_proto = {
>>> .arg1_type = ARG_PTR_TO_UNINIT_MEM,
>>> .arg2_type = ARG_CONST_SIZE,
>>> };
>>> +
>>> +#ifdef CONFIG_CGROUPS
>>> +BPF_CALL_0(bpf_get_current_cgroup_id)
>>> +{
>>> + struct cgroup *cgrp = task_dfl_cgroup(current);
>>> +
>>> + return cgrp->kn->id.id;
>>> +}
>>> +
>>> +const struct bpf_func_proto bpf_get_current_cgroup_id_proto = {
>>> + .func = bpf_get_current_cgroup_id,
>>> + .gpl_only = false,
>>> + .ret_type = RET_INTEGER,
>>> +};
>>> +#endif
>>
>> Nit: why not moving this function directly to bpf_trace.c?
>
> my preference would be to keep it in helpers.c as-is.
> imo bpf_trace.c is only for things that depend on kernel/trace/
>
>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>> index 752992c..e2ab5b7 100644
>>> --- a/kernel/trace/bpf_trace.c
>>> +++ b/kernel/trace/bpf_trace.c
>>> @@ -564,6 +564,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>>> return &bpf_get_prandom_u32_proto;
>>> case BPF_FUNC_probe_read_str:
>>> return &bpf_probe_read_str_proto;
>>> + case BPF_FUNC_get_current_cgroup_id:
>>> + return &bpf_get_current_cgroup_id_proto;
>>
>> When you have !CONFIG_CGROUPS, then it relies on the weak definition of the
>> bpf_get_current_cgroup_id_proto, which I would think at latest in fixup_bpf_calls()
>> bails out with 'kernel subsystem misconfigured func' due to func being NULL.
>>
>> Can't we just do the #ifdef CONFIG_CGROUPS around BPF_FUNC_get_current_cgroup_id
>> case instead? Then we bail out normally with 'unknown func' when cgroups are
>> not configured?
Thanks. Will send a followup patch soon.
>
> good idea.
>
Powered by blists - more mailing lists