Cgroup_iter is a type of bpf_iter. It walks over cgroups in four modes:
- walking a cgroup's descendants in pre-order.
- walking a cgroup's descendants in post-order.
- walking a cgroup's ancestors.
- process only the given cgroup.
When attaching cgroup_iter, one can set a cgroup to the iter_link
created from attaching. This cgroup is passed as a file descriptor
or cgroup id and serves as the starting point of the walk. If no
cgroup is specified, the starting point will be the root cgroup v2.
For walking descendants, one can specify the order: either pre-order or
post-order. For walking ancestors, the walk starts at the specified
cgroup and ends at the root.
One can also terminate the walk early by returning 1 from the iter
program.
Note that because walking cgroup hierarchy holds cgroup_mutex, the iter
program is called with cgroup_mutex held.
Currently only one session is supported, which means, depending on the
volume of data bpf program intends to send to user space, the number
of cgroups that can be walked is limited. For example, given the current
buffer size is 8 * PAGE_SIZE, if the program sends 64B data for each
cgroup, assuming PAGE_SIZE is 4kb, the total number of cgroups that can
be walked is 512. This is a limitation of cgroup_iter. If the output
data is larger than the kernel buffer size, after all data in the
kernel buffer is consumed by user space, the subsequent read() syscall
will signal EOPNOTSUPP. In order to work around, the user may have to
update their program to reduce the volume of data sent to output. For
example, skip some uninteresting cgroups. In future, we may extend
bpf_iter flags to allow customizing buffer size.
Acked-by: Yonghong Song <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Signed-off-by: Hao Luo <[email protected]>
---
include/linux/bpf.h | 8 +
include/uapi/linux/bpf.h | 30 ++
kernel/bpf/Makefile | 3 +
kernel/bpf/cgroup_iter.c | 284 ++++++++++++++++++
tools/include/uapi/linux/bpf.h | 30 ++
.../selftests/bpf/prog_tests/btf_dump.c | 4 +-
6 files changed, 357 insertions(+), 2 deletions(-)
create mode 100644 kernel/bpf/cgroup_iter.c
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 39bd36359c1e..dc9bf5e95ebf 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -48,6 +48,7 @@ struct mem_cgroup;
struct module;
struct bpf_func_state;
struct ftrace_ops;
+struct cgroup;
extern struct idr btf_idr;
extern spinlock_t btf_idr_lock;
@@ -1730,7 +1731,14 @@ int bpf_obj_get_user(const char __user *pathname, int flags);
int __init bpf_iter_ ## target(args) { return 0; }
struct bpf_iter_aux_info {
+ /* for map_elem iter */
struct bpf_map *map;
+
+ /* for cgroup iter */
+ struct {
+ struct cgroup *start; /* starting cgroup */
+ enum bpf_cgroup_iter_order order;
+ } cgroup;
};
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 934a2a8beb87..1c4e1c583880 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -87,10 +87,29 @@ struct bpf_cgroup_storage_key {
__u32 attach_type; /* program attach type (enum bpf_attach_type) */
};
+enum bpf_cgroup_iter_order {
+ BPF_ITER_ORDER_UNSPEC = 0,
+ BPF_ITER_SELF_ONLY, /* process only a single object. */
+ BPF_ITER_DESCENDANTS_PRE, /* walk descendants in pre-order. */
+ BPF_ITER_DESCENDANTS_POST, /* walk descendants in post-order. */
+ BPF_ITER_ANCESTORS_UP, /* walk ancestors upward. */
+};
+
union bpf_iter_link_info {
struct {
__u32 map_fd;
} map;
+ struct {
+ enum bpf_cgroup_iter_order order;
+
+ /* At most one of cgroup_fd and cgroup_id can be non-zero. If
+ * both are zero, the walk starts from the default cgroup v2
+ * root. For walking v1 hierarchy, one should always explicitly
+ * specify cgroup_fd.
+ */
+ __u32 cgroup_fd;
+ __u64 cgroup_id;
+ } cgroup;
};
/* BPF syscall commands, see bpf(2) man-page for more details. */
@@ -6159,11 +6178,22 @@ struct bpf_link_info {
struct {
__aligned_u64 target_name; /* in/out: target_name buffer ptr */
__u32 target_name_len; /* in/out: target_name buffer len */
+
+ /* If the iter specific field is 32 bits, it can be put
+ * in the first or second union. Otherwise it should be
+ * put in the second union.
+ */
union {
struct {
__u32 map_id;
} map;
};
+ union {
+ struct {
+ __u64 cgroup_id;
+ __u32 order;
+ } cgroup;
+ };
} iter;
struct {
__u32 netns_ino;
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index 057ba8e01e70..00e05b69a4df 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -24,6 +24,9 @@ endif
ifeq ($(CONFIG_PERF_EVENTS),y)
obj-$(CONFIG_BPF_SYSCALL) += stackmap.o
endif
+ifeq ($(CONFIG_CGROUPS),y)
+obj-$(CONFIG_BPF_SYSCALL) += cgroup_iter.o
+endif
obj-$(CONFIG_CGROUP_BPF) += cgroup.o
ifeq ($(CONFIG_INET),y)
obj-$(CONFIG_BPF_SYSCALL) += reuseport_array.o
diff --git a/kernel/bpf/cgroup_iter.c b/kernel/bpf/cgroup_iter.c
new file mode 100644
index 000000000000..cf6d763a57d5
--- /dev/null
+++ b/kernel/bpf/cgroup_iter.c
@@ -0,0 +1,284 @@
+// 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>
+
+#include "../cgroup/cgroup-internal.h" /* cgroup_mutex and cgroup_is_dead */
+
+/* cgroup_iter provides four modes of traversal to the cgroup hierarchy.
+ *
+ * 1. Walk the descendants of a cgroup in pre-order.
+ * 2. Walk the descendants of a cgroup in post-order.
+ * 3. Walk the ancestors of a cgroup.
+ * 4. Show the given cgroup only.
+ *
+ * For walking descendants, cgroup_iter can walk in either pre-order or
+ * post-order. For walking ancestors, the iter walks up from a cgroup to
+ * the root.
+ *
+ * The iter program can terminate the walk early by returning 1. Walk
+ * continues if prog returns 0.
+ *
+ * The prog can check (seq->num == 0) to determine whether this is
+ * the first element. The prog may also be passed a NULL cgroup,
+ * which means the walk has completed and the prog has a chance to
+ * do post-processing, such as outputting an epilogue.
+ *
+ * Note: the iter_prog is called with cgroup_mutex held.
+ *
+ * Currently only one session is supported, which means, depending on the
+ * volume of data bpf program intends to send to user space, the number
+ * of cgroups that can be walked is limited. For example, given the current
+ * buffer size is 8 * PAGE_SIZE, if the program sends 64B data for each
+ * cgroup, assuming PAGE_SIZE is 4kb, the total number of cgroups that can
+ * be walked is 512. This is a limitation of cgroup_iter. If the output data
+ * is larger than the kernel buffer size, after all data in the kernel buffer
+ * is consumed by user space, the subsequent read() syscall will signal
+ * EOPNOTSUPP. In order to work around, the user may have to update their
+ * program to reduce the volume of data sent to output. For example, skip
+ * some uninteresting cgroups.
+ */
+
+struct bpf_iter__cgroup {
+ __bpf_md_ptr(struct bpf_iter_meta *, meta);
+ __bpf_md_ptr(struct cgroup *, cgroup);
+};
+
+struct cgroup_iter_priv {
+ struct cgroup_subsys_state *start_css;
+ bool visited_all;
+ bool terminate;
+ int order;
+};
+
+static void *cgroup_iter_seq_start(struct seq_file *seq, loff_t *pos)
+{
+ struct cgroup_iter_priv *p = seq->private;
+
+ mutex_lock(&cgroup_mutex);
+
+ /* cgroup_iter doesn't support read across multiple sessions. */
+ if (*pos > 0) {
+ if (p->visited_all)
+ return NULL;
+
+ /* Haven't visited all, but because cgroup_mutex has dropped,
+ * return -EOPNOTSUPP to indicate incomplete iteration.
+ */
+ return ERR_PTR(-EOPNOTSUPP);
+ }
+
+ ++*pos;
+ p->terminate = false;
+ p->visited_all = false;
+ if (p->order == BPF_ITER_DESCENDANTS_PRE)
+ return css_next_descendant_pre(NULL, p->start_css);
+ else if (p->order == BPF_ITER_DESCENDANTS_POST)
+ return css_next_descendant_post(NULL, p->start_css);
+ else if (p->order == BPF_ITER_ANCESTORS_UP)
+ return p->start_css;
+ else /* BPF_ITER_SELF_ONLY */
+ return p->start_css;
+}
+
+static int __cgroup_iter_seq_show(struct seq_file *seq,
+ struct cgroup_subsys_state *css, int in_stop);
+
+static void cgroup_iter_seq_stop(struct seq_file *seq, void *v)
+{
+ struct cgroup_iter_priv *p = seq->private;
+
+ mutex_unlock(&cgroup_mutex);
+
+ /* pass NULL to the prog for post-processing */
+ if (!v) {
+ __cgroup_iter_seq_show(seq, NULL, true);
+ p->visited_all = true;
+ }
+}
+
+static void *cgroup_iter_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+ struct cgroup_subsys_state *curr = (struct cgroup_subsys_state *)v;
+ struct cgroup_iter_priv *p = seq->private;
+
+ ++*pos;
+ if (p->terminate)
+ return NULL;
+
+ if (p->order == BPF_ITER_DESCENDANTS_PRE)
+ return css_next_descendant_pre(curr, p->start_css);
+ else if (p->order == BPF_ITER_DESCENDANTS_POST)
+ return css_next_descendant_post(curr, p->start_css);
+ else if (p->order == BPF_ITER_ANCESTORS_UP)
+ return curr->parent;
+ else /* BPF_ITER_SELF_ONLY */
+ return NULL;
+}
+
+static int __cgroup_iter_seq_show(struct seq_file *seq,
+ struct cgroup_subsys_state *css, int in_stop)
+{
+ struct cgroup_iter_priv *p = seq->private;
+ struct bpf_iter__cgroup ctx;
+ struct bpf_iter_meta meta;
+ struct bpf_prog *prog;
+ int ret = 0;
+
+ /* cgroup is dead, skip this element */
+ if (css && cgroup_is_dead(css->cgroup))
+ return 0;
+
+ ctx.meta = &meta;
+ ctx.cgroup = css ? css->cgroup : NULL;
+ meta.seq = seq;
+ prog = bpf_iter_get_info(&meta, in_stop);
+ if (prog)
+ ret = bpf_iter_run_prog(prog, &ctx);
+
+ /* if prog returns > 0, terminate after this element. */
+ if (ret != 0)
+ p->terminate = true;
+
+ return 0;
+}
+
+static int cgroup_iter_seq_show(struct seq_file *seq, void *v)
+{
+ return __cgroup_iter_seq_show(seq, (struct cgroup_subsys_state *)v,
+ false);
+}
+
+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, struct bpf_iter_aux_info *aux)
+{
+ struct cgroup_iter_priv *p = (struct cgroup_iter_priv *)priv;
+ struct cgroup *cgrp = aux->cgroup.start;
+
+ p->start_css = &cgrp->self;
+ p->terminate = false;
+ p->visited_all = false;
+ p->order = aux->cgroup.order;
+ return 0;
+}
+
+static const struct bpf_iter_seq_info cgroup_iter_seq_info = {
+ .seq_ops = &cgroup_iter_seq_ops,
+ .init_seq_private = cgroup_iter_seq_init,
+ .seq_priv_size = sizeof(struct cgroup_iter_priv),
+};
+
+static int bpf_iter_attach_cgroup(struct bpf_prog *prog,
+ union bpf_iter_link_info *linfo,
+ struct bpf_iter_aux_info *aux)
+{
+ int fd = linfo->cgroup.cgroup_fd;
+ u64 id = linfo->cgroup.cgroup_id;
+ int order = linfo->cgroup.order;
+ struct cgroup *cgrp;
+
+ if (order != BPF_ITER_DESCENDANTS_PRE &&
+ order != BPF_ITER_DESCENDANTS_POST &&
+ order != BPF_ITER_ANCESTORS_UP &&
+ order != BPF_ITER_SELF_ONLY)
+ return -EINVAL;
+
+ if (fd && id)
+ return -EINVAL;
+
+ if (fd)
+ cgrp = cgroup_get_from_fd(fd);
+ else if (id)
+ cgrp = cgroup_get_from_id(id);
+ else /* walk the entire hierarchy by default. */
+ cgrp = cgroup_get_from_path("/");
+
+ if (IS_ERR(cgrp))
+ return PTR_ERR(cgrp);
+
+ aux->cgroup.start = cgrp;
+ aux->cgroup.order = order;
+ return 0;
+}
+
+static void bpf_iter_detach_cgroup(struct bpf_iter_aux_info *aux)
+{
+ cgroup_put(aux->cgroup.start);
+}
+
+static void bpf_iter_cgroup_show_fdinfo(const struct bpf_iter_aux_info *aux,
+ struct seq_file *seq)
+{
+ char *buf;
+
+ buf = kzalloc(PATH_MAX, GFP_KERNEL);
+ if (!buf) {
+ seq_puts(seq, "cgroup_path:\t<unknown>\n");
+ goto show_order;
+ }
+
+ /* If cgroup_path_ns() fails, buf will be an empty string, cgroup_path
+ * will print nothing.
+ *
+ * Path is in the calling process's cgroup namespace.
+ */
+ cgroup_path_ns(aux->cgroup.start, buf, PATH_MAX,
+ current->nsproxy->cgroup_ns);
+ seq_printf(seq, "cgroup_path:\t%s\n", buf);
+ kfree(buf);
+
+show_order:
+ if (aux->cgroup.order == BPF_ITER_DESCENDANTS_PRE)
+ seq_puts(seq, "order: descendants_pre\n");
+ else if (aux->cgroup.order == BPF_ITER_DESCENDANTS_POST)
+ seq_puts(seq, "order: descendants_post\n");
+ else if (aux->cgroup.order == BPF_ITER_ANCESTORS_UP)
+ seq_puts(seq, "order: ancestors_up\n");
+ else /* BPF_ITER_SELF_ONLY */
+ seq_puts(seq, "order: self_only\n");
+}
+
+static int bpf_iter_cgroup_fill_link_info(const struct bpf_iter_aux_info *aux,
+ struct bpf_link_info *info)
+{
+ info->iter.cgroup.order = aux->cgroup.order;
+ info->iter.cgroup.cgroup_id = cgroup_id(aux->cgroup.start);
+ return 0;
+}
+
+DEFINE_BPF_ITER_FUNC(cgroup, struct bpf_iter_meta *meta,
+ struct cgroup *cgroup)
+
+static struct bpf_iter_reg bpf_cgroup_reg_info = {
+ .target = "cgroup",
+ .feature = BPF_ITER_RESCHED,
+ .attach_target = bpf_iter_attach_cgroup,
+ .detach_target = bpf_iter_detach_cgroup,
+ .show_fdinfo = bpf_iter_cgroup_show_fdinfo,
+ .fill_link_info = bpf_iter_cgroup_fill_link_info,
+ .ctx_arg_info_size = 1,
+ .ctx_arg_info = {
+ { offsetof(struct bpf_iter__cgroup, cgroup),
+ PTR_TO_BTF_ID_OR_NULL },
+ },
+ .seq_info = &cgroup_iter_seq_info,
+};
+
+static int __init bpf_cgroup_iter_init(void)
+{
+ bpf_cgroup_reg_info.ctx_arg_info[0].btf_id = bpf_cgroup_btf_id[0];
+ return bpf_iter_reg_target(&bpf_cgroup_reg_info);
+}
+
+late_initcall(bpf_cgroup_iter_init);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 1d6085e15fc8..9696f05bc924 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -87,10 +87,29 @@ struct bpf_cgroup_storage_key {
__u32 attach_type; /* program attach type (enum bpf_attach_type) */
};
+enum bpf_cgroup_iter_order {
+ BPF_ITER_ORDER_UNSPEC = 0,
+ BPF_ITER_SELF_ONLY, /* process only a single object. */
+ BPF_ITER_DESCENDANTS_PRE, /* walk descendants in pre-order. */
+ BPF_ITER_DESCENDANTS_POST, /* walk descendants in post-order. */
+ BPF_ITER_ANCESTORS_UP, /* walk ancestors upward. */
+};
+
union bpf_iter_link_info {
struct {
__u32 map_fd;
} map;
+ struct {
+ enum bpf_cgroup_iter_order order;
+
+ /* At most one of cgroup_fd and cgroup_id can be non-zero. If
+ * both are zero, the walk starts from the default cgroup v2
+ * root. For walking v1 hierarchy, one should always explicitly
+ * specify cgroup_fd.
+ */
+ __u32 cgroup_fd;
+ __u64 cgroup_id;
+ } cgroup;
};
/* BPF syscall commands, see bpf(2) man-page for more details. */
@@ -6159,11 +6178,22 @@ struct bpf_link_info {
struct {
__aligned_u64 target_name; /* in/out: target_name buffer ptr */
__u32 target_name_len; /* in/out: target_name buffer len */
+
+ /* If the iter specific field is 32 bits, it can be put
+ * in the first or second union. Otherwise it should be
+ * put in the second union.
+ */
union {
struct {
__u32 map_id;
} map;
};
+ union {
+ struct {
+ __u64 cgroup_id;
+ __u32 order;
+ } cgroup;
+ };
} iter;
struct {
__u32 netns_ino;
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index 5fce7008d1ff..84c1cfaa2b02 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -764,8 +764,8 @@ static void test_btf_dump_struct_data(struct btf *btf, struct btf_dump *d,
/* union with nested struct */
TEST_BTF_DUMP_DATA(btf, d, "union", str, union bpf_iter_link_info, BTF_F_COMPACT,
- "(union bpf_iter_link_info){.map = (struct){.map_fd = (__u32)1,},}",
- { .map = { .map_fd = 1 }});
+ "(union bpf_iter_link_info){.map = (struct){.map_fd = (__u32)1,},.cgroup = (struct){.order = (__u32)1,.cgroup_fd = (__u32)1,},}",
+ { .cgroup = { .order = 1, .cgroup_fd = 1, }});
/* struct skb with nested structs/unions; because type output is so
* complex, we don't do a string comparison, just verify we return
--
2.37.1.595.g718a3a8f04-goog
Hello.
On Tue, Aug 23, 2022 at 08:00:27PM -0700, Hao Luo <[email protected]> wrote:
> +static int bpf_iter_attach_cgroup(struct bpf_prog *prog,
> + union bpf_iter_link_info *linfo,
> + struct bpf_iter_aux_info *aux)
> +{
> + int fd = linfo->cgroup.cgroup_fd;
> + u64 id = linfo->cgroup.cgroup_id;
> + int order = linfo->cgroup.order;
> + struct cgroup *cgrp;
> +
> + if (order != BPF_ITER_DESCENDANTS_PRE &&
> + order != BPF_ITER_DESCENDANTS_POST &&
> + order != BPF_ITER_ANCESTORS_UP &&
> + order != BPF_ITER_SELF_ONLY)
> + return -EINVAL;
> +
> + if (fd && id)
> + return -EINVAL;
> +
> + if (fd)
> + cgrp = cgroup_get_from_fd(fd);
> + else if (id)
> + cgrp = cgroup_get_from_id(id);
> + else /* walk the entire hierarchy by default. */
> + cgrp = cgroup_get_from_path("/");
> +
> + if (IS_ERR(cgrp))
> + return PTR_ERR(cgrp);
This section caught my eye.
Perhaps the simpler way for the default hierachy fallback would be
cgrp = &cgrp_dfl_root.cgrp;
cgroup_get(cgroup)
But maybe it's not what is the intention if cgroup NS should be taken
into account and cgroup_get_from_path() is buggy in this regard.
Would it make sense to prepend the patch below to your series?
Also, that makes me think about iter initialization with ID. In contrast
with FD passing (that's subject to some permissions and NS checks), the
retrieval via ID is not equipped with that, ids are not unguessable and
I'd consider cgroup IDs an implementation detail.
So, is the ID initialization that much useful? (I have no idea about
permissions model of BPF here, so it might be just fine but still it'd
be good to take cgroup NS into account. Likely for BPF_ITER_ANCESTORS_UP
too.)
HTH,
Michal
----8<----
From 1098e60e89d4d901b7eef04e531f2c889309a91b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20Koutn=C3=BD?= <[email protected]>
Date: Thu, 25 Aug 2022 15:19:04 +0200
Subject: [PATCH] cgroup: Honor caller's cgroup NS when resolving path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
cgroup_get_from_path() is not widely used function. Its callers presume
the path is resolved under cgroup namespace. (There is one caller
currently and resolving in init NS won't make harm (netfilter). However,
future users may be subject to different effects when resolving
globally.)
Since, there's currently no use for the global resolution, modify the
existing function to take cgroup NS into account.
Fixes: a79a908fd2b0 ("cgroup: introduce cgroup namespaces")
Signed-off-by: Michal Koutn? <[email protected]>
---
kernel/cgroup/cgroup.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index ffaccd6373f1..9280f4b41d8b 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6603,8 +6603,12 @@ struct cgroup *cgroup_get_from_path(const char *path)
{
struct kernfs_node *kn;
struct cgroup *cgrp = ERR_PTR(-ENOENT);
+ struct cgroup *root_cgrp;
- kn = kernfs_walk_and_get(cgrp_dfl_root.cgrp.kn, path);
+ spin_lock_irq(&css_set_lock);
+ root_cgrp = current_cgns_cgroup_from_root(&cgrp_dfl_root);
+ kn = kernfs_walk_and_get(root_cgrp->kn, path);
+ spin_unlock_irq(&css_set_lock);
if (!kn)
goto out;
base-commit: 3cc40a443a04d52b0c95255dce264068b01e9bfe
--
2.37.0
On Thu, Aug 25, 2022 at 8:24 AM Michal Koutný <[email protected]> wrote:
>
> Hello.
>
> On Tue, Aug 23, 2022 at 08:00:27PM -0700, Hao Luo <[email protected]> wrote:
> > +static int bpf_iter_attach_cgroup(struct bpf_prog *prog,
> > + union bpf_iter_link_info *linfo,
> > + struct bpf_iter_aux_info *aux)
> > +{
> > + int fd = linfo->cgroup.cgroup_fd;
> > + u64 id = linfo->cgroup.cgroup_id;
> > + int order = linfo->cgroup.order;
> > + struct cgroup *cgrp;
> > +
> > + if (order != BPF_ITER_DESCENDANTS_PRE &&
> > + order != BPF_ITER_DESCENDANTS_POST &&
> > + order != BPF_ITER_ANCESTORS_UP &&
> > + order != BPF_ITER_SELF_ONLY)
> > + return -EINVAL;
> > +
> > + if (fd && id)
> > + return -EINVAL;
> > +
> > + if (fd)
> > + cgrp = cgroup_get_from_fd(fd);
> > + else if (id)
> > + cgrp = cgroup_get_from_id(id);
> > + else /* walk the entire hierarchy by default. */
> > + cgrp = cgroup_get_from_path("/");
> > +
> > + if (IS_ERR(cgrp))
> > + return PTR_ERR(cgrp);
>
> This section caught my eye.
>
> Perhaps the simpler way for the default hierachy fallback would be
>
> cgrp = &cgrp_dfl_root.cgrp;
> cgroup_get(cgroup)
>
> But maybe it's not what is the intention if cgroup NS should be taken
> into account and cgroup_get_from_path() is buggy in this regard.
>
> Would it make sense to prepend the patch below to your series?
Yep. Being able to take cgroup NS into account is my intention here.
Right now, I imagine the control plane who uses this default option is
the system daemon which lives in the init cgroup ns. They could always
specify a particular cgroup using ID or FD. Printing the whole
hierarchy is something a system daemon would do. Anyhow, can I add the
appended patch if there is going to be a v10 of this patch series? If
v9 is ok to merge, I can send the appended patch separately. Does that
sound good? The appended patch looks good to me, thanks!
>
> Also, that makes me think about iter initialization with ID. In contrast
> with FD passing (that's subject to some permissions and NS checks), the
> retrieval via ID is not equipped with that, ids are not unguessable and
> I'd consider cgroup IDs an implementation detail.
>
> So, is the ID initialization that much useful? (I have no idea about
> permissions model of BPF here, so it might be just fine but still it'd
> be good to take cgroup NS into account. Likely for BPF_ITER_ANCESTORS_UP
> too.)
>
Permission is a valid point about FD. There was discussion in an
earlier version of this patch series [0]. The good thing about ID is
that it can be passed across processes and it's meaningful to appear
in logs. It's more user-friendly. So we decided to support both.
[0] https://lore.kernel.org/netdev/[email protected]/
> HTH,
> Michal
>
> ----8<----
> From 1098e60e89d4d901b7eef04e531f2c889309a91b Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20Koutn=C3=BD?= <[email protected]>
> Date: Thu, 25 Aug 2022 15:19:04 +0200
> Subject: [PATCH] cgroup: Honor caller's cgroup NS when resolving path
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> cgroup_get_from_path() is not widely used function. Its callers presume
> the path is resolved under cgroup namespace. (There is one caller
> currently and resolving in init NS won't make harm (netfilter). However,
> future users may be subject to different effects when resolving
> globally.)
> Since, there's currently no use for the global resolution, modify the
> existing function to take cgroup NS into account.
>
> Fixes: a79a908fd2b0 ("cgroup: introduce cgroup namespaces")
> Signed-off-by: Michal Koutný <[email protected]>
> ---
> kernel/cgroup/cgroup.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index ffaccd6373f1..9280f4b41d8b 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -6603,8 +6603,12 @@ struct cgroup *cgroup_get_from_path(const char *path)
> {
> struct kernfs_node *kn;
> struct cgroup *cgrp = ERR_PTR(-ENOENT);
> + struct cgroup *root_cgrp;
>
> - kn = kernfs_walk_and_get(cgrp_dfl_root.cgrp.kn, path);
> + spin_lock_irq(&css_set_lock);
> + root_cgrp = current_cgns_cgroup_from_root(&cgrp_dfl_root);
> + kn = kernfs_walk_and_get(root_cgrp->kn, path);
> + spin_unlock_irq(&css_set_lock);
> if (!kn)
> goto out;
>
>
> base-commit: 3cc40a443a04d52b0c95255dce264068b01e9bfe
> --
> 2.37.0
>
On Thu, Aug 25, 2022 at 8:24 AM Michal Koutný <[email protected]> wrote:
>
> Hello.
>
> On Tue, Aug 23, 2022 at 08:00:27PM -0700, Hao Luo <[email protected]> wrote:
> > +static int bpf_iter_attach_cgroup(struct bpf_prog *prog,
> > + union bpf_iter_link_info *linfo,
> > + struct bpf_iter_aux_info *aux)
> > +{
> > + int fd = linfo->cgroup.cgroup_fd;
> > + u64 id = linfo->cgroup.cgroup_id;
> > + int order = linfo->cgroup.order;
> > + struct cgroup *cgrp;
> > +
> > + if (order != BPF_ITER_DESCENDANTS_PRE &&
> > + order != BPF_ITER_DESCENDANTS_POST &&
> > + order != BPF_ITER_ANCESTORS_UP &&
> > + order != BPF_ITER_SELF_ONLY)
> > + return -EINVAL;
> > +
> > + if (fd && id)
> > + return -EINVAL;
> > +
> > + if (fd)
> > + cgrp = cgroup_get_from_fd(fd);
> > + else if (id)
> > + cgrp = cgroup_get_from_id(id);
> > + else /* walk the entire hierarchy by default. */
> > + cgrp = cgroup_get_from_path("/");
> > +
> > + if (IS_ERR(cgrp))
> > + return PTR_ERR(cgrp);
>
> This section caught my eye.
>
> Perhaps the simpler way for the default hierachy fallback would be
>
> cgrp = &cgrp_dfl_root.cgrp;
> cgroup_get(cgroup)
>
> But maybe it's not what is the intention if cgroup NS should be taken
> into account and cgroup_get_from_path() is buggy in this regard.
>
> Would it make sense to prepend the patch below to your series?
Michal, this patch series has merged. Do you need me to send your
patch on your behalf, or you want to do it yourself?
> ----8<----
> From 1098e60e89d4d901b7eef04e531f2c889309a91b Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20Koutn=C3=BD?= <[email protected]>
> Date: Thu, 25 Aug 2022 15:19:04 +0200
> Subject: [PATCH] cgroup: Honor caller's cgroup NS when resolving path
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> cgroup_get_from_path() is not widely used function. Its callers presume
> the path is resolved under cgroup namespace. (There is one caller
> currently and resolving in init NS won't make harm (netfilter). However,
> future users may be subject to different effects when resolving
> globally.)
> Since, there's currently no use for the global resolution, modify the
> existing function to take cgroup NS into account.
>
> Fixes: a79a908fd2b0 ("cgroup: introduce cgroup namespaces")
> Signed-off-by: Michal Koutný <[email protected]>
> ---
> kernel/cgroup/cgroup.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index ffaccd6373f1..9280f4b41d8b 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -6603,8 +6603,12 @@ struct cgroup *cgroup_get_from_path(const char *path)
> {
> struct kernfs_node *kn;
> struct cgroup *cgrp = ERR_PTR(-ENOENT);
> + struct cgroup *root_cgrp;
>
> - kn = kernfs_walk_and_get(cgrp_dfl_root.cgrp.kn, path);
> + spin_lock_irq(&css_set_lock);
> + root_cgrp = current_cgns_cgroup_from_root(&cgrp_dfl_root);
> + kn = kernfs_walk_and_get(root_cgrp->kn, path);
> + spin_unlock_irq(&css_set_lock);
> if (!kn)
> goto out;
>
>
> base-commit: 3cc40a443a04d52b0c95255dce264068b01e9bfe
> --
> 2.37.0
>
On Tue, Aug 23, 2022 at 8:01 PM Hao Luo <[email protected]> wrote:
>
> Cgroup_iter is a type of bpf_iter. It walks over cgroups in four modes:
>
> - walking a cgroup's descendants in pre-order.
> - walking a cgroup's descendants in post-order.
> - walking a cgroup's ancestors.
> - process only the given cgroup.
>
> When attaching cgroup_iter, one can set a cgroup to the iter_link
> created from attaching. This cgroup is passed as a file descriptor
> or cgroup id and serves as the starting point of the walk. If no
> cgroup is specified, the starting point will be the root cgroup v2.
>
> For walking descendants, one can specify the order: either pre-order or
> post-order. For walking ancestors, the walk starts at the specified
> cgroup and ends at the root.
>
> One can also terminate the walk early by returning 1 from the iter
> program.
>
> Note that because walking cgroup hierarchy holds cgroup_mutex, the iter
> program is called with cgroup_mutex held.
>
> Currently only one session is supported, which means, depending on the
> volume of data bpf program intends to send to user space, the number
> of cgroups that can be walked is limited. For example, given the current
> buffer size is 8 * PAGE_SIZE, if the program sends 64B data for each
> cgroup, assuming PAGE_SIZE is 4kb, the total number of cgroups that can
> be walked is 512. This is a limitation of cgroup_iter. If the output
> data is larger than the kernel buffer size, after all data in the
> kernel buffer is consumed by user space, the subsequent read() syscall
> will signal EOPNOTSUPP. In order to work around, the user may have to
> update their program to reduce the volume of data sent to output. For
> example, skip some uninteresting cgroups. In future, we may extend
> bpf_iter flags to allow customizing buffer size.
>
> Acked-by: Yonghong Song <[email protected]>
> Acked-by: Tejun Heo <[email protected]>
> Signed-off-by: Hao Luo <[email protected]>
> ---
> include/linux/bpf.h | 8 +
> include/uapi/linux/bpf.h | 30 ++
> kernel/bpf/Makefile | 3 +
> kernel/bpf/cgroup_iter.c | 284 ++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 30 ++
> .../selftests/bpf/prog_tests/btf_dump.c | 4 +-
> 6 files changed, 357 insertions(+), 2 deletions(-)
> create mode 100644 kernel/bpf/cgroup_iter.c
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 39bd36359c1e..dc9bf5e95ebf 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -48,6 +48,7 @@ struct mem_cgroup;
> struct module;
> struct bpf_func_state;
> struct ftrace_ops;
> +struct cgroup;
>
> extern struct idr btf_idr;
> extern spinlock_t btf_idr_lock;
> @@ -1730,7 +1731,14 @@ int bpf_obj_get_user(const char __user *pathname, int flags);
> int __init bpf_iter_ ## target(args) { return 0; }
>
> struct bpf_iter_aux_info {
> + /* for map_elem iter */
> struct bpf_map *map;
> +
> + /* for cgroup iter */
> + struct {
> + struct cgroup *start; /* starting cgroup */
> + enum bpf_cgroup_iter_order order;
> + } cgroup;
> };
>
> 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 934a2a8beb87..1c4e1c583880 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -87,10 +87,29 @@ struct bpf_cgroup_storage_key {
> __u32 attach_type; /* program attach type (enum bpf_attach_type) */
> };
>
> +enum bpf_cgroup_iter_order {
> + BPF_ITER_ORDER_UNSPEC = 0,
> + BPF_ITER_SELF_ONLY, /* process only a single object. */
> + BPF_ITER_DESCENDANTS_PRE, /* walk descendants in pre-order. */
> + BPF_ITER_DESCENDANTS_POST, /* walk descendants in post-order. */
> + BPF_ITER_ANCESTORS_UP, /* walk ancestors upward. */
> +};
just skimming through this, I noticed that we have "enum
bpf_cgroup_iter_order" (good, I like) but BPF_ITER_xxx with no CGROUP
part in it (not good, don't like :). All the enumerator names have
global visibility, so it would probably be best for them to be
CGROUP-specific and roughly match the enum name itself:
BPF_CGROUP_ITER_SELF_ONLY, etc?
> +
> union bpf_iter_link_info {
> struct {
> __u32 map_fd;
> } map;
> + struct {
> + enum bpf_cgroup_iter_order order;
> +
> + /* At most one of cgroup_fd and cgroup_id can be non-zero. If
> + * both are zero, the walk starts from the default cgroup v2
> + * root. For walking v1 hierarchy, one should always explicitly
> + * specify cgroup_fd.
> + */
> + __u32 cgroup_fd;
> + __u64 cgroup_id;
> + } cgroup;
> };
[...]
On Thu, Aug 25, 2022 at 1:18 PM Andrii Nakryiko
<[email protected]> wrote:
>
> On Tue, Aug 23, 2022 at 8:01 PM Hao Luo <[email protected]> wrote:
> >
[...]
> > 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 934a2a8beb87..1c4e1c583880 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -87,10 +87,29 @@ struct bpf_cgroup_storage_key {
> > __u32 attach_type; /* program attach type (enum bpf_attach_type) */
> > };
> >
> > +enum bpf_cgroup_iter_order {
> > + BPF_ITER_ORDER_UNSPEC = 0,
> > + BPF_ITER_SELF_ONLY, /* process only a single object. */
> > + BPF_ITER_DESCENDANTS_PRE, /* walk descendants in pre-order. */
> > + BPF_ITER_DESCENDANTS_POST, /* walk descendants in post-order. */
> > + BPF_ITER_ANCESTORS_UP, /* walk ancestors upward. */
> > +};
>
> just skimming through this, I noticed that we have "enum
> bpf_cgroup_iter_order" (good, I like) but BPF_ITER_xxx with no CGROUP
> part in it (not good, don't like :). All the enumerator names have
> global visibility, so it would probably be best for them to be
> CGROUP-specific and roughly match the enum name itself:
> BPF_CGROUP_ITER_SELF_ONLY, etc?
>
Ack. I will send a patch to fix this right now.
On Thu, Aug 25, 2022 at 11:56:11AM -0700, Hao Luo <[email protected]> wrote:
> Michal, this patch series has merged. Do you need me to send your
> patch on your behalf, or you want to do it yourself?
I added some more fixups. Continuation at [1].
Thanks,
Michal
[1] https://lore.kernel.org/r/[email protected]/
On Thu, Aug 25, 2022 at 10:58:26AM -0700, Hao Luo <[email protected]> wrote:
> Permission is a valid point about FD. There was discussion in an
> earlier version of this patch series [0].
(I'm sorry, I didn't follow all the version discussions closely.)
I think the permissions are a non-issue when unprivileged BPF is
disabled. If it's allowed, I think it'd be better solved generally
within the BPF iterator framework. (Maybe it's already present, I didn't
check.)
(OT:
> The good thing about ID is that it can be passed across processes
FDs can be passed too (parent-child trivially, others via SCM_RIGHTS
message).
> and it's meaningful to appear in logs. It's more user-friendly.
I'd say cgroup path wins both in meaning and user friendliness.
(Or maybe you meant different class of users.)
)
> So we decided to support both.
I accept cgroup ids are an establish{ing,ed} way to refer to cgroups
from userspace. Hence my fixups for the BPF cgroup iter (another thread)
for better namespacing consisntency.
Thanks,
Michal