Re: [PATCH bpf-next v1 8/9] bpf: Introduce cgroup iter

[Yonghong Song <yhs@fb.com>]

On 3/2/22 7:03 PM, Kumar Kartikeya Dwivedi wrote:
> On Thu, Mar 03, 2022 at 07:33:16AM IST, Yonghong Song wrote:
>>
>>
>> On 3/2/22 2:45 PM, Kumar Kartikeya Dwivedi wrote:
>>> On Sat, Feb 26, 2022 at 05:13:38AM IST, Hao Luo wrote:
>>>> Introduce a new type of iter prog: cgroup. Unlike other bpf_iter, this
>>>> iter doesn't iterate a set of kernel objects. Instead, it is supposed to
>>>> be parameterized by a cgroup id and prints only that cgroup. So one
>>>> needs to specify a target cgroup id when attaching this iter.
>>>>
>>>> The target cgroup's state can be read out via a link of this iter.
>>>> Typically, we can monitor cgroup creation and deletion using sleepable
>>>> tracing and use it to create corresponding directories in bpffs and pin
>>>> a cgroup id parameterized link in the directory. Then we can read the
>>>> auto-pinned iter link to get cgroup's state. The output of the iter link
>>>> is determined by the program. See the selftest test_cgroup_stats.c for
>>>> an example.
>>>>
>>>> Signed-off-by: Hao Luo <haoluo@google.com>
>>>> ---
>>>>    include/linux/bpf.h            |   1 +
>>>>    include/uapi/linux/bpf.h       |   6 ++
>>>>    kernel/bpf/Makefile            |   2 +-
>>>>    kernel/bpf/cgroup_iter.c       | 141 +++++++++++++++++++++++++++++++++
>>>>    tools/include/uapi/linux/bpf.h |   6 ++
>>>>    5 files changed, 155 insertions(+), 1 deletion(-)
>>>>    create mode 100644 kernel/bpf/cgroup_iter.c
>>>>
>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>> index 759ade7b24b3..3ce9b0b7ed89 100644
>>>> --- a/include/linux/bpf.h
>>>> +++ b/include/linux/bpf.h
>>>> @@ -1595,6 +1595,7 @@ int bpf_obj_get_path(bpfptr_t pathname, int flags);
>>>>
>>>>    struct bpf_iter_aux_info {
>>>>    	struct bpf_map *map;
>>>> +	u64 cgroup_id;
>>>>    };
>>>>
>>>>    typedef int (*bpf_iter_attach_target_t)(struct bpf_prog *prog,
>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>> index a5dbc794403d..855ad80d9983 100644
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -91,6 +91,9 @@ union bpf_iter_link_info {
>>>>    	struct {
>>>>    		__u32	map_fd;
>>>>    	} map;
>>>> +	struct {
>>>> +		__u64	cgroup_id;
>>>> +	} cgroup;
>>>>    };
>>>>
>>>>    /* BPF syscall commands, see bpf(2) man-page for more details. */
>>>> @@ -5887,6 +5890,9 @@ struct bpf_link_info {
>>>>    				struct {
>>>>    					__u32 map_id;
>>>>    				} map;
>>>> +				struct {
>>>> +					__u64 cgroup_id;
>>>> +				} cgroup;
>>>>    			};
>>>>    		} iter;
>>>>    		struct  {
>>>> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
>>>> index c1a9be6a4b9f..52a0e4c6e96e 100644
>>>> --- a/kernel/bpf/Makefile
>>>> +++ b/kernel/bpf/Makefile
>>>> @@ -8,7 +8,7 @@ CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-nogcse-yy)
>>>>
>>>>    obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
>>>>    obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o bloom_filter.o
>>>> -obj-$(CONFIG_BPF_SYSCALL) += local_storage.o queue_stack_maps.o ringbuf.o
>>>> +obj-$(CONFIG_BPF_SYSCALL) += local_storage.o queue_stack_maps.o ringbuf.o cgroup_iter.o
>>>>    obj-$(CONFIG_BPF_SYSCALL) += bpf_local_storage.o bpf_task_storage.o
>>>>    obj-${CONFIG_BPF_LSM}	  += bpf_inode_storage.o
>>>>    obj-$(CONFIG_BPF_SYSCALL) += disasm.o
>>>> diff --git a/kernel/bpf/cgroup_iter.c b/kernel/bpf/cgroup_iter.c
>>>> new file mode 100644
>>>> index 000000000000..011d9dcd1d51
>>>> --- /dev/null
>>>> +++ b/kernel/bpf/cgroup_iter.c
>>>> @@ -0,0 +1,141 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/* Copyright (c) 2022 Google */
>>>> +#include <linux/bpf.h>
>>>> +#include <linux/btf_ids.h>
>>>> +#include <linux/cgroup.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/seq_file.h>
>>>> +
>>>> +struct bpf_iter__cgroup {
>>>> +	__bpf_md_ptr(struct bpf_iter_meta *, meta);
>>>> +	__bpf_md_ptr(struct cgroup *, cgroup);
>>>> +};
>>>> +
>>>> +static void *cgroup_iter_seq_start(struct seq_file *seq, loff_t *pos)
>>>> +{
>>>> +	struct cgroup *cgroup;
>>>> +	u64 cgroup_id;
>>>> +
>>>> +	/* Only one session is supported. */
>>>> +	if (*pos > 0)
>>>> +		return NULL;
>>>> +
>>>> +	cgroup_id = *(u64 *)seq->private;
>>>> +	cgroup = cgroup_get_from_id(cgroup_id);
>>>> +	if (!cgroup)
>>>> +		return NULL;
>>>> +
>>>> +	if (*pos == 0)
>>>> +		++*pos;
>>>> +
>>>> +	return cgroup;
>>>> +}
>>>> +
>>>> +static void *cgroup_iter_seq_next(struct seq_file *seq, void *v, loff_t *pos)
>>>> +{
>>>> +	++*pos;
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +static int cgroup_iter_seq_show(struct seq_file *seq, void *v)
>>>> +{
>>>> +	struct bpf_iter__cgroup ctx;
>>>> +	struct bpf_iter_meta meta;
>>>> +	struct bpf_prog *prog;
>>>> +	int ret = 0;
>>>> +
>>>> +	ctx.meta = &meta;
>>>> +	ctx.cgroup = v;
>>>> +	meta.seq = seq;
>>>> +	prog = bpf_iter_get_info(&meta, false);
>>>> +	if (prog)
>>>> +		ret = bpf_iter_run_prog(prog, &ctx);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void cgroup_iter_seq_stop(struct seq_file *seq, void *v)
>>>> +{
>>>> +	if (v)
>>>> +		cgroup_put(v);
>>>> +}
>>>
>>> I think in existing iterators, we make a final call to seq_show, with v as NULL,
>>> is there a specific reason to do it differently for this? There is logic in
>>> bpf_iter.c to trigger ->stop() callback again when ->start() or ->next() returns
>>> NULL, to execute BPF program with NULL p, see the comment above stop label.
>>>
>>> If you do add the seq_show call with NULL, you'd also need to change the
>>> ctx_arg_info PTR_TO_BTF_ID to PTR_TO_BTF_ID_OR_NULL.
>>
>> Kumar, PTR_TO_BTF_ID should be okay since the show() never takes a non-NULL
>> cgroup. But we do have issues for cgroup_iter_seq_stop() which I missed
>> earlier.
>>
> 
> Right, I was thinking whether it should call seq_show for v == NULL case. All
> other iterators seem to do so, it's a bit different here since it is only
> iterating over a single cgroup, I guess, but it would be nice to have some
> consistency.

You are correct that I think it is okay since it only iterates with one
cgroup. This is different from other cases so far where more than one 
objects may be traversed. We may have future other use cases, e.g.,
one task. I think we can abstract out start()/next()/stop() callbacks
for such use cases. So it is okay it is different from other existing
iterators since they are indeed different.

> 
>> For cgroup_iter, the following is the current workflow:
>>     start -> not NULL -> show -> next -> NULL -> stop
>> or
>>     start -> NULL -> stop
>>
>> So for cgroup_iter_seq_stop, the input parameter 'v' will be NULL, so
>> the cgroup_put() is not actually called, i.e., corresponding cgroup is
>> not freed.
>>
>> There are two ways to fix the issue:
>>    . call cgroup_put() in next() before return NULL. This way,
>>      stop() will be a noop.
>>    . put cgroup_get_from_id() and cgroup_put() in
>>      bpf_iter_attach_cgroup() and bpf_iter_detach_cgroup().
>>
>> I prefer the second approach as it is cleaner.
>>
> 
> I think current approach is also not safe if cgroup_id gets reused, right? I.e.
> it only does cgroup_get_from_id in seq_start, not at attach time, so it may not
> be the same cgroup when calling read(2). kernfs is using idr_alloc_cyclic, so it
> is less likely to occur, but since it wraps around to find a free ID it might
> not be theoretical.

As Alexei mentioned, cgroup id is 64-bit, the collision should
be nearly impossible. Another option is to get a fd from
the cgroup path, and send the fd to the kernel. This probably
works.

> 
>>>
>>>> +
>>>> +static const struct seq_operations cgroup_iter_seq_ops = {
>>>> +	.start  = cgroup_iter_seq_start,
>>>> +	.next   = cgroup_iter_seq_next,
>>>> +	.stop   = cgroup_iter_seq_stop,
>>>> +	.show   = cgroup_iter_seq_show,
>>>> +};
>>>> +
>>>> +BTF_ID_LIST_SINGLE(bpf_cgroup_btf_id, struct, cgroup)
>>>> +
>>>> +static int cgroup_iter_seq_init(void *priv_data, struct bpf_iter_aux_info *aux)
>>>> +{
>>>> +	*(u64 *)priv_data = aux->cgroup_id;
>>>> +	return 0;
>>>> +}
>>>> +
[...]