Re: [PATCH v2 bpf-next 4/6] bpf: implement bpf_prog replacement for an active bpf_cgroup_link

On Thu, Mar 26, 2020 at 5:34 PM Alexei Starovoitov
[off-list ref] wrote:

On Thu, Mar 26, 2020 at 04:59:06PM -0700, Andrii Nakryiko wrote:

quoted

On Thu, Mar 26, 2020 at 4:35 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Tue, Mar 24, 2020 at 11:57:44PM -0700, Andrii Nakryiko wrote:

quoted

+/* Swap updated BPF program for given link in effective program arrays across
+ * all descendant cgroups. This function is guaranteed to succeed.
+ */
+static void replace_effective_prog(struct cgroup *cgrp,
+                                enum bpf_attach_type type,
+                                struct bpf_cgroup_link *link)
+{
+     struct bpf_prog_array_item *item;
+     struct cgroup_subsys_state *css;
+     struct bpf_prog_array *progs;
+     struct bpf_prog_list *pl;
+     struct list_head *head;
+     struct cgroup *cg;
+     int pos;
+
+     css_for_each_descendant_pre(css, &cgrp->self) {
+             struct cgroup *desc = container_of(css, struct cgroup, self);
+
+             if (percpu_ref_is_zero(&desc->bpf.refcnt))
+                     continue;
+
+             /* found position of link in effective progs array */
+             for (pos = 0, cg = desc; cg; cg = cgroup_parent(cg)) {
+                     if (pos && !(cg->bpf.flags[type] & BPF_F_ALLOW_MULTI))
+                             continue;
+
+                     head = &cg->bpf.progs[type];
+                     list_for_each_entry(pl, head, node) {
+                             if (!prog_list_prog(pl))
+                                     continue;
+                             if (pl->link == link)
+                                     goto found;
+                             pos++;
+                     }
+             }
+found:
+             BUG_ON(!cg);
+             progs = rcu_dereference_protected(
+                             desc->bpf.effective[type],
+                             lockdep_is_held(&cgroup_mutex));
+             item = &progs->items[pos];
+             WRITE_ONCE(item->prog, link->link.prog);
+     }
+}
+
+/**
+ * __cgroup_bpf_replace() - Replace link's program and propagate the change
+ *                          to descendants
+ * @cgrp: The cgroup which descendants to traverse
+ * @link: A link for which to replace BPF program
+ * @type: Type of attach operation
+ *
+ * Must be called with cgroup_mutex held.
+ */
+int __cgroup_bpf_replace(struct cgroup *cgrp, struct bpf_cgroup_link *link,
+                      struct bpf_prog *new_prog)
+{
+     struct list_head *progs = &cgrp->bpf.progs[link->type];
+     struct bpf_prog *old_prog;
+     struct bpf_prog_list *pl;
+     bool found = false;
+
+     if (link->link.prog->type != new_prog->type)
+             return -EINVAL;
+
+     list_for_each_entry(pl, progs, node) {
+             if (pl->link == link) {
+                     found = true;
+                     break;
+             }
+     }
+     if (!found)
+             return -ENOENT;
+
+     old_prog = xchg(&link->link.prog, new_prog);
+     replace_effective_prog(cgrp, link->type, link);

I think with 'found = true' in this function you're assuming that it will be
found in replace_effective_prog() ? I don't think that's the case.
Try to create bpf_link with BPF_F_ALLOW_OVERRIDE, override it in a child cgroup
with another link and then try to LINK_UPDATE the former. The link is there,
but the prog is not executing and it's not in effective array. What LINK_UPDATE
suppose to do? I guess it should succeed?

Yes, this is a great catch! I should have used ALLOW_OVERRIDE at the
root cgroup level in my selftest, it would catch it immediately.

BUG_ON(!cg) in replace_effective_prog() is too aggressive, if I
replace it with `if (!cg) continue;` it will handle this as well.

quoted

Even trickier that the prog will be in effective array in some of
css_for_each_descendant_pre() and not in others. This cgroup attach semantics
were convoluted from the day one. Apparently people use all three variants now,
but I wouldn't bet that everyone understands it.

Agree about convoluted logic, spent enormous time understanding and
modifying it :)

But apart from BUG_ON(!cg) problem, everything works because each
level of hierarchy is treated independently in
replace_effective_prog(). Search for attached link on each level is
reset and performed anew. If found - we replace program, if not - must
be ALLOW_OVERRIDE case, i.e., actually overridden link.

quoted

Hence my proposal to support F_ALLOW_MULTI for links only. At least initially.
It's so much simpler to explain. And owning bpf_link will guarantee that the
prog is executing (unless cgroup is removed and sockets are closed). I guess
default (no-override) is acceptable to bpf_link as well and in that sense it
will be very similar to XDP with single prog attached. So I think I can live
with default and ALLOW_MULTI for now. But we should probably redesign
overriding capabilities. Folks need to attach multiple progs to a given cgroup
and disallow all progs in children. Currently it's not possible to do, since
MULTI in the parent allows at least one (default, override or multi) in the
children. bpf_link inheriting this logic won't help to solve this use case. It
feels that link should stay as multi only and override or not in the children
should be a separate property. Probably not related to link at all. It fits
better as a cgroup permission.

Yeah, we had a brief discussion with Andrey on mailing list. Not sure
what the solution looks like, but it should be orthogonal to link/prog
attachment operation, probably.

If you insist and Andrey is ok with dropping ALLOW_OVERRIDE, it's
easy. But fixing the logic to handle it is also easy. So are we sure
about supporting 2 out of 3 existing modes? :)

I wasn't clear enough. My preference is only multi for bpf_link with a concrete

Ah, ok, it certainly read as default + multi should be supported.
Alright, I'll drop NONE and OVERRIDE mode (so back to initial
version).

plan how cgroup permissions can do no-override, selective override, and
whatever else container folks need.
I can imagine somebody may want to attach bind/connect at outer container level
and disallow this specific attach_type for children while allowing other
cgroup-bpf prog types in inner containers. There is no way to do so now and
flags for bpf_link is not the answer either.

quoted

Thanks, will rebase on top of bpf-next master for v3.

please wait with repost until this discussion settles.

Sure, will do.