[PATCH bpf-next 0/5] bpf: Support replacing cgroup-bpf program in MULTI mode

[PATCH bpf-next 0/5] bpf: Support replacing cgroup-bpf program in MULTI mode

[Andrey Ignatov]

This patch set adds support for replacing cgroup-bpf programs attached with
BPF_F_ALLOW_MULTI flag so that any program a list can be updated to a new
version without service interruption and order of programs can be preserved.

Please see patch 3 for details on the use-case and API changes.

Other patches:
Patch 1 is preliminary refactoring of __cgroup_bpf_attach to simplify it.
Patch 2 is minor cleanup of hierarchy_allows_attach.
Patch 4 extends libbpf API to support new set of attach attributes.
Patch 5 adds selftest coverage for the new API.


Andrey Ignatov (5):
  bpf: Simplify __cgroup_bpf_attach
  bpf: Remove unused new_flags in hierarchy_allows_attach()
  bpf: Support replacing cgroup-bpf program in MULTI mode
  libbpf: Introduce bpf_prog_attach_xattr
  selftests/bpf: Cover BPF_F_REPLACE in test_cgroup_attach

 include/linux/bpf-cgroup.h                    |  4 +-
 include/uapi/linux/bpf.h                      | 10 ++
 kernel/bpf/cgroup.c                           | 97 ++++++++++---------
 kernel/bpf/syscall.c                          |  4 +-
 kernel/cgroup/cgroup.c                        |  5 +-
 tools/include/uapi/linux/bpf.h                | 10 ++
 tools/lib/bpf/bpf.c                           | 22 ++++-
 tools/lib/bpf/bpf.h                           | 10 ++
 tools/lib/bpf/libbpf.map                      |  5 +
 .../selftests/bpf/test_cgroup_attach.c        | 61 +++++++++++-
 10 files changed, 168 insertions(+), 60 deletions(-)

-- 
2.17.1

[PATCH bpf-next 1/5] bpf: Simplify __cgroup_bpf_attach

[Andrey Ignatov]

__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.

Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
 kernel/bpf/cgroup.c | 62 +++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 39 deletions(-)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 9f90d3c92bda..e8cbdd1be687 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -295,9 +295,8 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 	struct bpf_prog *old_prog = NULL;
 	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE],
 		*old_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {NULL};
+	struct bpf_prog_list *pl, *replace_pl = NULL;
 	enum bpf_cgroup_storage_type stype;
-	struct bpf_prog_list *pl;
-	bool pl_was_allocated;
 	int err;
 
 	if ((flags & BPF_F_ALLOW_OVERRIDE) && (flags & BPF_F_ALLOW_MULTI))
@@ -317,6 +316,16 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 	if (prog_list_length(progs) >= BPF_CGROUP_MAX_PROGS)
 		return -E2BIG;
 
+	if (flags & BPF_F_ALLOW_MULTI) {
+		list_for_each_entry(pl, progs, node) {
+			if (pl->prog == prog)
+				/* disallow attaching the same prog twice */
+				return -EINVAL;
+		}
+	} else if (!list_empty(progs)) {
+		replace_pl = list_first_entry(progs, typeof(*pl), node);
+	}
+
 	for_each_cgroup_storage_type(stype) {
 		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
 		if (IS_ERR(storage[stype])) {
@@ -327,52 +336,27 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 		}
 	}
 
-	if (flags & BPF_F_ALLOW_MULTI) {
-		list_for_each_entry(pl, progs, node) {
-			if (pl->prog == prog) {
-				/* disallow attaching the same prog twice */
-				for_each_cgroup_storage_type(stype)
-					bpf_cgroup_storage_free(storage[stype]);
-				return -EINVAL;
-			}
+	if (replace_pl) {
+		pl = replace_pl;
+		old_prog = pl->prog;
+		for_each_cgroup_storage_type(stype) {
+			old_storage[stype] = pl->storage[stype];
+			bpf_cgroup_storage_unlink(old_storage[stype]);
 		}
-
+	} else {
 		pl = kmalloc(sizeof(*pl), GFP_KERNEL);
 		if (!pl) {
 			for_each_cgroup_storage_type(stype)
 				bpf_cgroup_storage_free(storage[stype]);
 			return -ENOMEM;
 		}
-
-		pl_was_allocated = true;
-		pl->prog = prog;
-		for_each_cgroup_storage_type(stype)
-			pl->storage[stype] = storage[stype];
 		list_add_tail(&pl->node, progs);
-	} else {
-		if (list_empty(progs)) {
-			pl = kmalloc(sizeof(*pl), GFP_KERNEL);
-			if (!pl) {
-				for_each_cgroup_storage_type(stype)
-					bpf_cgroup_storage_free(storage[stype]);
-				return -ENOMEM;
-			}
-			pl_was_allocated = true;
-			list_add_tail(&pl->node, progs);
-		} else {
-			pl = list_first_entry(progs, typeof(*pl), node);
-			old_prog = pl->prog;
-			for_each_cgroup_storage_type(stype) {
-				old_storage[stype] = pl->storage[stype];
-				bpf_cgroup_storage_unlink(old_storage[stype]);
-			}
-			pl_was_allocated = false;
-		}
-		pl->prog = prog;
-		for_each_cgroup_storage_type(stype)
-			pl->storage[stype] = storage[stype];
 	}
 
+	pl->prog = prog;
+	for_each_cgroup_storage_type(stype)
+		pl->storage[stype] = storage[stype];
+
 	cgrp->bpf.flags[type] = flags;
 
 	err = update_effective_progs(cgrp, type);
@@ -401,7 +385,7 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 		pl->storage[stype] = old_storage[stype];
 		bpf_cgroup_storage_link(old_storage[stype], cgrp, type);
 	}
-	if (pl_was_allocated) {
+	if (!replace_pl) {
 		list_del(&pl->node);
 		kfree(pl);
 	}
-- 
2.17.1

[PATCH bpf-next 2/5] bpf: Remove unused new_flags in hierarchy_allows_attach()

[Andrey Ignatov]

new_flags is unused, remove it.

Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
 kernel/bpf/cgroup.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index e8cbdd1be687..283efe3ce052 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -103,8 +103,7 @@ static u32 prog_list_length(struct list_head *head)
  * if parent has overridable or multi-prog, allow attaching
  */
 static bool hierarchy_allows_attach(struct cgroup *cgrp,
-				    enum bpf_attach_type type,
-				    u32 new_flags)
+				    enum bpf_attach_type type)
 {
 	struct cgroup *p;
 
@@ -303,7 +302,7 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
 		/* invalid combination */
 		return -EINVAL;
 
-	if (!hierarchy_allows_attach(cgrp, type, flags))
+	if (!hierarchy_allows_attach(cgrp, type))
 		return -EPERM;
 
 	if (!list_empty(progs) && cgrp->bpf.flags[type] != flags)
-- 
2.17.1

[PATCH bpf-next 3/5] bpf: Support replacing cgroup-bpf program in MULTI mode

[Andrey Ignatov]

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).

Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
 include/linux/bpf-cgroup.h     |  4 +++-
 include/uapi/linux/bpf.h       | 10 ++++++++++
 kernel/bpf/cgroup.c            | 30 ++++++++++++++++++++++++++----
 kernel/bpf/syscall.c           |  4 ++--
 kernel/cgroup/cgroup.c         |  5 +++--
 tools/include/uapi/linux/bpf.h | 10 ++++++++++
 6 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index 169fd25f6bc2..18f6a6da7c3c 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -85,6 +85,7 @@ int cgroup_bpf_inherit(struct cgroup *cgrp);
 void cgroup_bpf_offline(struct cgroup *cgrp);
 
 int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
+			struct bpf_prog *replace_prog,
 			enum bpf_attach_type type, u32 flags);
 int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
 			enum bpf_attach_type type);
@@ -93,7 +94,8 @@ int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
 
 /* Wrapper for __cgroup_bpf_*() protected by cgroup_mutex */
 int cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
-		      enum bpf_attach_type type, u32 flags);
+		      struct bpf_prog *replace_prog, enum bpf_attach_type type,
+		      u32 flags);
 int cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
 		      enum bpf_attach_type type, u32 flags);
 int cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index dbbcf0b02970..7df436da542d 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -231,6 +231,11 @@ enum bpf_attach_type {
  * When children program makes decision (like picking TCP CA or sock bind)
  * parent program has a chance to override it.
  *
+ * With BPF_F_ALLOW_MULTI a new program is added to the end of the list of
+ * programs for a cgroup. Though it's possible to replace an old program at
+ * any position by also specifying BPF_F_REPLACE flag and position itself in
+ * replace_bpf_fd attribute. Old program at this position will be released.
+ *
  * A cgroup with MULTI or OVERRIDE flag allows any attach flags in sub-cgroups.
  * A cgroup with NONE doesn't allow any programs in sub-cgroups.
  * Ex1:
@@ -249,6 +254,7 @@ enum bpf_attach_type {
  */
 #define BPF_F_ALLOW_OVERRIDE	(1U << 0)
 #define BPF_F_ALLOW_MULTI	(1U << 1)
+#define BPF_F_REPLACE		(1U << 2)
 
 /* If BPF_F_STRICT_ALIGNMENT is used in BPF_PROG_LOAD command, the
  * verifier will perform strict alignment checking as if the kernel
@@ -442,6 +448,10 @@ union bpf_attr {
 		__u32		attach_bpf_fd;	/* eBPF program to attach */
 		__u32		attach_type;
 		__u32		attach_flags;
+		__u32		replace_bpf_fd;	/* previously attached eBPF
+						 * program to replace if
+						 * BPF_F_REPLACE is used
+						 */
 	};
 
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 283efe3ce052..45346c79613a 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -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;
 
@@ -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)) {
+		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;
 }
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index e3461ec59570..1e4abb618c5a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2040,10 +2040,10 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
 	}
 }
 
-#define BPF_PROG_ATTACH_LAST_FIELD attach_flags
+#define BPF_PROG_ATTACH_LAST_FIELD replace_bpf_fd
 
 #define BPF_F_ATTACH_MASK \
-	(BPF_F_ALLOW_OVERRIDE | BPF_F_ALLOW_MULTI)
+	(BPF_F_ALLOW_OVERRIDE | BPF_F_ALLOW_MULTI | BPF_F_REPLACE)
 
 static int bpf_prog_attach(const union bpf_attr *attr)
 {
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 735af8f15f95..725365df066d 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6288,12 +6288,13 @@ void cgroup_sk_free(struct sock_cgroup_data *skcd)
 
 #ifdef CONFIG_CGROUP_BPF
 int cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
-		      enum bpf_attach_type type, u32 flags)
+		      struct bpf_prog *replace_prog, enum bpf_attach_type type,
+		      u32 flags)
 {
 	int ret;
 
 	mutex_lock(&cgroup_mutex);
-	ret = __cgroup_bpf_attach(cgrp, prog, type, flags);
+	ret = __cgroup_bpf_attach(cgrp, prog, replace_prog, type, flags);
 	mutex_unlock(&cgroup_mutex);
 	return ret;
 }
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index dbbcf0b02970..7df436da542d 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -231,6 +231,11 @@ enum bpf_attach_type {
  * When children program makes decision (like picking TCP CA or sock bind)
  * parent program has a chance to override it.
  *
+ * With BPF_F_ALLOW_MULTI a new program is added to the end of the list of
+ * programs for a cgroup. Though it's possible to replace an old program at
+ * any position by also specifying BPF_F_REPLACE flag and position itself in
+ * replace_bpf_fd attribute. Old program at this position will be released.
+ *
  * A cgroup with MULTI or OVERRIDE flag allows any attach flags in sub-cgroups.
  * A cgroup with NONE doesn't allow any programs in sub-cgroups.
  * Ex1:
@@ -249,6 +254,7 @@ enum bpf_attach_type {
  */
 #define BPF_F_ALLOW_OVERRIDE	(1U << 0)
 #define BPF_F_ALLOW_MULTI	(1U << 1)
+#define BPF_F_REPLACE		(1U << 2)
 
 /* If BPF_F_STRICT_ALIGNMENT is used in BPF_PROG_LOAD command, the
  * verifier will perform strict alignment checking as if the kernel
@@ -442,6 +448,10 @@ union bpf_attr {
 		__u32		attach_bpf_fd;	/* eBPF program to attach */
 		__u32		attach_type;
 		__u32		attach_flags;
+		__u32		replace_bpf_fd;	/* previously attached eBPF
+						 * program to replace if
+						 * BPF_F_REPLACE is used
+						 */
 	};
 
 	struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
-- 
2.17.1

[PATCH bpf-next 4/5] libbpf: Introduce bpf_prog_attach_xattr

[Andrey Ignatov]

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 <rdna@fb.com>
---
 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(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 98596e15390f..5a2830fac227 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -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));
 }
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 3c791fa8e68e..4b7269d3bae7 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -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);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 8ddc2c40e482..42b065454031 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -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

[PATCH bpf-next 5/5] selftests/bpf: Cover BPF_F_REPLACE in test_cgroup_attach

[Andrey Ignatov]

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 <rdna@fb.com>
---
 .../selftests/bpf/test_cgroup_attach.c        | 61 +++++++++++++++++--
 1 file changed, 56 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_cgroup_attach.c b/tools/testing/selftests/bpf/test_cgroup_attach.c
index 7671909ee1cb..b9148d752207 100644
--- a/tools/testing/selftests/bpf/test_cgroup_attach.c
+++ b/tools/testing/selftests/bpf/test_cgroup_attach.c
@@ -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);
+
+	attach_attr.flags = BPF_F_REPLACE;
+	if (!bpf_prog_attach_xattr(&attach_attr)) {
+		log_err("Unexpected success with REPLACE alone");
+		goto err;
+	}
+	assert(errno == EINVAL);
+	attach_attr.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE;
+
+	attach_attr.replace_prog_fd = -1;
+	if (!bpf_prog_attach_xattr(&attach_attr)) {
+		log_err("Unexpected success with bad replace fd");
+		goto err;
+	}
+	assert(errno == EBADF);
+
+	/* replacing a program that is not attached to cgroup should fail  */
+	attach_attr.replace_prog_fd = allow_prog[3];
+	if (!bpf_prog_attach_xattr(&attach_attr)) {
+		log_err("Unexpected success: replace not-attached prog on cg1");
+		goto err;
+	}
+	assert(errno == ENOENT);
+	attach_attr.replace_prog_fd = allow_prog[0];
+
+	/* replace 1st from the top program */
+	if (bpf_prog_attach_xattr(&attach_attr)) {
+		log_err("Replace prog1 with prog7 on cg1");
+		goto err;
+	}
+	value = 0;
+	assert(bpf_map_update_elem(map_fd, &key, &value, 0) == 0);
+	assert(system(PING_CMD) == 0);
+	assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0);
+	assert(value == 64 + 2 + 8 + 16);
+
 	/* detach 3rd from bottom program and ping again */
 	errno = 0;
 	if (!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS)) {
@@ -414,7 +465,7 @@ static int test_multiprog(void)
 	assert(bpf_map_update_elem(map_fd, &key, &value, 0) == 0);
 	assert(system(PING_CMD) == 0);
 	assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0);
-	assert(value == 1 + 2 + 16);
+	assert(value == 64 + 2 + 16);
 
 	/* detach 2nd from bottom program and ping again */
 	if (bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS)) {
@@ -425,7 +476,7 @@ static int test_multiprog(void)
 	assert(bpf_map_update_elem(map_fd, &key, &value, 0) == 0);
 	assert(system(PING_CMD) == 0);
 	assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0);
-	assert(value == 1 + 2 + 4);
+	assert(value == 64 + 2 + 4);
 
 	prog_cnt = 4;
 	assert(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, BPF_F_QUERY_EFFECTIVE,
-- 
2.17.1

Re: [PATCH bpf-next 4/5] libbpf: Introduce bpf_prog_attach_xattr

[Andrii Nakryiko]

On Tue, Dec 10, 2019 at 6:35 PM Andrey Ignatov <rdna@fb.com> wrote:
>
> 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.
>

I think doing opts is a better way to go forward. With current
approach, next time we need another extra field, we'd need to add yet
another function and/or symbol version existing one. BTW, with
xxx_opts approach, we tried to keep mandatory arguments that are going
to be always specified as first few arguments of a function, and other
stuff that's optional (e.g., flags or replace_prog_fd seem to be good
candidates), would go under opts. This differs from xattr way, which
is why I'm pointing this out.

> Signed-off-by: Andrey Ignatov <rdna@fb.com>
> ---
>  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(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 98596e15390f..5a2830fac227 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -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));
>  }
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 3c791fa8e68e..4b7269d3bae7 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -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);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 8ddc2c40e482..42b065454031 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -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
>

Re: [PATCH bpf-next 1/5] bpf: Simplify __cgroup_bpf_attach

[Martin Lau]

On Tue, Dec 10, 2019 at 06:33:27PM -0800, Andrey Ignatov wrote:
> __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.
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [Potential Spoof] [PATCH bpf-next 2/5] bpf: Remove unused new_flags in hierarchy_allows_attach()

[Martin Lau]

On Tue, Dec 10, 2019 at 06:33:28PM -0800, Andrey Ignatov wrote:
> new_flags is unused, remove it.
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [Potential Spoof] [PATCH bpf-next 3/5] bpf: Support replacing cgroup-bpf program in MULTI mode

[Martin Lau]

On Tue, Dec 10, 2019 at 06:33:29PM -0800, Andrey Ignatov wrote:
> 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).
Thanks for the details explanation.

[ ... ]

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 283efe3ce052..45346c79613a 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -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;
>  
> @@ -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 <kafai@fb.com>

> +		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;
>  }

Re: [PATCH bpf-next 5/5] selftests/bpf: Cover BPF_F_REPLACE in test_cgroup_attach

[Martin Lau]

On Tue, Dec 10, 2019 at 06:33:31PM -0800, Andrey Ignatov wrote:
> 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 <rdna@fb.com>
> ---
>  .../selftests/bpf/test_cgroup_attach.c        | 61 +++++++++++++++++--
>  1 file changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_cgroup_attach.c b/tools/testing/selftests/bpf/test_cgroup_attach.c
> index 7671909ee1cb..b9148d752207 100644
> --- a/tools/testing/selftests/bpf/test_cgroup_attach.c
> +++ b/tools/testing/selftests/bpf/test_cgroup_attach.c
> @@ -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);
> +
> +	attach_attr.flags = BPF_F_REPLACE;
> +	if (!bpf_prog_attach_xattr(&attach_attr)) {
> +		log_err("Unexpected success with REPLACE alone");
> +		goto err;
> +	}
> +	assert(errno == EINVAL);
> +	attach_attr.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE;
> +
> +	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.


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

Re: [PATCH bpf-next 3/5] bpf: Support replacing cgroup-bpf program in MULTI mode

[Andrey Ignatov]

Martin Lau <kafai@fb.com> [Thu, 2019-12-12 10:18 -0800]:
> On Tue, Dec 10, 2019 at 06:33:29PM -0800, Andrey Ignatov wrote:
> > 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).
> Thanks for the details explanation.
> 
> [ ... ]
> 
> > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > index 283efe3ce052..45346c79613a 100644
> > --- a/kernel/bpf/cgroup.c
> > +++ b/kernel/bpf/cgroup.c
> > @@ -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;
> >  
> > @@ -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 <kafai@fb.com>

Thank you for review Martin!

I considered doing exactly this and a few other options to split the
logic between __cgroup_bpf_attach() and cgroup_bpf_prog_attach() since
it's not super clear what belongs where, but decided to go with the
current approach.

A couple of reasons I split it this way:

1) 
To keep the whole logic and decisions in __cgroup_bpf_attach() and use
cgroup_bpf_prog_attach() only to acquire cgroup-bpf specific resources
that correspond to the user input, such as cgroup and program to
replace. Unfortunately to acquire replace_prog I still need to check
flags to avoid unnecessary work for the most common case when
BPF_F_REPLACE is not set, but IMO it's better to keep the logic to
verify flags combinations in one place, __cgroup_bpf_attach().

2)
Also I think saved_flags would be introduced sooner or later anyway if
new flags are added since as it can be seen there is a clear separation
between flags that control programs arrangement, like OVERRIDE and
MULTI, and should be remembered for the whole life time of the program,
and one-time-needed flags such as REPLACE that are needed only once to
attach program and don't make sense in its future life time.


> > +		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;
> >  }

-- 
Andrey Ignatov

Re: [PATCH bpf-next 5/5] selftests/bpf: Cover BPF_F_REPLACE in test_cgroup_attach

[Andrey Ignatov]

Martin Lau <kafai@fb.com> [Thu, 2019-12-12 10:26 -0800]:
> On Tue, Dec 10, 2019 at 06:33:31PM -0800, Andrey Ignatov wrote:
...
> > +	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.

Right, I'll change libbpf part and this test according to the feedback
from Andrii and send v2.

Thank you! 

-- 
Andrey Ignatov