bpf: Support replacing cgroup-bpf program in MULTI mode

Introduce a new bpf_prog_attach_xattr function that accepts an extendable structure and supports passing a new attribute to BPF_PROG_ATTACH command: replace_prog_fd that is fd of previously attached cgroup-bpf program to replace if recently introduced BPF_F_REPLACE flag is used.

The new function is named to be consistent with other xattr-functions (bpf_prog_test_run_xattr, bpf_create_map_xattr, bpf_load_program_xattr).

NOTE: DECLARE_LIBBPF_OPTS macro is not used here because it's available in libbpf.h, and unavailable in bpf.h. Please let me know if the macro should be shared in a common place and used here instead of declaring struct bpf_prog_attach_attr directly.

Signed-off-by Andrey Ignatov <[email protected]>

---
 tools/lib/bpf/bpf.c      | 22 ++++++++++++++++++----
 tools/lib/bpf/bpf.h      | 10 ++++++++++
 tools/lib/bpf/libbpf.map |  5 +++++
 3 files changed, 33 insertions(+), 4 deletions(-)

tools/lib/bpf/bpf.c +18 -4

index 98596e15390f..5a2830fac227 100644
@@ -466,14 +466,28 @@ int bpf_obj_get(const char *pathname)
 
 int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
                    unsigned int flags)
+{
+       struct bpf_prog_attach_attr attach_attr;
+
+       memset(&attach_attr, 0, sizeof(attach_attr));
+       attach_attr.target_fd   = target_fd;
+       attach_attr.prog_fd     = prog_fd;
+       attach_attr.type        = type;
+       attach_attr.flags       = flags;
+
+       return bpf_prog_attach_xattr(&attach_attr);
+}
+
+int bpf_prog_attach_xattr(const struct bpf_prog_attach_attr *attach_attr)
 {
        union bpf_attr attr;
 
        memset(&attr, 0, sizeof(attr));
-       attr.target_fd     = target_fd;
-       attr.attach_bpf_fd = prog_fd;
-       attr.attach_type   = type;
-       attr.attach_flags  = flags;
+       attr.target_fd     = attach_attr->target_fd;
+       attr.attach_bpf_fd = attach_attr->prog_fd;
+       attr.attach_type   = attach_attr->type;
+       attr.attach_flags  = attach_attr->flags;
+       attr.replace_bpf_fd = attach_attr->replace_prog_fd;
 
        return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr));
 }

tools/lib/bpf/bpf.h +10 -0

index 3c791fa8e68e..4b7269d3bae7 100644
@@ -128,8 +128,18 @@ LIBBPF_API int bpf_map_get_next_key(int fd, const void *key, void *next_key);
 LIBBPF_API int bpf_map_freeze(int fd);
 LIBBPF_API int bpf_obj_pin(int fd, const char *pathname);
 LIBBPF_API int bpf_obj_get(const char *pathname);
+
+struct bpf_prog_attach_attr {
+       int target_fd;
+       int prog_fd;
+       enum bpf_attach_type type;
+       unsigned int flags;
+       int replace_prog_fd;
+};
+
 LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
                               enum bpf_attach_type type, unsigned int flags);
+LIBBPF_API int bpf_prog_attach_xattr(const struct bpf_prog_attach_attr *attr);
 LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
 LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
                                enum bpf_attach_type type);

tools/lib/bpf/libbpf.map +5 -1

index 8ddc2c40e482..42b065454031 100644
@@ -208,3 +208,8 @@ LIBBPF_0.0.6 {
                btf__find_by_name_kind;
                libbpf_find_vmlinux_btf_id;
 } LIBBPF_0.0.5;
+
+LIBBPF_0.0.7 {
+       global:
+               bpf_prog_attach_xattr;
+} LIBBPF_0.0.6;
--

2.17.1

__cgroup_bpf_attach has a lot of identical code to handle two scenarios:
BPF_F_ALLOW_MULTI is set and unset.

Simplify it by splitting the two main steps:

* First, the decision is made whether a new bpf_prog_list entry should be allocated or existing entry should be reused for the new program. This decision is saved in replace_pl pointer;

* Next, replace_pl pointer is used to handle both possible states of BPF_F_ALLOW_MULTI flag (set / unset) instead of doing similar work for them separately.

This splitting, in turn, allows to make further simplifications:

* The check for attaching same program twice in BPF_F_ALLOW_MULTI mode can be done before allocating cgroup storage, so that if user tries to attach same program twice no alloc/free happens as it was before;

* pl_was_allocated becomes redundant so it's removed.

new_flags is unused, remove it.

The common use-case in production is to have multiple cgroup-bpf programs per attach type that cover multiple use-cases. Such programs are attached with BPF_F_ALLOW_MULTI and can be maintained by different people.

Order of programs usually matters, for example imagine two egress
programs: the first one drops packets and the second one counts packets. If they're swapped the result of counting program will be different.

It brings operational challenges with updating cgroup-bpf program(s) attached with BPF_F_ALLOW_MULTI since there is no way to replace a program:

* One way to update is to detach all programs first and then attach the new version(s) again in the right order. This introduces an interruption in the work a program is doing and may not be acceptable (e.g. if it's egress firewall);

* Another way is attach the new version of a program first and only then detach the old version. This introduces the time interval when two versions of same program are working, what may not be acceptable if a program is not idempotent. It also imposes additional burden on program developers to make sure that two versions of their program can co-exist.

Solve the problem by introducing a "replace" mode in BPF_PROG_ATTACH command for cgroup-bpf programs being attached with BPF_F_ALLOW_MULTI flag. This mode is enabled by newly introduced BPF_F_REPLACE attach flag and bpf_attr.replace_bpf_fd attribute to pass fd of the old program to replace

That way user can replace any program among those attached with BPF_F_ALLOW_MULTI flag without the problems described above.

Details of the new API:

* If BPF_F_REPLACE is set but replace_bpf_fd doesn't have valid descriptor of BPF program, BPF_PROG_ATTACH will return corresponding error (EINVAL or EBADF).

* If replace_bpf_fd has valid descriptor of BPF program but such a program is not attached to specified cgroup, BPF_PROG_ATTACH will return ENOENT.

BPF_F_REPLACE is introduced to make the user intend clear, since replace_bpf_fd alone can't be used for this (its default value, 0, is a valid fd). BPF_F_REPLACE also makes it possible to extend the API in the future (e.g. add BPF_F_BEFORE and BPF_F_AFTER if needed).

kernel/bpf/cgroup.c +13 -3

index 283efe3ce052..45346c79613a 100644
@@ -282,14 +282,17 @@ static int update_effective_progs(struct cgroup *cgrp,
  *                         propagate the change to descendants
  * @cgrp: The cgroup which descendants to traverse
  * @prog: A program to attach
+ * @replace_prog: Previously attached program to replace if BPF_F_REPLACE is set
  * @type: Type of attach operation
  * @flags: Option flags
  *
  * Must be called with cgroup_mutex held.
  */
 int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
+			struct bpf_prog *replace_prog,
 			enum bpf_attach_type type, u32 flags)
 {
+	u32 saved_flags = (flags & (BPF_F_ALLOW_OVERRIDE | BPF_F_ALLOW_MULTI));
 	struct list_head *progs = &cgrp->bpf.progs[type];
 	struct bpf_prog *old_prog = NULL;
 	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE],

@@ -298,14 +301,15 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 	enum bpf_cgroup_storage_type stype;
 	int err;
 
-	if ((flags & BPF_F_ALLOW_OVERRIDE) && (flags & BPF_F_ALLOW_MULTI))
+	if (((flags & BPF_F_ALLOW_OVERRIDE) && (flags & BPF_F_ALLOW_MULTI)) ||
+	    ((flags & BPF_F_REPLACE) && !(flags & BPF_F_ALLOW_MULTI)))
 		/* invalid combination */
 		return -EINVAL;
 
 	if (!hierarchy_allows_attach(cgrp, type))
 		return -EPERM;
 
-	if (!list_empty(progs) && cgrp->bpf.flags[type] != flags)
+	if (!list_empty(progs) && cgrp->bpf.flags[type] != saved_flags)
 		/* Disallow attaching non-overridable on top
 		 * of existing overridable in this cgroup.
 		 * Disallow attaching multi-prog if overridable or none

@@ -320,7 +324,12 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 			if (pl->prog == prog)
 				/* disallow attaching the same prog twice */
 				return -EINVAL;
+			if (pl->prog == replace_prog)
+				replace_pl = pl;
 		}
+		if ((flags & BPF_F_REPLACE) && !replace_pl)
+			/* prog to replace not found for cgroup */
+			return -ENOENT;
 	} else if (!list_empty(progs)) {
 		replace_pl = list_first_entry(progs, typeof(*pl), node);
 	}

@@ -356,7 +365,7 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 	for_each_cgroup_storage_type(stype)
 		pl->storage[stype] = storage[stype];
 
-	cgrp->bpf.flags[type] = flags;
+	cgrp->bpf.flags[type] = saved_flags;
 
 	err = update_effective_progs(cgrp, type);
 	if (err)

@@ -522,6 +531,7 @@ int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
 int cgroup_bpf_prog_attach(const union bpf_attr *attr,
 			   enum bpf_prog_type ptype, struct bpf_prog *prog)
 {
+	struct bpf_prog *replace_prog = NULL;
 	struct cgroup *cgrp;
 	int ret;
 

kernel/bpf/cgroup.c +11 -0

+		replace_prog = bpf_prog_get_type(attr->replace_bpf_fd, ptype);
+		if (IS_ERR(replace_prog)) {
+			cgroup_put(cgrp);
+			return PTR_ERR(replace_prog);
+		}
+	}
+
+	ret = cgroup_bpf_attach(cgrp, prog, replace_prog, attr->attach_type,
 				attr->attach_flags);
+
+	if (replace_prog)
+		bpf_prog_put(replace_prog);
 	cgroup_put(cgrp);
 	return ret;
 }

Test replacement of a cgroup-bpf program attached with BPF_F_ALLOW_MULTI and possible failure modes: invalid combination of flags, invalid replace_bpf_fd, replacing a non-attachd to specified cgroup program.

Example of program replacing:

  # gdb -q ./test_cgroup_attach
  Reading symbols from /data/users/rdna/bin/test_cgroup_attach...done.
  ...
  Breakpoint 1, test_multiprog () at test_cgroup_attach.c:442
  442     test_cgroup_attach.c: No such file or directory.
  (gdb)
  [2]+  Stopped                 gdb -q ./test_cgroup_attach
  # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1
  ID       AttachType      AttachFlags     Name
  35       egress          multi
  36       egress          multi
  # fg gdb -q ./test_cgroup_attach
  c
  Continuing.
  Detaching after fork from child process 361.

Breakpoint 2, test_multiprog () at test_cgroup_attach.c:453
453 in test_cgroup_attach.c
(gdb)
[2]+ Stopped gdb -q ./test_cgroup_attach

  # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1
  ID       AttachType      AttachFlags     Name
  41       egress          multi
  36       egress          multi

Signed-off-by Andrey Ignatov <[email protected]>

---
 .../selftests/bpf/test_cgroup_attach.c        | 61 +++++++++++++++++--
 1 file changed, 56 insertions(+), 5 deletions(-)

tools/testing/selftests/bpf/test_cgroup_attach.c +19 -3

index 7671909ee1cb..b9148d752207 100644
@@ -250,7 +250,7 @@ static int prog_load_cnt(int verdict, int val)
 		BPF_LD_MAP_FD(BPF_REG_1, map_fd),
 		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
 		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
-		BPF_MOV64_IMM(BPF_REG_1, val), /* r1 = 1 */
+		BPF_MOV64_IMM(BPF_REG_1, val), /* r1 = val */
 		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
 
 		BPF_LD_MAP_FD(BPF_REG_1, cgroup_storage_fd),

@@ -290,11 +290,12 @@ static int test_multiprog(void)
 {
 	__u32 prog_ids[4], prog_cnt = 0, attach_flags, saved_prog_id;
 	int cg1 = 0, cg2 = 0, cg3 = 0, cg4 = 0, cg5 = 0, key = 0;
-	int drop_prog, allow_prog[6] = {}, rc = 0;
+	int drop_prog, allow_prog[7] = {}, rc = 0;
+	struct bpf_prog_attach_attr attach_attr;
 	unsigned long long value;
 	int i = 0;
 
-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < ARRAY_SIZE(allow_prog); i++) {
 		allow_prog[i] = prog_load_cnt(1, 1 << i);
 		if (!allow_prog[i])
 			goto err;

@@ -400,6 +401,56 @@ static int test_multiprog(void)
 	assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0);
 	assert(value == 1 + 2 + 8 + 16);
 
+	/* invalid input */
+
+	memset(&attach_attr, 0, sizeof(attach_attr));
+	attach_attr.target_fd		= cg1;
+	attach_attr.prog_fd		= allow_prog[6];
+	attach_attr.replace_prog_fd	= allow_prog[0];
+	attach_attr.type		= BPF_CGROUP_INET_EGRESS;
+	attach_attr.flags		= BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE;
+
+	if (!bpf_prog_attach_xattr(&attach_attr)) {
+		log_err("Unexpected success with OVERRIDE | REPLACE");
+		goto err;
+	}
+	assert(errno == EINVAL);
+

tools/testing/selftests/bpf/test_cgroup_attach.c +3 -0

+		log_err("Unexpected success with bad replace fd");
+		goto err;
+	}

Quoted 40 lines

The common use-case in production is to have multiple cgroup-bpf programs per attach type that cover multiple use-cases. Such programs are attached with BPF_F_ALLOW_MULTI and can be maintained by different people.

Order of programs usually matters, for example imagine two egress
programs: the first one drops packets and the second one counts packets. If they're swapped the result of counting program will be different.

It brings operational challenges with updating cgroup-bpf program(s) attached with BPF_F_ALLOW_MULTI since there is no way to replace a program:

* One way to update is to detach all programs first and then attach the new version(s) again in the right order. This introduces an interruption in the work a program is doing and may not be acceptable (e.g. if it's egress firewall);

* Another way is attach the new version of a program first and only then detach the old version. This introduces the time interval when two versions of same program are working, what may not be acceptable if a program is not idempotent. It also imposes additional burden on program developers to make sure that two versions of their program can co-exist.

Solve the problem by introducing a "replace" mode in BPF_PROG_ATTACH command for cgroup-bpf programs being attached with BPF_F_ALLOW_MULTI flag. This mode is enabled by newly introduced BPF_F_REPLACE attach flag and bpf_attr.replace_bpf_fd attribute to pass fd of the old program to replace

That way user can replace any program among those attached with BPF_F_ALLOW_MULTI flag without the problems described above.

Details of the new API:

* If BPF_F_REPLACE is set but replace_bpf_fd doesn't have valid descriptor of BPF program, BPF_PROG_ATTACH will return corresponding error (EINVAL or EBADF).

Quoted 8 lines

* If replace_bpf_fd has valid descriptor of BPF program but such a program is not attached to specified cgroup, BPF_PROG_ATTACH will return ENOENT.

BPF_F_REPLACE is introduced to make the user intend clear, since replace_bpf_fd alone can't be used for this (its default value, 0, is a valid fd). BPF_F_REPLACE also makes it possible to extend the API in the future (e.g. add BPF_F_BEFORE and BPF_F_AFTER if needed).

Thanks for the details explanation.

[ ... ]

Quoted 71 lines

kernel/bpf/cgroup.c +13 -3

index 283efe3ce052..45346c79613a 100644
@@ -282,14 +282,17 @@ static int update_effective_progs(struct cgroup *cgrp,
  *                         propagate the change to descendants
  * @cgrp: The cgroup which descendants to traverse
  * @prog: A program to attach
+ * @replace_prog: Previously attached program to replace if BPF_F_REPLACE is set
  * @type: Type of attach operation
  * @flags: Option flags
  *
  * Must be called with cgroup_mutex held.
  */
 int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
+			struct bpf_prog *replace_prog,
 			enum bpf_attach_type type, u32 flags)
 {
+	u32 saved_flags = (flags & (BPF_F_ALLOW_OVERRIDE | BPF_F_ALLOW_MULTI));
 	struct list_head *progs = &cgrp->bpf.progs[type];
 	struct bpf_prog *old_prog = NULL;
 	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE],

@@ -298,14 +301,15 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 	enum bpf_cgroup_storage_type stype;
 	int err;
 
-	if ((flags & BPF_F_ALLOW_OVERRIDE) && (flags & BPF_F_ALLOW_MULTI))
+	if (((flags & BPF_F_ALLOW_OVERRIDE) && (flags & BPF_F_ALLOW_MULTI)) ||
+	    ((flags & BPF_F_REPLACE) && !(flags & BPF_F_ALLOW_MULTI)))
 		/* invalid combination */
 		return -EINVAL;
 
 	if (!hierarchy_allows_attach(cgrp, type))
 		return -EPERM;
 
-	if (!list_empty(progs) && cgrp->bpf.flags[type] != flags)
+	if (!list_empty(progs) && cgrp->bpf.flags[type] != saved_flags)
 		/* Disallow attaching non-overridable on top
 		 * of existing overridable in this cgroup.
 		 * Disallow attaching multi-prog if overridable or none

@@ -320,7 +324,12 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 			if (pl->prog == prog)
 				/* disallow attaching the same prog twice */
 				return -EINVAL;
+			if (pl->prog == replace_prog)
+				replace_pl = pl;
 		}
+		if ((flags & BPF_F_REPLACE) && !replace_pl)
+			/* prog to replace not found for cgroup */
+			return -ENOENT;
 	} else if (!list_empty(progs)) {
 		replace_pl = list_first_entry(progs, typeof(*pl), node);
 	}

@@ -356,7 +365,7 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 	for_each_cgroup_storage_type(stype)
 		pl->storage[stype] = storage[stype];
 
-	cgrp->bpf.flags[type] = flags;
+	cgrp->bpf.flags[type] = saved_flags;
 
 	err = update_effective_progs(cgrp, type);
 	if (err)

@@ -522,6 +531,7 @@ int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
 int cgroup_bpf_prog_attach(const union bpf_attr *attr,
 			   enum bpf_prog_type ptype, struct bpf_prog *prog)
 {
+	struct bpf_prog *replace_prog = NULL;
 	struct cgroup *cgrp;
 	int ret;
 

kernel/bpf/cgroup.c int cgroup_bpf_prog_attach(const union bpf_attr *attr, · line 543

Quoted 6 lines

kernel/bpf/cgroup.c +2 -1

@@ -529,8 +539,20 @@ int cgroup_bpf_prog_attach(const union bpf_attr *attr,
 	if (IS_ERR(cgrp))
 		return PTR_ERR(cgrp);
 
-	ret = cgroup_bpf_attach(cgrp, prog, attr->attach_type,
+	if ((attr->attach_flags & BPF_F_ALLOW_MULTI) &&
+	    (attr->attach_flags & BPF_F_REPLACE)) {

The patch looks good. One optional nit for consideration,

Since it is testing BPF_F_REPLACE here already,
how about moving the
"((flags & BPF_F_REPLACE) && !(flags & BPF_F_ALLOW_MULTI))" test from __cgroup_bpf_attach() to this function here? Clear the BPF_F_REPLACE bit before passing to cgroup_bpf_attach().

Then the "saved_flags" logic in cgroup_bpf_attach() can go away. cgroup_bpf_attach() can work on the "replace_prog" alone.

Acked-by Martin KaFai Lau <[email protected]>

Quoted 15 lines

kernel/bpf/cgroup.c +11 -0

+		replace_prog = bpf_prog_get_type(attr->replace_bpf_fd, ptype);
+		if (IS_ERR(replace_prog)) {
+			cgroup_put(cgrp);
+			return PTR_ERR(replace_prog);
+		}
+	}
+
+	ret = cgroup_bpf_attach(cgrp, prog, replace_prog, attr->attach_type,
 				attr->attach_flags);
+
+	if (replace_prog)
+		bpf_prog_put(replace_prog);
 	cgroup_put(cgrp);
 	return ret;
 }

Review

Quoted 2 lines

(quoted diff) +2 -0

+	attach_attr.replace_prog_fd = -1;
+	if (!bpf_prog_attach_xattr(&attach_attr)) {

The whole set LGTM. I expect this attach bit will change based on the discussion in patch 4.