Re: [PATCH bpf-next v9 1/5] bpf: Introduce cgroup iter

On Thu, Aug 25, 2022 at 8:24 AM Michal Koutný [off-list ref] wrote:

Hello.

On Tue, Aug 23, 2022 at 08:00:27PM -0700, Hao Luo [off-list ref] wrote:

quoted

+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/YuK+eg3lgwJ2CJnJ@slm.duckdns.org/ (local)