* [PATCH v3 bpf-next 0/6] bpf: Support replacing cgroup-bpf program in MULTI mode
@ 2019-12-19 1:55 Andrey Ignatov
2019-12-19 1:55 ` [PATCH v3 bpf-next 1/6] bpf: Simplify __cgroup_bpf_attach Andrey Ignatov
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: Andrey Ignatov @ 2019-12-19 1:55 UTC (permalink / raw)
To: bpf; +Cc: Andrey Ignatov, ast, daniel, kafai, andriin, kernel-team
v2->v3:
- rely on DECLARE_LIBBPF_OPTS from libbpf_common.h;
- separate "required" and "optional" arguments in bpf_prog_attach_xattr;
- convert test_cgroup_attach to prog_tests;
- move the new selftest to prog_tests/cgroup_attach_multi.
v1->v2:
- move DECLARE_LIBBPF_OPTS from libbpf.h to bpf.h (patch 4);
- switch new libbpf API to OPTS framework;
- switch selftest to libbpf OPTS framework.
This patch set adds support for replacing cgroup-bpf programs attached with
BPF_F_ALLOW_MULTI flag so that any program in 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 converts test_cgroup_attach to prog_tests.
Patch 6 adds selftest coverage for the new API.
Andrey Ignatov (6):
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: Convert test_cgroup_attach to prog_tests
selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi
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 | 16 +-
tools/lib/bpf/bpf.h | 11 +
tools/lib/bpf/libbpf.map | 1 +
tools/testing/selftests/bpf/.gitignore | 1 -
tools/testing/selftests/bpf/Makefile | 3 +-
.../bpf/prog_tests/cgroup_attach_autodetach.c | 111 ++++
.../bpf/prog_tests/cgroup_attach_multi.c | 285 +++++++++
.../bpf/prog_tests/cgroup_attach_override.c | 148 +++++
.../selftests/bpf/test_cgroup_attach.c | 571 ------------------
15 files changed, 651 insertions(+), 626 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c
create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c
delete mode 100644 tools/testing/selftests/bpf/test_cgroup_attach.c
--
2.17.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v3 bpf-next 1/6] bpf: Simplify __cgroup_bpf_attach 2019-12-19 1:55 [PATCH v3 bpf-next 0/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov @ 2019-12-19 1:55 ` Andrey Ignatov 2019-12-19 1:55 ` [PATCH v3 bpf-next 2/6] bpf: Remove unused new_flags in hierarchy_allows_attach() Andrey Ignatov ` (4 subsequent siblings) 5 siblings, 0 replies; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 1:55 UTC (permalink / raw) To: bpf; +Cc: Andrey Ignatov, ast, daniel, kafai, andriin, kernel-team __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> Acked-by: Martin KaFai Lau <kafai@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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 bpf-next 2/6] bpf: Remove unused new_flags in hierarchy_allows_attach() 2019-12-19 1:55 [PATCH v3 bpf-next 0/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov 2019-12-19 1:55 ` [PATCH v3 bpf-next 1/6] bpf: Simplify __cgroup_bpf_attach Andrey Ignatov @ 2019-12-19 1:55 ` Andrey Ignatov 2019-12-19 1:56 ` [PATCH v3 bpf-next 3/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov ` (3 subsequent siblings) 5 siblings, 0 replies; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 1:55 UTC (permalink / raw) To: bpf; +Cc: Andrey Ignatov, ast, daniel, kafai, andriin, kernel-team new_flags is unused, remove it. Signed-off-by: Andrey Ignatov <rdna@fb.com> Acked-by: Martin KaFai Lau <kafai@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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 bpf-next 3/6] bpf: Support replacing cgroup-bpf program in MULTI mode 2019-12-19 1:55 [PATCH v3 bpf-next 0/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov 2019-12-19 1:55 ` [PATCH v3 bpf-next 1/6] bpf: Simplify __cgroup_bpf_attach Andrey Ignatov 2019-12-19 1:55 ` [PATCH v3 bpf-next 2/6] bpf: Remove unused new_flags in hierarchy_allows_attach() Andrey Ignatov @ 2019-12-19 1:56 ` Andrey Ignatov 2019-12-19 1:56 ` [PATCH v3 bpf-next 4/6] libbpf: Introduce bpf_prog_attach_xattr Andrey Ignatov ` (2 subsequent siblings) 5 siblings, 0 replies; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 1:56 UTC (permalink / raw) To: bpf; +Cc: Andrey Ignatov, ast, daniel, kafai, andriin, kernel-team 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 intent 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> Acked-by: Martin KaFai Lau <kafai@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 b08c362f4e02..81ee8595dfee 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2073,10 +2073,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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 bpf-next 4/6] libbpf: Introduce bpf_prog_attach_xattr 2019-12-19 1:55 [PATCH v3 bpf-next 0/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov ` (2 preceding siblings ...) 2019-12-19 1:56 ` [PATCH v3 bpf-next 3/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov @ 2019-12-19 1:56 ` Andrey Ignatov 2019-12-19 5:49 ` Andrii Nakryiko 2019-12-19 1:56 ` [PATCH v3 bpf-next 5/6] selftests/bpf: Convert test_cgroup_attach to prog_tests Andrey Ignatov 2019-12-19 1:56 ` [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi Andrey Ignatov 5 siblings, 1 reply; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 1:56 UTC (permalink / raw) To: bpf; +Cc: Andrey Ignatov, ast, daniel, kafai, andriin, kernel-team Introduce a new bpf_prog_attach_xattr function that, in addition to program fd, target fd and attach type, accepts an extendable struct bpf_prog_attach_opts. bpf_prog_attach_opts relies on DECLARE_LIBBPF_OPTS macro to maintain backward and forward compatibility and has the following "optional" attach attributes: * existing attach_flags, since it's not required when attaching in NONE mode. Even though it's quite often used in MULTI and OVERRIDE mode it seems to be a good idea to reduce number of arguments to bpf_prog_attach_xattr; * newly introduced attribute of BPF_PROG_ATTACH command: replace_prog_fd that is fd of previously attached cgroup-bpf program to replace if 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). The struct bpf_prog_attach_opts is supposed to be used with DECLARE_LIBBPF_OPTS macro. Signed-off-by: Andrey Ignatov <rdna@fb.com> --- tools/lib/bpf/bpf.c | 16 +++++++++++++++- tools/lib/bpf/bpf.h | 11 +++++++++++ tools/lib/bpf/libbpf.map | 1 + 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index 98596e15390f..ebb4f8d71bdb 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -466,6 +466,17 @@ 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) +{ + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, opts, + .flags = flags, + ); + + return bpf_prog_attach_xattr(prog_fd, target_fd, type, &opts); +} + +int bpf_prog_attach_xattr(int prog_fd, int target_fd, + enum bpf_attach_type type, + const struct bpf_prog_attach_opts *opts) { union bpf_attr attr; @@ -473,7 +484,10 @@ int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type, attr.target_fd = target_fd; attr.attach_bpf_fd = prog_fd; attr.attach_type = type; - attr.attach_flags = flags; + if (opts) { + attr.attach_flags = opts->flags; + attr.replace_bpf_fd = opts->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 269807ce9ef5..f0ab8519986e 100644 --- a/tools/lib/bpf/bpf.h +++ b/tools/lib/bpf/bpf.h @@ -126,8 +126,19 @@ 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_opts { + size_t sz; /* size of this struct for forward/backward compatibility */ + unsigned int flags; + int replace_prog_fd; +}; +#define bpf_prog_attach_opts__last_field 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(int prog_fd, int attachable_fd, + enum bpf_attach_type type, + const struct bpf_prog_attach_opts *opts); 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 e3a471f38a71..e9713a574243 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -219,6 +219,7 @@ LIBBPF_0.0.7 { bpf_object__detach_skeleton; bpf_object__load_skeleton; bpf_object__open_skeleton; + bpf_prog_attach_xattr; bpf_program__attach; bpf_program__name; btf__align_of; -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 4/6] libbpf: Introduce bpf_prog_attach_xattr 2019-12-19 1:56 ` [PATCH v3 bpf-next 4/6] libbpf: Introduce bpf_prog_attach_xattr Andrey Ignatov @ 2019-12-19 5:49 ` Andrii Nakryiko 2019-12-19 7:03 ` Andrey Ignatov 0 siblings, 1 reply; 18+ messages in thread From: Andrii Nakryiko @ 2019-12-19 5:49 UTC (permalink / raw) To: Andrey Ignatov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team On Wed, Dec 18, 2019 at 6:56 PM Andrey Ignatov <rdna@fb.com> wrote: > > Introduce a new bpf_prog_attach_xattr function that, in addition to > program fd, target fd and attach type, accepts an extendable struct > bpf_prog_attach_opts. > > bpf_prog_attach_opts relies on DECLARE_LIBBPF_OPTS macro to maintain > backward and forward compatibility and has the following "optional" > attach attributes: > > * existing attach_flags, since it's not required when attaching in NONE > mode. Even though it's quite often used in MULTI and OVERRIDE mode it > seems to be a good idea to reduce number of arguments to > bpf_prog_attach_xattr; > > * newly introduced attribute of BPF_PROG_ATTACH command: replace_prog_fd > that is fd of previously attached cgroup-bpf program to replace if > 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). > > The struct bpf_prog_attach_opts is supposed to be used with > DECLARE_LIBBPF_OPTS macro. > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > --- > tools/lib/bpf/bpf.c | 16 +++++++++++++++- > tools/lib/bpf/bpf.h | 11 +++++++++++ > tools/lib/bpf/libbpf.map | 1 + > 3 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index 98596e15390f..ebb4f8d71bdb 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -466,6 +466,17 @@ 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) > +{ > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, opts, > + .flags = flags, > + ); > + > + return bpf_prog_attach_xattr(prog_fd, target_fd, type, &opts); > +} > + > +int bpf_prog_attach_xattr(int prog_fd, int target_fd, > + enum bpf_attach_type type, > + const struct bpf_prog_attach_opts *opts) > { > union bpf_attr attr; > You need to validate opts with OPTS_VALID macro (see btf_dump__emit_type_decl() for simple example). > @@ -473,7 +484,10 @@ int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type, > attr.target_fd = target_fd; > attr.attach_bpf_fd = prog_fd; > attr.attach_type = type; > - attr.attach_flags = flags; > + if (opts) { > + attr.attach_flags = opts->flags; > + attr.replace_bpf_fd = opts->replace_prog_fd; > + } Please use OPTS_GET() macro to fetch values from opts struct and provide default value if they are not specified. > > return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr)); > } [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 4/6] libbpf: Introduce bpf_prog_attach_xattr 2019-12-19 5:49 ` Andrii Nakryiko @ 2019-12-19 7:03 ` Andrey Ignatov 0 siblings, 0 replies; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 7:03 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:50 -0800]: > On Wed, Dec 18, 2019 at 6:56 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Introduce a new bpf_prog_attach_xattr function that, in addition to > > program fd, target fd and attach type, accepts an extendable struct > > bpf_prog_attach_opts. > > > > bpf_prog_attach_opts relies on DECLARE_LIBBPF_OPTS macro to maintain > > backward and forward compatibility and has the following "optional" > > attach attributes: > > > > * existing attach_flags, since it's not required when attaching in NONE > > mode. Even though it's quite often used in MULTI and OVERRIDE mode it > > seems to be a good idea to reduce number of arguments to > > bpf_prog_attach_xattr; > > > > * newly introduced attribute of BPF_PROG_ATTACH command: replace_prog_fd > > that is fd of previously attached cgroup-bpf program to replace if > > 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). > > > > The struct bpf_prog_attach_opts is supposed to be used with > > DECLARE_LIBBPF_OPTS macro. > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > --- > > tools/lib/bpf/bpf.c | 16 +++++++++++++++- > > tools/lib/bpf/bpf.h | 11 +++++++++++ > > tools/lib/bpf/libbpf.map | 1 + > > 3 files changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > > index 98596e15390f..ebb4f8d71bdb 100644 > > --- a/tools/lib/bpf/bpf.c > > +++ b/tools/lib/bpf/bpf.c > > @@ -466,6 +466,17 @@ 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) > > +{ > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, opts, > > + .flags = flags, > > + ); > > + > > + return bpf_prog_attach_xattr(prog_fd, target_fd, type, &opts); > > +} > > + > > +int bpf_prog_attach_xattr(int prog_fd, int target_fd, > > + enum bpf_attach_type type, > > + const struct bpf_prog_attach_opts *opts) > > { > > union bpf_attr attr; > > > > You need to validate opts with OPTS_VALID macro (see > btf_dump__emit_type_decl() for simple example). > > > @@ -473,7 +484,10 @@ int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type, > > attr.target_fd = target_fd; > > attr.attach_bpf_fd = prog_fd; > > attr.attach_type = type; > > - attr.attach_flags = flags; > > + if (opts) { > > + attr.attach_flags = opts->flags; > > + attr.replace_bpf_fd = opts->replace_prog_fd; > > + } > > Please use OPTS_GET() macro to fetch values from opts struct and > provide default value if they are not specified. OK, will do both OPTS_VALID and OPTS_GET in v4. > > > > > > return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr)); > > } > > [...] -- Andrey Ignatov ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 bpf-next 5/6] selftests/bpf: Convert test_cgroup_attach to prog_tests 2019-12-19 1:55 [PATCH v3 bpf-next 0/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov ` (3 preceding siblings ...) 2019-12-19 1:56 ` [PATCH v3 bpf-next 4/6] libbpf: Introduce bpf_prog_attach_xattr Andrey Ignatov @ 2019-12-19 1:56 ` Andrey Ignatov 2019-12-19 6:11 ` Andrii Nakryiko 2019-12-19 1:56 ` [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi Andrey Ignatov 5 siblings, 1 reply; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 1:56 UTC (permalink / raw) To: bpf; +Cc: Andrey Ignatov, ast, daniel, kafai, andriin, kernel-team Convert test_cgroup_attach to prog_tests. This change does a lot of things but in many cases it's pretty expensive to separate them, so they go in one commit. Nevertheless the logic is ketp as is and changes made are just moving things around, simplifying them (w/o changing the meaning of the tests) and making prog_tests compatible: * split the 3 tests in the file into 3 separate files in prog_tests/; * rename the test functions to test_<file_base_name>; * remove unused includes, constants, variables and functions from every test; * replace `if`-s with or `if (CHECK())` where additional context should be logged and with `if (CHECK_FAIL())` where line number is enough; * switch from `log_err()` to logging via `CHECK()`; * replace `assert`-s with `CHECK_FAIL()` to avoid crashing the whole test_progs if one assertion fails; * replace cgroup_helpers with test__join_cgroup() in cgroup_attach_override only, other tests need more fine-grained control for cgroup creation/deletion so cgroup_helpers are still used there; * simplify cgroup_attach_autodetach by switching to easiest possible program since this test doesn't really need such a complicated program as cgroup_attach_multi does; * remove test_cgroup_attach.c itself. Signed-off-by: Andrey Ignatov <rdna@fb.com> --- tools/testing/selftests/bpf/.gitignore | 1 - tools/testing/selftests/bpf/Makefile | 3 +- .../bpf/prog_tests/cgroup_attach_autodetach.c | 111 ++++ .../bpf/prog_tests/cgroup_attach_multi.c | 238 ++++++++ .../bpf/prog_tests/cgroup_attach_override.c | 148 +++++ .../selftests/bpf/test_cgroup_attach.c | 571 ------------------ 6 files changed, 498 insertions(+), 574 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c delete mode 100644 tools/testing/selftests/bpf/test_cgroup_attach.c diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore index ce5af95ede42..b139b3d75ebb 100644 --- a/tools/testing/selftests/bpf/.gitignore +++ b/tools/testing/selftests/bpf/.gitignore @@ -21,7 +21,6 @@ test_lirc_mode2_user get_cgroup_id_user test_skb_cgroup_id_user test_socket_cookie -test_cgroup_attach test_cgroup_storage test_select_reuseport test_flow_dissector diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index c652bd84ef0e..866fc1cadd7c 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -32,7 +32,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \ test_cgroup_storage \ test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \ - test_cgroup_attach test_progs-no_alu32 + test_progs-no_alu32 # Also test bpf-gcc, if present ifneq ($(BPF_GCC),) @@ -136,7 +136,6 @@ $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c $(OUTPUT)/test_netcnt: cgroup_helpers.c $(OUTPUT)/test_sock_fields: cgroup_helpers.c $(OUTPUT)/test_sysctl: cgroup_helpers.c -$(OUTPUT)/test_cgroup_attach: cgroup_helpers.c .PHONY: force diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c new file mode 100644 index 000000000000..5b13f2c6c402 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <test_progs.h> + +#include "cgroup_helpers.h" + +#define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" + +char bpf_log_buf[BPF_LOG_BUF_SIZE]; + +static int prog_load(void) +{ + struct bpf_insn prog[] = { + BPF_MOV64_IMM(BPF_REG_0, 1), /* r0 = 1 */ + BPF_EXIT_INSN(), + }; + size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn); + + return bpf_load_program(BPF_PROG_TYPE_CGROUP_SKB, + prog, insns_cnt, "GPL", 0, + bpf_log_buf, BPF_LOG_BUF_SIZE); +} + +void test_cgroup_attach_autodetach(void) +{ + __u32 duration = 0, prog_cnt = 4, attach_flags; + int allow_prog[2] = {-1}; + __u32 prog_ids[2] = {0}; + void *ptr = NULL; + int cg = 0, i; + int attempts; + + for (i = 0; i < ARRAY_SIZE(allow_prog); i++) { + allow_prog[i] = prog_load(); + if (CHECK(allow_prog[i] < 0, "prog_load", + "verifier output:\n%s\n-------\n", bpf_log_buf)) + goto err; + } + + if (CHECK_FAIL(setup_cgroup_environment())) + goto err; + + /* create a cgroup, attach two programs and remember their ids */ + cg = create_and_get_cgroup("/cg_autodetach"); + if (CHECK_FAIL(cg < 0)) + goto err; + + if (CHECK_FAIL(join_cgroup("/cg_autodetach"))) + goto err; + + for (i = 0; i < ARRAY_SIZE(allow_prog); i++) + if (CHECK(bpf_prog_attach(allow_prog[i], cg, + BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_MULTI), + "prog_attach", "prog[%d], errno=%d\n", i, errno)) + goto err; + + /* make sure that programs are attached and run some traffic */ + if (CHECK(bpf_prog_query(cg, BPF_CGROUP_INET_EGRESS, 0, &attach_flags, + prog_ids, &prog_cnt), + "prog_query", "errno=%d\n", errno)) + goto err; + if (CHECK_FAIL(system(PING_CMD))) + goto err; + + /* allocate some memory (4Mb) to pin the original cgroup */ + ptr = malloc(4 * (1 << 20)); + if (CHECK_FAIL(!ptr)) + goto err; + + /* close programs and cgroup fd */ + for (i = 0; i < ARRAY_SIZE(allow_prog); i++) { + close(allow_prog[i]); + allow_prog[i] = -1; + } + + close(cg); + cg = 0; + + /* leave the cgroup and remove it. don't detach programs */ + cleanup_cgroup_environment(); + + /* wait for the asynchronous auto-detachment. + * wait for no more than 5 sec and give up. + */ + for (i = 0; i < ARRAY_SIZE(prog_ids); i++) { + for (attempts = 5; attempts >= 0; attempts--) { + int fd = bpf_prog_get_fd_by_id(prog_ids[i]); + + if (fd < 0) + break; + + /* don't leave the fd open */ + close(fd); + + if (CHECK_FAIL(!attempts)) + goto err; + + sleep(1); + } + } + +err: + for (i = 0; i < ARRAY_SIZE(allow_prog); i++) + if (allow_prog[i] >= 0) + close(allow_prog[i]); + if (cg) + close(cg); + free(ptr); + cleanup_cgroup_environment(); +} diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c new file mode 100644 index 000000000000..4eaab7435044 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c @@ -0,0 +1,238 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <test_progs.h> + +#include "cgroup_helpers.h" + +#define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" + +char bpf_log_buf[BPF_LOG_BUF_SIZE]; + +static int map_fd = -1; + +static int prog_load_cnt(int verdict, int val) +{ + int cgroup_storage_fd, percpu_cgroup_storage_fd; + + if (map_fd < 0) + map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, 4, 8, 1, 0); + if (map_fd < 0) { + printf("failed to create map '%s'\n", strerror(errno)); + return -1; + } + + cgroup_storage_fd = bpf_create_map(BPF_MAP_TYPE_CGROUP_STORAGE, + sizeof(struct bpf_cgroup_storage_key), 8, 0, 0); + if (cgroup_storage_fd < 0) { + printf("failed to create map '%s'\n", strerror(errno)); + return -1; + } + + percpu_cgroup_storage_fd = bpf_create_map( + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, + sizeof(struct bpf_cgroup_storage_key), 8, 0, 0); + if (percpu_cgroup_storage_fd < 0) { + printf("failed to create map '%s'\n", strerror(errno)); + return -1; + } + + struct bpf_insn prog[] = { + BPF_MOV32_IMM(BPF_REG_0, 0), + BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_0, -4), /* *(u32 *)(fp - 4) = r0 */ + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), /* r2 = fp - 4 */ + 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_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), + BPF_MOV64_IMM(BPF_REG_2, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_local_storage), + BPF_MOV64_IMM(BPF_REG_1, val), + BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_W, BPF_REG_0, BPF_REG_1, 0, 0), + + BPF_LD_MAP_FD(BPF_REG_1, percpu_cgroup_storage_fd), + BPF_MOV64_IMM(BPF_REG_2, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_local_storage), + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_0, 0), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, 0x1), + BPF_STX_MEM(BPF_W, BPF_REG_0, BPF_REG_3, 0), + + BPF_MOV64_IMM(BPF_REG_0, verdict), /* r0 = verdict */ + BPF_EXIT_INSN(), + }; + size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn); + int ret; + + ret = bpf_load_program(BPF_PROG_TYPE_CGROUP_SKB, + prog, insns_cnt, "GPL", 0, + bpf_log_buf, BPF_LOG_BUF_SIZE); + + close(cgroup_storage_fd); + return ret; +} + +void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; + unsigned long long value; + __u32 duration = 0; + int i = 0; + + for (i = 0; i < ARRAY_SIZE(allow_prog); i++) { + allow_prog[i] = prog_load_cnt(1, 1 << i); + if (CHECK(allow_prog[i] < 0, "prog_load", + "verifier output:\n%s\n-------\n", bpf_log_buf)) + goto err; + } + + if (CHECK_FAIL(setup_cgroup_environment())) + goto err; + + cg1 = create_and_get_cgroup("/cg1"); + if (CHECK_FAIL(cg1 < 0)) + goto err; + cg2 = create_and_get_cgroup("/cg1/cg2"); + if (CHECK_FAIL(cg2 < 0)) + goto err; + cg3 = create_and_get_cgroup("/cg1/cg2/cg3"); + if (CHECK_FAIL(cg3 < 0)) + goto err; + cg4 = create_and_get_cgroup("/cg1/cg2/cg3/cg4"); + if (CHECK_FAIL(cg4 < 0)) + goto err; + cg5 = create_and_get_cgroup("/cg1/cg2/cg3/cg4/cg5"); + if (CHECK_FAIL(cg5 < 0)) + goto err; + + if (CHECK_FAIL(join_cgroup("/cg1/cg2/cg3/cg4/cg5"))) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_MULTI), + "prog0_attach_to_cg1_multi", "errno=%d\n", errno)) + goto err; + + if (CHECK(!bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_MULTI), + "fail_same_prog_attach_to_cg1", "unexpected success\n")) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog[1], cg1, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_MULTI), + "prog1_attach_to_cg1_multi", "errno=%d\n", errno)) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog[2], cg2, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_OVERRIDE), + "prog2_attach_to_cg2_override", "errno=%d\n", errno)) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_MULTI), + "prog3_attach_to_cg3_multi", "errno=%d\n", errno)) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog[4], cg4, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_OVERRIDE), + "prog4_attach_to_cg4_override", "errno=%d\n", errno)) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog[5], cg5, BPF_CGROUP_INET_EGRESS, 0), + "prog5_attach_to_cg5_none", "errno=%d\n", errno)) + goto err; + + CHECK_FAIL(system(PING_CMD)); + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); + CHECK_FAIL(value != 1 + 2 + 8 + 32); + + /* query the number of effective progs in cg5 */ + CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, + BPF_F_QUERY_EFFECTIVE, NULL, NULL, &prog_cnt)); + CHECK_FAIL(prog_cnt != 4); + /* retrieve prog_ids of effective progs in cg5 */ + CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, + BPF_F_QUERY_EFFECTIVE, &attach_flags, + prog_ids, &prog_cnt)); + CHECK_FAIL(prog_cnt != 4); + CHECK_FAIL(attach_flags != 0); + saved_prog_id = prog_ids[0]; + /* check enospc handling */ + prog_ids[0] = 0; + prog_cnt = 2; + CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, + BPF_F_QUERY_EFFECTIVE, &attach_flags, + prog_ids, &prog_cnt) != -1); + CHECK_FAIL(errno != ENOSPC); + CHECK_FAIL(prog_cnt != 4); + /* check that prog_ids are returned even when buffer is too small */ + CHECK_FAIL(prog_ids[0] != saved_prog_id); + /* retrieve prog_id of single attached prog in cg5 */ + prog_ids[0] = 0; + CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, 0, NULL, + prog_ids, &prog_cnt)); + CHECK_FAIL(prog_cnt != 1); + CHECK_FAIL(prog_ids[0] != saved_prog_id); + + /* detach bottom program and ping again */ + if (CHECK(bpf_prog_detach2(-1, cg5, BPF_CGROUP_INET_EGRESS), + "prog_detach_from_cg5", "errno=%d\n", errno)) + goto err; + + value = 0; + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); + CHECK_FAIL(system(PING_CMD)); + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); + CHECK_FAIL(value != 1 + 2 + 8 + 16); + + /* detach 3rd from bottom program and ping again */ + if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), + "fail_prog_detach_from_cg3", "unexpected success\n")) + goto err; + + if (CHECK(bpf_prog_detach2(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS), + "prog3_detach_from_cg3", "errno=%d\n", errno)) + goto err; + + value = 0; + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); + CHECK_FAIL(system(PING_CMD)); + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); + CHECK_FAIL(value != 1 + 2 + 16); + + /* detach 2nd from bottom program and ping again */ + if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), + "prog_detach_from_cg4", "errno=%d\n", errno)) + goto err; + + value = 0; + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); + CHECK_FAIL(system(PING_CMD)); + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); + CHECK_FAIL(value != 1 + 2 + 4); + + prog_cnt = 4; + CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, + BPF_F_QUERY_EFFECTIVE, &attach_flags, + prog_ids, &prog_cnt)); + CHECK_FAIL(prog_cnt != 3); + CHECK_FAIL(attach_flags != 0); + CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, 0, NULL, + prog_ids, &prog_cnt)); + CHECK_FAIL(prog_cnt != 0); + +err: + for (i = 0; i < ARRAY_SIZE(allow_prog); i++) + if (allow_prog[i] >= 0) + close(allow_prog[i]); + close(cg1); + close(cg2); + close(cg3); + close(cg4); + close(cg5); + cleanup_cgroup_environment(); +} diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c new file mode 100644 index 000000000000..9d8cb48b99de --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <test_progs.h> + +#include "cgroup_helpers.h" + +#define FOO "/foo" +#define BAR "/foo/bar/" +#define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" + +char bpf_log_buf[BPF_LOG_BUF_SIZE]; + +static int prog_load(int verdict) +{ + struct bpf_insn prog[] = { + BPF_MOV64_IMM(BPF_REG_0, verdict), /* r0 = verdict */ + BPF_EXIT_INSN(), + }; + size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn); + + return bpf_load_program(BPF_PROG_TYPE_CGROUP_SKB, + prog, insns_cnt, "GPL", 0, + bpf_log_buf, BPF_LOG_BUF_SIZE); +} + +void test_cgroup_attach_override(void) +{ + int drop_prog = -1, allow_prog = -1, foo = -1, bar = -1; + __u32 duration = 0; + + allow_prog = prog_load(1); + if (CHECK(allow_prog < 0, "prog_load_allow", + "verifier output:\n%s\n-------\n", bpf_log_buf)) + goto err; + + drop_prog = prog_load(0); + if (CHECK(drop_prog < 0, "prog_load_drop", + "verifier output:\n%s\n-------\n", bpf_log_buf)) + goto err; + + foo = test__join_cgroup(FOO); + if (CHECK(foo < 0, "cgroup_join_foo", "cgroup setup failed\n")) + goto err; + + if (CHECK(bpf_prog_attach(drop_prog, foo, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_OVERRIDE), + "prog_attach_drop_foo_override", + "attach prog to %s failed, errno=%d\n", FOO, errno)) + goto err; + + if (CHECK(!system(PING_CMD), "ping_fail", + "ping unexpectedly succeeded\n")) + goto err; + + bar = test__join_cgroup(BAR); + if (CHECK(bar < 0, "cgroup_join_bar", "cgroup setup failed\n")) + goto err; + + if (CHECK(!system(PING_CMD), "ping_fail", + "ping unexpectedly succeeded\n")) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_OVERRIDE), + "prog_attach_allow_bar_override", + "attach prog to %s failed, errno=%d\n", BAR, errno)) + goto err; + + if (CHECK(system(PING_CMD), "ping_ok", "ping failed\n")) + goto err; + + if (CHECK(bpf_prog_detach(bar, BPF_CGROUP_INET_EGRESS), + "prog_detach_bar", + "detach prog from %s failed, errno=%d\n", BAR, errno)) + goto err; + + if (CHECK(!system(PING_CMD), "ping_fail", + "ping unexpectedly succeeded\n")) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_OVERRIDE), + "prog_attach_allow_bar_override", + "attach prog to %s failed, errno=%d\n", BAR, errno)) + goto err; + + if (CHECK(bpf_prog_detach(foo, BPF_CGROUP_INET_EGRESS), + "prog_detach_foo", + "detach prog from %s failed, errno=%d\n", FOO, errno)) + goto err; + + if (CHECK(system(PING_CMD), "ping_ok", "ping failed\n")) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_OVERRIDE), + "prog_attach_allow_bar_override", + "attach prog to %s failed, errno=%d\n", BAR, errno)) + goto err; + + if (CHECK(!bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 0), + "fail_prog_attach_allow_bar_none", + "attach prog to %s unexpectedly succeeded\n", BAR)) + goto err; + + if (CHECK(bpf_prog_detach(bar, BPF_CGROUP_INET_EGRESS), + "prog_detach_bar", + "detach prog from %s failed, errno=%d\n", BAR, errno)) + goto err; + + if (CHECK(!bpf_prog_detach(foo, BPF_CGROUP_INET_EGRESS), + "fail_prog_detach_foo", + "double detach from %s unexpectedly succeeded\n", FOO)) + goto err; + + if (CHECK(bpf_prog_attach(allow_prog, foo, BPF_CGROUP_INET_EGRESS, 0), + "prog_attach_allow_foo_none", + "attach prog to %s failed, errno=%d\n", FOO, errno)) + goto err; + + if (CHECK(!bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 0), + "fail_prog_attach_allow_bar_none", + "attach prog to %s unexpectedly succeeded\n", BAR)) + goto err; + + if (CHECK(!bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_OVERRIDE), + "fail_prog_attach_allow_bar_override", + "attach prog to %s unexpectedly succeeded\n", BAR)) + goto err; + + if (CHECK(!bpf_prog_attach(allow_prog, foo, BPF_CGROUP_INET_EGRESS, + BPF_F_ALLOW_OVERRIDE), + "fail_prog_attach_allow_foo_override", + "attach prog to %s unexpectedly succeeded\n", FOO)) + goto err; + + if (CHECK(bpf_prog_attach(drop_prog, foo, BPF_CGROUP_INET_EGRESS, 0), + "prog_attach_drop_foo_none", + "attach prog to %s failed, errno=%d\n", FOO, errno)) + goto err; + +err: + close(foo); + close(bar); + close(allow_prog); + close(drop_prog); +} diff --git a/tools/testing/selftests/bpf/test_cgroup_attach.c b/tools/testing/selftests/bpf/test_cgroup_attach.c deleted file mode 100644 index 7671909ee1cb..000000000000 --- a/tools/testing/selftests/bpf/test_cgroup_attach.c +++ /dev/null @@ -1,571 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 - -/* eBPF example program: - * - * - Creates arraymap in kernel with 4 bytes keys and 8 byte values - * - * - Loads eBPF program - * - * The eBPF program accesses the map passed in to store two pieces of - * information. The number of invocations of the program, which maps - * to the number of packets received, is stored to key 0. Key 1 is - * incremented on each iteration by the number of bytes stored in - * the skb. The program also stores the number of received bytes - * in the cgroup storage. - * - * - Attaches the new program to a cgroup using BPF_PROG_ATTACH - * - * - Every second, reads map[0] and map[1] to see how many bytes and - * packets were seen on any socket of tasks in the given cgroup. - */ - -#define _GNU_SOURCE - -#include <stdio.h> -#include <stdlib.h> -#include <assert.h> -#include <sys/resource.h> -#include <sys/time.h> -#include <unistd.h> -#include <linux/filter.h> - -#include <linux/bpf.h> -#include <bpf/bpf.h> - -#include "bpf_util.h" -#include "bpf_rlimit.h" -#include "cgroup_helpers.h" - -#define FOO "/foo" -#define BAR "/foo/bar/" -#define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" - -char bpf_log_buf[BPF_LOG_BUF_SIZE]; - -#ifdef DEBUG -#define debug(args...) printf(args) -#else -#define debug(args...) -#endif - -static int prog_load(int verdict) -{ - int ret; - struct bpf_insn prog[] = { - BPF_MOV64_IMM(BPF_REG_0, verdict), /* r0 = verdict */ - BPF_EXIT_INSN(), - }; - size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn); - - ret = bpf_load_program(BPF_PROG_TYPE_CGROUP_SKB, - prog, insns_cnt, "GPL", 0, - bpf_log_buf, BPF_LOG_BUF_SIZE); - - if (ret < 0) { - log_err("Loading program"); - printf("Output from verifier:\n%s\n-------\n", bpf_log_buf); - return 0; - } - return ret; -} - -static int test_foo_bar(void) -{ - int drop_prog, allow_prog, foo = 0, bar = 0, rc = 0; - - allow_prog = prog_load(1); - if (!allow_prog) - goto err; - - drop_prog = prog_load(0); - if (!drop_prog) - goto err; - - if (setup_cgroup_environment()) - goto err; - - /* Create cgroup /foo, get fd, and join it */ - foo = create_and_get_cgroup(FOO); - if (foo < 0) - goto err; - - if (join_cgroup(FOO)) - goto err; - - if (bpf_prog_attach(drop_prog, foo, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_OVERRIDE)) { - log_err("Attaching prog to /foo"); - goto err; - } - - debug("Attached DROP prog. This ping in cgroup /foo should fail...\n"); - assert(system(PING_CMD) != 0); - - /* Create cgroup /foo/bar, get fd, and join it */ - bar = create_and_get_cgroup(BAR); - if (bar < 0) - goto err; - - if (join_cgroup(BAR)) - goto err; - - debug("Attached DROP prog. This ping in cgroup /foo/bar should fail...\n"); - assert(system(PING_CMD) != 0); - - if (bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_OVERRIDE)) { - log_err("Attaching prog to /foo/bar"); - goto err; - } - - debug("Attached PASS prog. This ping in cgroup /foo/bar should pass...\n"); - assert(system(PING_CMD) == 0); - - if (bpf_prog_detach(bar, BPF_CGROUP_INET_EGRESS)) { - log_err("Detaching program from /foo/bar"); - goto err; - } - - debug("Detached PASS from /foo/bar while DROP is attached to /foo.\n" - "This ping in cgroup /foo/bar should fail...\n"); - assert(system(PING_CMD) != 0); - - if (bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_OVERRIDE)) { - log_err("Attaching prog to /foo/bar"); - goto err; - } - - if (bpf_prog_detach(foo, BPF_CGROUP_INET_EGRESS)) { - log_err("Detaching program from /foo"); - goto err; - } - - debug("Attached PASS from /foo/bar and detached DROP from /foo.\n" - "This ping in cgroup /foo/bar should pass...\n"); - assert(system(PING_CMD) == 0); - - if (bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_OVERRIDE)) { - log_err("Attaching prog to /foo/bar"); - goto err; - } - - if (!bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 0)) { - errno = 0; - log_err("Unexpected success attaching prog to /foo/bar"); - goto err; - } - - if (bpf_prog_detach(bar, BPF_CGROUP_INET_EGRESS)) { - log_err("Detaching program from /foo/bar"); - goto err; - } - - if (!bpf_prog_detach(foo, BPF_CGROUP_INET_EGRESS)) { - errno = 0; - log_err("Unexpected success in double detach from /foo"); - goto err; - } - - if (bpf_prog_attach(allow_prog, foo, BPF_CGROUP_INET_EGRESS, 0)) { - log_err("Attaching non-overridable prog to /foo"); - goto err; - } - - if (!bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 0)) { - errno = 0; - log_err("Unexpected success attaching non-overridable prog to /foo/bar"); - goto err; - } - - if (!bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_OVERRIDE)) { - errno = 0; - log_err("Unexpected success attaching overridable prog to /foo/bar"); - goto err; - } - - if (!bpf_prog_attach(allow_prog, foo, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_OVERRIDE)) { - errno = 0; - log_err("Unexpected success attaching overridable prog to /foo"); - goto err; - } - - if (bpf_prog_attach(drop_prog, foo, BPF_CGROUP_INET_EGRESS, 0)) { - log_err("Attaching different non-overridable prog to /foo"); - goto err; - } - - goto out; - -err: - rc = 1; - -out: - close(foo); - close(bar); - cleanup_cgroup_environment(); - if (!rc) - printf("#override:PASS\n"); - else - printf("#override:FAIL\n"); - return rc; -} - -static int map_fd = -1; - -static int prog_load_cnt(int verdict, int val) -{ - int cgroup_storage_fd, percpu_cgroup_storage_fd; - - if (map_fd < 0) - map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, 4, 8, 1, 0); - if (map_fd < 0) { - printf("failed to create map '%s'\n", strerror(errno)); - return -1; - } - - cgroup_storage_fd = bpf_create_map(BPF_MAP_TYPE_CGROUP_STORAGE, - sizeof(struct bpf_cgroup_storage_key), 8, 0, 0); - if (cgroup_storage_fd < 0) { - printf("failed to create map '%s'\n", strerror(errno)); - return -1; - } - - percpu_cgroup_storage_fd = bpf_create_map( - BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, - sizeof(struct bpf_cgroup_storage_key), 8, 0, 0); - if (percpu_cgroup_storage_fd < 0) { - printf("failed to create map '%s'\n", strerror(errno)); - return -1; - } - - struct bpf_insn prog[] = { - BPF_MOV32_IMM(BPF_REG_0, 0), - BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_0, -4), /* *(u32 *)(fp - 4) = r0 */ - BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), - BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), /* r2 = fp - 4 */ - 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_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), - BPF_MOV64_IMM(BPF_REG_2, 0), - BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_local_storage), - BPF_MOV64_IMM(BPF_REG_1, val), - BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_W, BPF_REG_0, BPF_REG_1, 0, 0), - - BPF_LD_MAP_FD(BPF_REG_1, percpu_cgroup_storage_fd), - BPF_MOV64_IMM(BPF_REG_2, 0), - BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_local_storage), - BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_0, 0), - BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, 0x1), - BPF_STX_MEM(BPF_W, BPF_REG_0, BPF_REG_3, 0), - - BPF_MOV64_IMM(BPF_REG_0, verdict), /* r0 = verdict */ - BPF_EXIT_INSN(), - }; - size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn); - int ret; - - ret = bpf_load_program(BPF_PROG_TYPE_CGROUP_SKB, - prog, insns_cnt, "GPL", 0, - bpf_log_buf, BPF_LOG_BUF_SIZE); - - if (ret < 0) { - log_err("Loading program"); - printf("Output from verifier:\n%s\n-------\n", bpf_log_buf); - return 0; - } - close(cgroup_storage_fd); - return ret; -} - - -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; - unsigned long long value; - int i = 0; - - for (i = 0; i < 6; i++) { - allow_prog[i] = prog_load_cnt(1, 1 << i); - if (!allow_prog[i]) - goto err; - } - drop_prog = prog_load_cnt(0, 1); - if (!drop_prog) - goto err; - - if (setup_cgroup_environment()) - goto err; - - cg1 = create_and_get_cgroup("/cg1"); - if (cg1 < 0) - goto err; - cg2 = create_and_get_cgroup("/cg1/cg2"); - if (cg2 < 0) - goto err; - cg3 = create_and_get_cgroup("/cg1/cg2/cg3"); - if (cg3 < 0) - goto err; - cg4 = create_and_get_cgroup("/cg1/cg2/cg3/cg4"); - if (cg4 < 0) - goto err; - cg5 = create_and_get_cgroup("/cg1/cg2/cg3/cg4/cg5"); - if (cg5 < 0) - goto err; - - if (join_cgroup("/cg1/cg2/cg3/cg4/cg5")) - goto err; - - if (bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_MULTI)) { - log_err("Attaching prog to cg1"); - goto err; - } - if (!bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_MULTI)) { - log_err("Unexpected success attaching the same prog to cg1"); - goto err; - } - if (bpf_prog_attach(allow_prog[1], cg1, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_MULTI)) { - log_err("Attaching prog2 to cg1"); - goto err; - } - if (bpf_prog_attach(allow_prog[2], cg2, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_OVERRIDE)) { - log_err("Attaching prog to cg2"); - goto err; - } - if (bpf_prog_attach(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_MULTI)) { - log_err("Attaching prog to cg3"); - goto err; - } - if (bpf_prog_attach(allow_prog[4], cg4, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_OVERRIDE)) { - log_err("Attaching prog to cg4"); - goto err; - } - if (bpf_prog_attach(allow_prog[5], cg5, BPF_CGROUP_INET_EGRESS, 0)) { - log_err("Attaching prog to cg5"); - goto err; - } - assert(system(PING_CMD) == 0); - assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0); - assert(value == 1 + 2 + 8 + 32); - - /* query the number of effective progs in cg5 */ - assert(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, BPF_F_QUERY_EFFECTIVE, - NULL, NULL, &prog_cnt) == 0); - assert(prog_cnt == 4); - /* retrieve prog_ids of effective progs in cg5 */ - assert(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, BPF_F_QUERY_EFFECTIVE, - &attach_flags, prog_ids, &prog_cnt) == 0); - assert(prog_cnt == 4); - assert(attach_flags == 0); - saved_prog_id = prog_ids[0]; - /* check enospc handling */ - prog_ids[0] = 0; - prog_cnt = 2; - assert(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, BPF_F_QUERY_EFFECTIVE, - &attach_flags, prog_ids, &prog_cnt) == -1 && - errno == ENOSPC); - assert(prog_cnt == 4); - /* check that prog_ids are returned even when buffer is too small */ - assert(prog_ids[0] == saved_prog_id); - /* retrieve prog_id of single attached prog in cg5 */ - prog_ids[0] = 0; - assert(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, 0, - NULL, prog_ids, &prog_cnt) == 0); - assert(prog_cnt == 1); - assert(prog_ids[0] == saved_prog_id); - - /* detach bottom program and ping again */ - if (bpf_prog_detach2(-1, cg5, BPF_CGROUP_INET_EGRESS)) { - log_err("Detaching prog from cg5"); - 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 == 1 + 2 + 8 + 16); - - /* detach 3rd from bottom program and ping again */ - errno = 0; - if (!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS)) { - log_err("Unexpected success on detach from cg3"); - goto err; - } - if (bpf_prog_detach2(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS)) { - log_err("Detaching from cg3"); - 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 == 1 + 2 + 16); - - /* detach 2nd from bottom program and ping again */ - if (bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS)) { - log_err("Detaching prog from cg4"); - 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 == 1 + 2 + 4); - - prog_cnt = 4; - assert(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, BPF_F_QUERY_EFFECTIVE, - &attach_flags, prog_ids, &prog_cnt) == 0); - assert(prog_cnt == 3); - assert(attach_flags == 0); - assert(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, 0, - NULL, prog_ids, &prog_cnt) == 0); - assert(prog_cnt == 0); - goto out; -err: - rc = 1; - -out: - for (i = 0; i < 6; i++) - if (allow_prog[i] > 0) - close(allow_prog[i]); - close(cg1); - close(cg2); - close(cg3); - close(cg4); - close(cg5); - cleanup_cgroup_environment(); - if (!rc) - printf("#multi:PASS\n"); - else - printf("#multi:FAIL\n"); - return rc; -} - -static int test_autodetach(void) -{ - __u32 prog_cnt = 4, attach_flags; - int allow_prog[2] = {0}; - __u32 prog_ids[2] = {0}; - int cg = 0, i, rc = -1; - void *ptr = NULL; - int attempts; - - for (i = 0; i < ARRAY_SIZE(allow_prog); i++) { - allow_prog[i] = prog_load_cnt(1, 1 << i); - if (!allow_prog[i]) - goto err; - } - - if (setup_cgroup_environment()) - goto err; - - /* create a cgroup, attach two programs and remember their ids */ - cg = create_and_get_cgroup("/cg_autodetach"); - if (cg < 0) - goto err; - - if (join_cgroup("/cg_autodetach")) - goto err; - - for (i = 0; i < ARRAY_SIZE(allow_prog); i++) { - if (bpf_prog_attach(allow_prog[i], cg, BPF_CGROUP_INET_EGRESS, - BPF_F_ALLOW_MULTI)) { - log_err("Attaching prog[%d] to cg:egress", i); - goto err; - } - } - - /* make sure that programs are attached and run some traffic */ - assert(bpf_prog_query(cg, BPF_CGROUP_INET_EGRESS, 0, &attach_flags, - prog_ids, &prog_cnt) == 0); - assert(system(PING_CMD) == 0); - - /* allocate some memory (4Mb) to pin the original cgroup */ - ptr = malloc(4 * (1 << 20)); - if (!ptr) - goto err; - - /* close programs and cgroup fd */ - for (i = 0; i < ARRAY_SIZE(allow_prog); i++) { - close(allow_prog[i]); - allow_prog[i] = 0; - } - - close(cg); - cg = 0; - - /* leave the cgroup and remove it. don't detach programs */ - cleanup_cgroup_environment(); - - /* wait for the asynchronous auto-detachment. - * wait for no more than 5 sec and give up. - */ - for (i = 0; i < ARRAY_SIZE(prog_ids); i++) { - for (attempts = 5; attempts >= 0; attempts--) { - int fd = bpf_prog_get_fd_by_id(prog_ids[i]); - - if (fd < 0) - break; - - /* don't leave the fd open */ - close(fd); - - if (!attempts) - goto err; - - sleep(1); - } - } - - rc = 0; -err: - for (i = 0; i < ARRAY_SIZE(allow_prog); i++) - if (allow_prog[i] > 0) - close(allow_prog[i]); - if (cg) - close(cg); - free(ptr); - cleanup_cgroup_environment(); - if (!rc) - printf("#autodetach:PASS\n"); - else - printf("#autodetach:FAIL\n"); - return rc; -} - -int main(void) -{ - int (*tests[])(void) = { - test_foo_bar, - test_multiprog, - test_autodetach, - }; - int errors = 0; - int i; - - for (i = 0; i < ARRAY_SIZE(tests); i++) - if (tests[i]()) - errors++; - - if (errors) - printf("test_cgroup_attach:FAIL\n"); - else - printf("test_cgroup_attach:PASS\n"); - - return errors ? EXIT_FAILURE : EXIT_SUCCESS; -} -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 5/6] selftests/bpf: Convert test_cgroup_attach to prog_tests 2019-12-19 1:56 ` [PATCH v3 bpf-next 5/6] selftests/bpf: Convert test_cgroup_attach to prog_tests Andrey Ignatov @ 2019-12-19 6:11 ` Andrii Nakryiko 2019-12-19 7:20 ` Andrey Ignatov 0 siblings, 1 reply; 18+ messages in thread From: Andrii Nakryiko @ 2019-12-19 6:11 UTC (permalink / raw) To: Andrey Ignatov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team On Wed, Dec 18, 2019 at 6:14 PM Andrey Ignatov <rdna@fb.com> wrote: > > Convert test_cgroup_attach to prog_tests. > > This change does a lot of things but in many cases it's pretty expensive > to separate them, so they go in one commit. Nevertheless the logic is > ketp as is and changes made are just moving things around, simplifying > them (w/o changing the meaning of the tests) and making prog_tests > compatible: > > * split the 3 tests in the file into 3 separate files in prog_tests/; > > * rename the test functions to test_<file_base_name>; > > * remove unused includes, constants, variables and functions from every > test; > > * replace `if`-s with or `if (CHECK())` where additional context should > be logged and with `if (CHECK_FAIL())` where line number is enough; > > * switch from `log_err()` to logging via `CHECK()`; > > * replace `assert`-s with `CHECK_FAIL()` to avoid crashing the whole > test_progs if one assertion fails; > > * replace cgroup_helpers with test__join_cgroup() in > cgroup_attach_override only, other tests need more fine-grained > control for cgroup creation/deletion so cgroup_helpers are still used > there; > > * simplify cgroup_attach_autodetach by switching to easiest possible > program since this test doesn't really need such a complicated program > as cgroup_attach_multi does; > > * remove test_cgroup_attach.c itself. > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > --- > tools/testing/selftests/bpf/.gitignore | 1 - > tools/testing/selftests/bpf/Makefile | 3 +- > .../bpf/prog_tests/cgroup_attach_autodetach.c | 111 ++++ > .../bpf/prog_tests/cgroup_attach_multi.c | 238 ++++++++ > .../bpf/prog_tests/cgroup_attach_override.c | 148 +++++ > .../selftests/bpf/test_cgroup_attach.c | 571 ------------------ > 6 files changed, 498 insertions(+), 574 deletions(-) > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c > delete mode 100644 tools/testing/selftests/bpf/test_cgroup_attach.c > [...] > + > + if (CHECK_FAIL(setup_cgroup_environment())) > + goto err; > + > + cg1 = create_and_get_cgroup("/cg1"); > + if (CHECK_FAIL(cg1 < 0)) > + goto err; > + cg2 = create_and_get_cgroup("/cg1/cg2"); > + if (CHECK_FAIL(cg2 < 0)) > + goto err; > + cg3 = create_and_get_cgroup("/cg1/cg2/cg3"); > + if (CHECK_FAIL(cg3 < 0)) > + goto err; > + cg4 = create_and_get_cgroup("/cg1/cg2/cg3/cg4"); > + if (CHECK_FAIL(cg4 < 0)) > + goto err; > + cg5 = create_and_get_cgroup("/cg1/cg2/cg3/cg4/cg5"); > + if (CHECK_FAIL(cg5 < 0)) > + goto err; > + > + if (CHECK_FAIL(join_cgroup("/cg1/cg2/cg3/cg4/cg5"))) > + goto err; > + > + if (CHECK(bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, > + BPF_F_ALLOW_MULTI), > + "prog0_attach_to_cg1_multi", "errno=%d\n", errno)) > + goto err; > + > + if (CHECK(!bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, > + BPF_F_ALLOW_MULTI), > + "fail_same_prog_attach_to_cg1", "unexpected success\n")) > + goto err; > + > + if (CHECK(bpf_prog_attach(allow_prog[1], cg1, BPF_CGROUP_INET_EGRESS, > + BPF_F_ALLOW_MULTI), > + "prog1_attach_to_cg1_multi", "errno=%d\n", errno)) > + goto err; > + > + if (CHECK(bpf_prog_attach(allow_prog[2], cg2, BPF_CGROUP_INET_EGRESS, > + BPF_F_ALLOW_OVERRIDE), > + "prog2_attach_to_cg2_override", "errno=%d\n", errno)) > + goto err; > + > + if (CHECK(bpf_prog_attach(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS, > + BPF_F_ALLOW_MULTI), > + "prog3_attach_to_cg3_multi", "errno=%d\n", errno)) > + goto err; > + > + if (CHECK(bpf_prog_attach(allow_prog[4], cg4, BPF_CGROUP_INET_EGRESS, > + BPF_F_ALLOW_OVERRIDE), > + "prog4_attach_to_cg4_override", "errno=%d\n", errno)) > + goto err; > + > + if (CHECK(bpf_prog_attach(allow_prog[5], cg5, BPF_CGROUP_INET_EGRESS, 0), > + "prog5_attach_to_cg5_none", "errno=%d\n", errno)) > + goto err; nit: this looks like a good candidate for a loop... > + > + CHECK_FAIL(system(PING_CMD)); > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > + CHECK_FAIL(value != 1 + 2 + 8 + 32); > + [...] > -int main(void) > -{ > - int (*tests[])(void) = { > - test_foo_bar, > - test_multiprog, > - test_autodetach, > - }; > - int errors = 0; > - int i; > - > - for (i = 0; i < ARRAY_SIZE(tests); i++) > - if (tests[i]()) > - errors++; Depending on what you think is better structure (I couldn't follow through entire test logic...), you could have done this as a single test with 3 subtests. Search for test__start_subtest(). If that saves you some duplication, I think it's worth converting (which will be 1-to-1 with original structure). But either way thanks a lot for doing the convertion, brings as another step closer to more uniform and consolidated testing infra. > - > - if (errors) > - printf("test_cgroup_attach:FAIL\n"); > - else > - printf("test_cgroup_attach:PASS\n"); > - > - return errors ? EXIT_FAILURE : EXIT_SUCCESS; > -} > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 5/6] selftests/bpf: Convert test_cgroup_attach to prog_tests 2019-12-19 6:11 ` Andrii Nakryiko @ 2019-12-19 7:20 ` Andrey Ignatov 2019-12-19 20:40 ` Andrii Nakryiko 0 siblings, 1 reply; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 7:20 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 22:11 -0800]: > On Wed, Dec 18, 2019 at 6:14 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Convert test_cgroup_attach to prog_tests. > > > > This change does a lot of things but in many cases it's pretty expensive > > to separate them, so they go in one commit. Nevertheless the logic is > > ketp as is and changes made are just moving things around, simplifying > > them (w/o changing the meaning of the tests) and making prog_tests > > compatible: > > > > * split the 3 tests in the file into 3 separate files in prog_tests/; > > > > * rename the test functions to test_<file_base_name>; > > > > * remove unused includes, constants, variables and functions from every > > test; > > > > * replace `if`-s with or `if (CHECK())` where additional context should > > be logged and with `if (CHECK_FAIL())` where line number is enough; > > > > * switch from `log_err()` to logging via `CHECK()`; > > > > * replace `assert`-s with `CHECK_FAIL()` to avoid crashing the whole > > test_progs if one assertion fails; > > > > * replace cgroup_helpers with test__join_cgroup() in > > cgroup_attach_override only, other tests need more fine-grained > > control for cgroup creation/deletion so cgroup_helpers are still used > > there; > > > > * simplify cgroup_attach_autodetach by switching to easiest possible > > program since this test doesn't really need such a complicated program > > as cgroup_attach_multi does; > > > > * remove test_cgroup_attach.c itself. > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > --- > > tools/testing/selftests/bpf/.gitignore | 1 - > > tools/testing/selftests/bpf/Makefile | 3 +- > > .../bpf/prog_tests/cgroup_attach_autodetach.c | 111 ++++ > > .../bpf/prog_tests/cgroup_attach_multi.c | 238 ++++++++ > > .../bpf/prog_tests/cgroup_attach_override.c | 148 +++++ > > .../selftests/bpf/test_cgroup_attach.c | 571 ------------------ > > 6 files changed, 498 insertions(+), 574 deletions(-) > > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c > > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c > > delete mode 100644 tools/testing/selftests/bpf/test_cgroup_attach.c > > > > [...] > > > + > > + if (CHECK_FAIL(setup_cgroup_environment())) > > + goto err; > > + > > + cg1 = create_and_get_cgroup("/cg1"); > > + if (CHECK_FAIL(cg1 < 0)) > > + goto err; > > + cg2 = create_and_get_cgroup("/cg1/cg2"); > > + if (CHECK_FAIL(cg2 < 0)) > > + goto err; > > + cg3 = create_and_get_cgroup("/cg1/cg2/cg3"); > > + if (CHECK_FAIL(cg3 < 0)) > > + goto err; > > + cg4 = create_and_get_cgroup("/cg1/cg2/cg3/cg4"); > > + if (CHECK_FAIL(cg4 < 0)) > > + goto err; > > + cg5 = create_and_get_cgroup("/cg1/cg2/cg3/cg4/cg5"); > > + if (CHECK_FAIL(cg5 < 0)) > > + goto err; > > + > > + if (CHECK_FAIL(join_cgroup("/cg1/cg2/cg3/cg4/cg5"))) > > + goto err; > > + > > + if (CHECK(bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, > > + BPF_F_ALLOW_MULTI), > > + "prog0_attach_to_cg1_multi", "errno=%d\n", errno)) > > + goto err; > > + > > + if (CHECK(!bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, > > + BPF_F_ALLOW_MULTI), > > + "fail_same_prog_attach_to_cg1", "unexpected success\n")) > > + goto err; > > + > > + if (CHECK(bpf_prog_attach(allow_prog[1], cg1, BPF_CGROUP_INET_EGRESS, > > + BPF_F_ALLOW_MULTI), > > + "prog1_attach_to_cg1_multi", "errno=%d\n", errno)) > > + goto err; > > + > > + if (CHECK(bpf_prog_attach(allow_prog[2], cg2, BPF_CGROUP_INET_EGRESS, > > + BPF_F_ALLOW_OVERRIDE), > > + "prog2_attach_to_cg2_override", "errno=%d\n", errno)) > > + goto err; > > + > > + if (CHECK(bpf_prog_attach(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS, > > + BPF_F_ALLOW_MULTI), > > + "prog3_attach_to_cg3_multi", "errno=%d\n", errno)) > > + goto err; > > + > > + if (CHECK(bpf_prog_attach(allow_prog[4], cg4, BPF_CGROUP_INET_EGRESS, > > + BPF_F_ALLOW_OVERRIDE), > > + "prog4_attach_to_cg4_override", "errno=%d\n", errno)) > > + goto err; > > + > > + if (CHECK(bpf_prog_attach(allow_prog[5], cg5, BPF_CGROUP_INET_EGRESS, 0), > > + "prog5_attach_to_cg5_none", "errno=%d\n", errno)) > > + goto err; > > nit: this looks like a good candidate for a loop... These tests can benefit from a lot of cleanup steps and yeah, this can be one of them. I'd prefer to deal with it separately though since it shouldn't be a blocker for this patch set. That's why I preserved this logic with just adding checks. > > + CHECK_FAIL(system(PING_CMD)); > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > + CHECK_FAIL(value != 1 + 2 + 8 + 32); > > + > > [...] > > > -int main(void) > > -{ > > - int (*tests[])(void) = { > > - test_foo_bar, > > - test_multiprog, > > - test_autodetach, > > - }; > > - int errors = 0; > > - int i; > > - > > - for (i = 0; i < ARRAY_SIZE(tests); i++) > > - if (tests[i]()) > > - errors++; > > Depending on what you think is better structure (I couldn't follow > through entire test logic...), you could have done this as a single > test with 3 subtests. Search for test__start_subtest(). If that saves > you some duplication, I think it's worth converting (which will be > 1-to-1 with original structure). Yeah, I saw test__start_subtest() and checked if it would fit here. IMO it wouldn't since these tests don't have much in common and can't be converted to, say, parametrized tests that differ only in input that can be easily described by a struct. They do cover parts of same feature (cgroup attach) but focus on different parts of it. That's why separate files. This actually helped to identify and remove a bunch of junk from each test (like simplifying testing prog in autodetach). > But either way thanks a lot for doing the convertion, brings as > another step closer to more uniform and consolidated testing infra. > > > - > > - if (errors) > > - printf("test_cgroup_attach:FAIL\n"); > > - else > > - printf("test_cgroup_attach:PASS\n"); > > - > > - return errors ? EXIT_FAILURE : EXIT_SUCCESS; > > -} > > -- > > 2.17.1 > > -- Andrey Ignatov ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 5/6] selftests/bpf: Convert test_cgroup_attach to prog_tests 2019-12-19 7:20 ` Andrey Ignatov @ 2019-12-19 20:40 ` Andrii Nakryiko 0 siblings, 0 replies; 18+ messages in thread From: Andrii Nakryiko @ 2019-12-19 20:40 UTC (permalink / raw) To: Andrey Ignatov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team On Wed, Dec 18, 2019 at 11:21 PM Andrey Ignatov <rdna@fb.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 22:11 -0800]: > > On Wed, Dec 18, 2019 at 6:14 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > Convert test_cgroup_attach to prog_tests. > > > > > > This change does a lot of things but in many cases it's pretty expensive > > > to separate them, so they go in one commit. Nevertheless the logic is > > > ketp as is and changes made are just moving things around, simplifying > > > them (w/o changing the meaning of the tests) and making prog_tests > > > compatible: > > > > > > * split the 3 tests in the file into 3 separate files in prog_tests/; > > > > > > * rename the test functions to test_<file_base_name>; > > > > > > * remove unused includes, constants, variables and functions from every > > > test; > > > > > > * replace `if`-s with or `if (CHECK())` where additional context should > > > be logged and with `if (CHECK_FAIL())` where line number is enough; > > > > > > * switch from `log_err()` to logging via `CHECK()`; > > > > > > * replace `assert`-s with `CHECK_FAIL()` to avoid crashing the whole > > > test_progs if one assertion fails; > > > > > > * replace cgroup_helpers with test__join_cgroup() in > > > cgroup_attach_override only, other tests need more fine-grained > > > control for cgroup creation/deletion so cgroup_helpers are still used > > > there; > > > > > > * simplify cgroup_attach_autodetach by switching to easiest possible > > > program since this test doesn't really need such a complicated program > > > as cgroup_attach_multi does; > > > > > > * remove test_cgroup_attach.c itself. > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > --- > > > tools/testing/selftests/bpf/.gitignore | 1 - > > > tools/testing/selftests/bpf/Makefile | 3 +- > > > .../bpf/prog_tests/cgroup_attach_autodetach.c | 111 ++++ > > > .../bpf/prog_tests/cgroup_attach_multi.c | 238 ++++++++ > > > .../bpf/prog_tests/cgroup_attach_override.c | 148 +++++ > > > .../selftests/bpf/test_cgroup_attach.c | 571 ------------------ > > > 6 files changed, 498 insertions(+), 574 deletions(-) > > > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c > > > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c > > > delete mode 100644 tools/testing/selftests/bpf/test_cgroup_attach.c > > > > > > > [...] > > > > > + > > > + if (CHECK_FAIL(setup_cgroup_environment())) > > > + goto err; > > > + > > > + cg1 = create_and_get_cgroup("/cg1"); > > > + if (CHECK_FAIL(cg1 < 0)) > > > + goto err; > > > + cg2 = create_and_get_cgroup("/cg1/cg2"); > > > + if (CHECK_FAIL(cg2 < 0)) > > > + goto err; > > > + cg3 = create_and_get_cgroup("/cg1/cg2/cg3"); > > > + if (CHECK_FAIL(cg3 < 0)) > > > + goto err; > > > + cg4 = create_and_get_cgroup("/cg1/cg2/cg3/cg4"); > > > + if (CHECK_FAIL(cg4 < 0)) > > > + goto err; > > > + cg5 = create_and_get_cgroup("/cg1/cg2/cg3/cg4/cg5"); > > > + if (CHECK_FAIL(cg5 < 0)) > > > + goto err; > > > + > > > + if (CHECK_FAIL(join_cgroup("/cg1/cg2/cg3/cg4/cg5"))) > > > + goto err; > > > + > > > + if (CHECK(bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, > > > + BPF_F_ALLOW_MULTI), > > > + "prog0_attach_to_cg1_multi", "errno=%d\n", errno)) > > > + goto err; > > > + > > > + if (CHECK(!bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS, > > > + BPF_F_ALLOW_MULTI), > > > + "fail_same_prog_attach_to_cg1", "unexpected success\n")) > > > + goto err; > > > + > > > + if (CHECK(bpf_prog_attach(allow_prog[1], cg1, BPF_CGROUP_INET_EGRESS, > > > + BPF_F_ALLOW_MULTI), > > > + "prog1_attach_to_cg1_multi", "errno=%d\n", errno)) > > > + goto err; > > > + > > > + if (CHECK(bpf_prog_attach(allow_prog[2], cg2, BPF_CGROUP_INET_EGRESS, > > > + BPF_F_ALLOW_OVERRIDE), > > > + "prog2_attach_to_cg2_override", "errno=%d\n", errno)) > > > + goto err; > > > + > > > + if (CHECK(bpf_prog_attach(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS, > > > + BPF_F_ALLOW_MULTI), > > > + "prog3_attach_to_cg3_multi", "errno=%d\n", errno)) > > > + goto err; > > > + > > > + if (CHECK(bpf_prog_attach(allow_prog[4], cg4, BPF_CGROUP_INET_EGRESS, > > > + BPF_F_ALLOW_OVERRIDE), > > > + "prog4_attach_to_cg4_override", "errno=%d\n", errno)) > > > + goto err; > > > + > > > + if (CHECK(bpf_prog_attach(allow_prog[5], cg5, BPF_CGROUP_INET_EGRESS, 0), > > > + "prog5_attach_to_cg5_none", "errno=%d\n", errno)) > > > + goto err; > > > > nit: this looks like a good candidate for a loop... > > These tests can benefit from a lot of cleanup steps and yeah, this can > be one of them. I'd prefer to deal with it separately though since it > shouldn't be a blocker for this patch set. That's why I preserved this > logic with just adding checks. > Sure, no problem. > > > > + CHECK_FAIL(system(PING_CMD)); > > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > + CHECK_FAIL(value != 1 + 2 + 8 + 32); > > > + > > > > [...] > > > > > -int main(void) > > > -{ > > > - int (*tests[])(void) = { > > > - test_foo_bar, > > > - test_multiprog, > > > - test_autodetach, > > > - }; > > > - int errors = 0; > > > - int i; > > > - > > > - for (i = 0; i < ARRAY_SIZE(tests); i++) > > > - if (tests[i]()) > > > - errors++; > > > > Depending on what you think is better structure (I couldn't follow > > through entire test logic...), you could have done this as a single > > test with 3 subtests. Search for test__start_subtest(). If that saves > > you some duplication, I think it's worth converting (which will be > > 1-to-1 with original structure). > > Yeah, I saw test__start_subtest() and checked if it would fit here. IMO > it wouldn't since these tests don't have much in common and can't be > converted to, say, parametrized tests that differ only in input that can > be easily described by a struct. They do cover parts of same feature > (cgroup attach) but focus on different parts of it. That's why separate > files. This actually helped to identify and remove a bunch of junk from > each test (like simplifying testing prog in autodetach). OK, I don't insist, just wasn't sure if you are aware of subtests facility. Acked-by: Andrii Nakryiko <andriin@fb.com> > > > > But either way thanks a lot for doing the convertion, brings as > > another step closer to more uniform and consolidated testing infra. > > > > > - > > > - if (errors) > > > - printf("test_cgroup_attach:FAIL\n"); > > > - else > > > - printf("test_cgroup_attach:PASS\n"); > > > - > > > - return errors ? EXIT_FAILURE : EXIT_SUCCESS; > > > -} > > > -- > > > 2.17.1 > > > > > -- > Andrey Ignatov ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi 2019-12-19 1:55 [PATCH v3 bpf-next 0/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov ` (4 preceding siblings ...) 2019-12-19 1:56 ` [PATCH v3 bpf-next 5/6] selftests/bpf: Convert test_cgroup_attach to prog_tests Andrey Ignatov @ 2019-12-19 1:56 ` Andrey Ignatov 2019-12-19 5:58 ` Andrii Nakryiko 5 siblings, 1 reply; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 1:56 UTC (permalink / raw) To: bpf; +Cc: Andrey Ignatov, ast, daniel, kafai, andriin, kernel-team Test replacing 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 --args ./test_progs --name=cgroup_attach_multi ... Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 (gdb) [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 ID AttachType AttachFlags Name 2133 egress multi 2134 egress multi # fg gdb -q --args ./test_progs --name=cgroup_attach_multi (gdb) c Continuing. Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 (gdb) [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 ID AttachType AttachFlags Name 2139 egress multi 2134 egress multi Signed-off-by: Andrey Ignatov <rdna@fb.com> --- .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c index 4eaab7435044..2ff21dbce179 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); + int allow_prog[7] = {-1}; unsigned long long value; __u32 duration = 0; int i = 0; @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); CHECK_FAIL(value != 1 + 2 + 8 + 16); + /* test replace */ + + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; + attach_opts.replace_prog_fd = allow_prog[0]; + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "fail_prog_replace_override", "unexpected success\n")) + goto err; + CHECK_FAIL(errno != EINVAL); + + attach_opts.flags = BPF_F_REPLACE; + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "fail_prog_replace_no_multi", "unexpected success\n")) + goto err; + CHECK_FAIL(errno != EINVAL); + + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; + attach_opts.replace_prog_fd = -1; + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "fail_prog_replace_bad_fd", "unexpected success\n")) + goto err; + CHECK_FAIL(errno != EBADF); + + /* replacing a program that is not attached to cgroup should fail */ + attach_opts.replace_prog_fd = allow_prog[3]; + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "fail_prog_replace_no_ent", "unexpected success\n")) + goto err; + CHECK_FAIL(errno != ENOENT); + + /* replace 1st from the top program */ + attach_opts.replace_prog_fd = allow_prog[0]; + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "prog_replace", "errno=%d\n", errno)) + goto err; + + value = 0; + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); + CHECK_FAIL(system(PING_CMD)); + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); + CHECK_FAIL(value != 64 + 2 + 8 + 16); + /* detach 3rd from bottom program and ping again */ if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), "fail_prog_detach_from_cg3", "unexpected success\n")) @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); CHECK_FAIL(system(PING_CMD)); CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); - CHECK_FAIL(value != 1 + 2 + 16); + CHECK_FAIL(value != 64 + 2 + 16); /* detach 2nd from bottom program and ping again */ if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); CHECK_FAIL(system(PING_CMD)); CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); - CHECK_FAIL(value != 1 + 2 + 4); + CHECK_FAIL(value != 64 + 2 + 4); prog_cnt = 4; CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi 2019-12-19 1:56 ` [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi Andrey Ignatov @ 2019-12-19 5:58 ` Andrii Nakryiko 2019-12-19 7:35 ` Andrey Ignatov 0 siblings, 1 reply; 18+ messages in thread From: Andrii Nakryiko @ 2019-12-19 5:58 UTC (permalink / raw) To: Andrey Ignatov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > ... > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > (gdb) > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > ID AttachType AttachFlags Name > 2133 egress multi > 2134 egress multi > # fg > gdb -q --args ./test_progs --name=cgroup_attach_multi > (gdb) c > Continuing. > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > (gdb) > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > ID AttachType AttachFlags Name > 2139 egress multi > 2134 egress multi > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > --- > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > 1 file changed, 50 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > index 4eaab7435044..2ff21dbce179 100644 > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > + int allow_prog[7] = {-1}; > unsigned long long value; > __u32 duration = 0; > int i = 0; > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > + /* test replace */ > + > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > + attach_opts.replace_prog_fd = allow_prog[0]; > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "fail_prog_replace_override", "unexpected success\n")) > + goto err; > + CHECK_FAIL(errno != EINVAL); CHECK macro above can prints both in success and failure scenarios, which means that errno of bpf_prog_attach_xattr can be overriden by a bunch of other functions. So if this check is critical, you'd have to remember errno before calling CHECK. Same for all the check below. > + > + attach_opts.flags = BPF_F_REPLACE; > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "fail_prog_replace_no_multi", "unexpected success\n")) > + goto err; > + CHECK_FAIL(errno != EINVAL); > + > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > + attach_opts.replace_prog_fd = -1; > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "fail_prog_replace_bad_fd", "unexpected success\n")) > + goto err; > + CHECK_FAIL(errno != EBADF); > + > + /* replacing a program that is not attached to cgroup should fail */ > + attach_opts.replace_prog_fd = allow_prog[3]; > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "fail_prog_replace_no_ent", "unexpected success\n")) > + goto err; > + CHECK_FAIL(errno != ENOENT); > + > + /* replace 1st from the top program */ > + attach_opts.replace_prog_fd = allow_prog[0]; > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "prog_replace", "errno=%d\n", errno)) > + goto err; > + > + value = 0; > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > + CHECK_FAIL(system(PING_CMD)); > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > + > /* detach 3rd from bottom program and ping again */ > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > "fail_prog_detach_from_cg3", "unexpected success\n")) > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > CHECK_FAIL(system(PING_CMD)); > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > - CHECK_FAIL(value != 1 + 2 + 16); > + CHECK_FAIL(value != 64 + 2 + 16); > > /* detach 2nd from bottom program and ping again */ > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > CHECK_FAIL(system(PING_CMD)); > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > - CHECK_FAIL(value != 1 + 2 + 4); > + CHECK_FAIL(value != 64 + 2 + 4); > > prog_cnt = 4; > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi 2019-12-19 5:58 ` Andrii Nakryiko @ 2019-12-19 7:35 ` Andrey Ignatov 2019-12-19 20:44 ` Andrii Nakryiko 0 siblings, 1 reply; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 7:35 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > ... > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > (gdb) > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > ID AttachType AttachFlags Name > > 2133 egress multi > > 2134 egress multi > > # fg > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > (gdb) c > > Continuing. > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > (gdb) > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > ID AttachType AttachFlags Name > > 2139 egress multi > > 2134 egress multi > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > --- > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > index 4eaab7435044..2ff21dbce179 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > + int allow_prog[7] = {-1}; > > unsigned long long value; > > __u32 duration = 0; > > int i = 0; > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > + /* test replace */ > > + > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > + attach_opts.replace_prog_fd = allow_prog[0]; > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "fail_prog_replace_override", "unexpected success\n")) > > + goto err; > > + CHECK_FAIL(errno != EINVAL); > > CHECK macro above can prints both in success and failure scenarios, > which means that errno of bpf_prog_attach_xattr can be overriden by a > bunch of other functions. So if this check is critical, you'd have to > remember errno before calling CHECK. Same for all the check below. If bpf_prog_attach_xattr finishes successfully (what is unexpected here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't be run at all so "success" case is not a problem. If bpf_prog_attach_xattr fails (what is expected) it has to set errno and this is the errno that will be checked by CHECK_FAIL, i.e. failure case is not a problem at all. If you mean printf() that is called from "PASS" branch of CHECK then I don't actually see a way to distinguish errno from failed bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and printf() from the CHECK() w/o changing CHECK() itself. I think CHECK() can be improved wrt errno so that it saves errno before calling anything that can affect it and restore it afterwards. But this is not specific to this patch so IMO it should be done separately with, ideally, checking that it doesn't break some other tests. > > + > > + attach_opts.flags = BPF_F_REPLACE; > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > + goto err; > > + CHECK_FAIL(errno != EINVAL); > > + > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > + attach_opts.replace_prog_fd = -1; > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > + goto err; > > + CHECK_FAIL(errno != EBADF); > > + > > + /* replacing a program that is not attached to cgroup should fail */ > > + attach_opts.replace_prog_fd = allow_prog[3]; > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > + goto err; > > + CHECK_FAIL(errno != ENOENT); > > + > > + /* replace 1st from the top program */ > > + attach_opts.replace_prog_fd = allow_prog[0]; > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "prog_replace", "errno=%d\n", errno)) > > + goto err; > > + > > + value = 0; > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > + CHECK_FAIL(system(PING_CMD)); > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > + > > /* detach 3rd from bottom program and ping again */ > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > CHECK_FAIL(system(PING_CMD)); > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > - CHECK_FAIL(value != 1 + 2 + 16); > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > /* detach 2nd from bottom program and ping again */ > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > CHECK_FAIL(system(PING_CMD)); > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > - CHECK_FAIL(value != 1 + 2 + 4); > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > prog_cnt = 4; > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > -- > > 2.17.1 > > -- Andrey Ignatov ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi 2019-12-19 7:35 ` Andrey Ignatov @ 2019-12-19 20:44 ` Andrii Nakryiko 2019-12-19 21:09 ` Andrey Ignatov 0 siblings, 1 reply; 18+ messages in thread From: Andrii Nakryiko @ 2019-12-19 20:44 UTC (permalink / raw) To: Andrey Ignatov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team On Wed, Dec 18, 2019 at 11:35 PM Andrey Ignatov <rdna@fb.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > > ... > > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > > (gdb) > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > ID AttachType AttachFlags Name > > > 2133 egress multi > > > 2134 egress multi > > > # fg > > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > > (gdb) c > > > Continuing. > > > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > > (gdb) > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > ID AttachType AttachFlags Name > > > 2139 egress multi > > > 2134 egress multi > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > --- > > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > index 4eaab7435044..2ff21dbce179 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > > + int allow_prog[7] = {-1}; > > > unsigned long long value; > > > __u32 duration = 0; > > > int i = 0; > > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > > > + /* test replace */ > > > + > > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "fail_prog_replace_override", "unexpected success\n")) > > > + goto err; > > > + CHECK_FAIL(errno != EINVAL); > > > > CHECK macro above can prints both in success and failure scenarios, > > which means that errno of bpf_prog_attach_xattr can be overriden by a > > bunch of other functions. So if this check is critical, you'd have to > > remember errno before calling CHECK. Same for all the check below. > > If bpf_prog_attach_xattr finishes successfully (what is unexpected > here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't > be run at all so "success" case is not a problem. > > If bpf_prog_attach_xattr fails (what is expected) it has to set errno > and this is the errno that will be checked by CHECK_FAIL, i.e. failure > case is not a problem at all. > > If you mean printf() that is called from "PASS" branch of CHECK then I > don't actually see a way to distinguish errno from failed > bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and > printf() from the CHECK() w/o changing CHECK() itself. well, of course you can do that: err = bpf_xxx(...); saved_errno = errno; if (CHECK(!err, ...)) goto handle_err; if (CHECK_FAIL(saved_errno)) { ... } It's just more cumbersome, but nothing impossible. > > I think CHECK() can be improved wrt errno so that it saves errno before > calling anything that can affect it and restore it afterwards. But this > is not specific to this patch so IMO it should be done separately with, > ideally, checking that it doesn't break some other tests. That might be a way to do this, of course. We can add that later. I think that errno for error code is quite inconvenient, I wonder if it might be possible to retrofit kernel-style error code as a result instead. That's what we do for high-level libbpf API, it might be too late for low-level one though. > > > > + > > > + attach_opts.flags = BPF_F_REPLACE; > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > > + goto err; > > > + CHECK_FAIL(errno != EINVAL); > > > + > > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > > + attach_opts.replace_prog_fd = -1; > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > > + goto err; > > > + CHECK_FAIL(errno != EBADF); > > > + > > > + /* replacing a program that is not attached to cgroup should fail */ > > > + attach_opts.replace_prog_fd = allow_prog[3]; > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > > + goto err; > > > + CHECK_FAIL(errno != ENOENT); > > > + > > > + /* replace 1st from the top program */ > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "prog_replace", "errno=%d\n", errno)) > > > + goto err; > > > + > > > + value = 0; > > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > + CHECK_FAIL(system(PING_CMD)); > > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > > + > > > /* detach 3rd from bottom program and ping again */ > > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > CHECK_FAIL(system(PING_CMD)); > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > - CHECK_FAIL(value != 1 + 2 + 16); > > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > > > /* detach 2nd from bottom program and ping again */ > > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > CHECK_FAIL(system(PING_CMD)); > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > - CHECK_FAIL(value != 1 + 2 + 4); > > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > > > prog_cnt = 4; > > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > > -- > > > 2.17.1 > > > > > -- > Andrey Ignatov ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi 2019-12-19 20:44 ` Andrii Nakryiko @ 2019-12-19 21:09 ` Andrey Ignatov 2019-12-19 21:19 ` Andrii Nakryiko 0 siblings, 1 reply; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 21:09 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2019-12-19 12:44 -0800]: > On Wed, Dec 18, 2019 at 11:35 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > > > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > > > ... > > > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > > > (gdb) > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > ID AttachType AttachFlags Name > > > > 2133 egress multi > > > > 2134 egress multi > > > > # fg > > > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > (gdb) c > > > > Continuing. > > > > > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > > > (gdb) > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > ID AttachType AttachFlags Name > > > > 2139 egress multi > > > > 2134 egress multi > > > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > > --- > > > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > index 4eaab7435044..2ff21dbce179 100644 > > > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > > > + int allow_prog[7] = {-1}; > > > > unsigned long long value; > > > > __u32 duration = 0; > > > > int i = 0; > > > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > > > > > + /* test replace */ > > > > + > > > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "fail_prog_replace_override", "unexpected success\n")) > > > > + goto err; > > > > + CHECK_FAIL(errno != EINVAL); > > > > > > CHECK macro above can prints both in success and failure scenarios, > > > which means that errno of bpf_prog_attach_xattr can be overriden by a > > > bunch of other functions. So if this check is critical, you'd have to > > > remember errno before calling CHECK. Same for all the check below. > > > > If bpf_prog_attach_xattr finishes successfully (what is unexpected > > here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't > > be run at all so "success" case is not a problem. > > > > If bpf_prog_attach_xattr fails (what is expected) it has to set errno > > and this is the errno that will be checked by CHECK_FAIL, i.e. failure > > case is not a problem at all. > > > > If you mean printf() that is called from "PASS" branch of CHECK then I > > don't actually see a way to distinguish errno from failed > > bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and > > printf() from the CHECK() w/o changing CHECK() itself. > > well, of course you can do that: > > err = bpf_xxx(...); > saved_errno = errno; > if (CHECK(!err, ...)) > goto handle_err; > if (CHECK_FAIL(saved_errno)) { ... } > > It's just more cumbersome, but nothing impossible. Oh, yeah, that would work, but I agree that it doesn't look great :) > > I think CHECK() can be improved wrt errno so that it saves errno before > > calling anything that can affect it and restore it afterwards. But this > > is not specific to this patch so IMO it should be done separately with, > > ideally, checking that it doesn't break some other tests. > > That might be a way to do this, of course. We can add that later. Sounds good. I can follow-up on the CHECK() change separately so that every test that does similar checks doesn't have to have additional code to deal with errno. > I > think that errno for error code is quite inconvenient, I wonder if it > might be possible to retrofit kernel-style error code as a result > instead. That's what we do for high-level libbpf API, it might be too > late for low-level one though. There are a bunch of places that actually do it in bpf.c, e.g. bpf_prog_test_run_xattr() and bpf_load_program_xattr() return -EINVAL for error conditions unrelated to sys_bpf. I also added similar thing in patch 4 (libbpf part) for `!OPTS_VALID` scenario. But for vast majority of cases where wrappers simply return whatever sys_bpf returned IMO it makes sense to preserve the semantics similar to that of sys_bpf since it's just thin wrappers. Again, just my opinion. > > > > + > > > > + attach_opts.flags = BPF_F_REPLACE; > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > > > + goto err; > > > > + CHECK_FAIL(errno != EINVAL); > > > > + > > > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > > > + attach_opts.replace_prog_fd = -1; > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > > > + goto err; > > > > + CHECK_FAIL(errno != EBADF); > > > > + > > > > + /* replacing a program that is not attached to cgroup should fail */ > > > > + attach_opts.replace_prog_fd = allow_prog[3]; > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > > > + goto err; > > > > + CHECK_FAIL(errno != ENOENT); > > > > + > > > > + /* replace 1st from the top program */ > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "prog_replace", "errno=%d\n", errno)) > > > > + goto err; > > > > + > > > > + value = 0; > > > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > + CHECK_FAIL(system(PING_CMD)); > > > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > > > + > > > > /* detach 3rd from bottom program and ping again */ > > > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > CHECK_FAIL(system(PING_CMD)); > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > - CHECK_FAIL(value != 1 + 2 + 16); > > > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > > > > > /* detach 2nd from bottom program and ping again */ > > > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > CHECK_FAIL(system(PING_CMD)); > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > - CHECK_FAIL(value != 1 + 2 + 4); > > > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > > > > > prog_cnt = 4; > > > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > > > -- > > > > 2.17.1 > > > > > > > > -- > > Andrey Ignatov -- Andrey Ignatov ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi 2019-12-19 21:09 ` Andrey Ignatov @ 2019-12-19 21:19 ` Andrii Nakryiko 2019-12-19 21:25 ` Andrey Ignatov 0 siblings, 1 reply; 18+ messages in thread From: Andrii Nakryiko @ 2019-12-19 21:19 UTC (permalink / raw) To: Andrey Ignatov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team On Thu, Dec 19, 2019 at 1:09 PM Andrey Ignatov <rdna@fb.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2019-12-19 12:44 -0800]: > > On Wed, Dec 18, 2019 at 11:35 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > > > > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > > > > ... > > > > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > > > > (gdb) > > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > > ID AttachType AttachFlags Name > > > > > 2133 egress multi > > > > > 2134 egress multi > > > > > # fg > > > > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > (gdb) c > > > > > Continuing. > > > > > > > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > > > > (gdb) > > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > > ID AttachType AttachFlags Name > > > > > 2139 egress multi > > > > > 2134 egress multi > > > > > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > > > --- > > > > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > > > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > index 4eaab7435044..2ff21dbce179 100644 > > > > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > > > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > > > > + int allow_prog[7] = {-1}; > > > > > unsigned long long value; > > > > > __u32 duration = 0; > > > > > int i = 0; > > > > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > > > > > > > + /* test replace */ > > > > > + > > > > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "fail_prog_replace_override", "unexpected success\n")) > > > > > + goto err; > > > > > + CHECK_FAIL(errno != EINVAL); > > > > > > > > CHECK macro above can prints both in success and failure scenarios, > > > > which means that errno of bpf_prog_attach_xattr can be overriden by a > > > > bunch of other functions. So if this check is critical, you'd have to > > > > remember errno before calling CHECK. Same for all the check below. > > > > > > If bpf_prog_attach_xattr finishes successfully (what is unexpected > > > here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't > > > be run at all so "success" case is not a problem. > > > > > > If bpf_prog_attach_xattr fails (what is expected) it has to set errno > > > and this is the errno that will be checked by CHECK_FAIL, i.e. failure > > > case is not a problem at all. > > > > > > If you mean printf() that is called from "PASS" branch of CHECK then I > > > don't actually see a way to distinguish errno from failed > > > bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and > > > printf() from the CHECK() w/o changing CHECK() itself. > > > > well, of course you can do that: > > > > err = bpf_xxx(...); > > saved_errno = errno; > > if (CHECK(!err, ...)) > > goto handle_err; > > if (CHECK_FAIL(saved_errno)) { ... } > > > > It's just more cumbersome, but nothing impossible. > > Oh, yeah, that would work, but I agree that it doesn't look great :) > > > > I think CHECK() can be improved wrt errno so that it saves errno before > > > calling anything that can affect it and restore it afterwards. But this > > > is not specific to this patch so IMO it should be done separately with, > > > ideally, checking that it doesn't break some other tests. > > > > That might be a way to do this, of course. We can add that later. > > Sounds good. I can follow-up on the CHECK() change separately so that > every test that does similar checks doesn't have to have additional code > to deal with errno. great > > > I > > think that errno for error code is quite inconvenient, I wonder if it > > might be possible to retrofit kernel-style error code as a result > > instead. That's what we do for high-level libbpf API, it might be too > > late for low-level one though. > > There are a bunch of places that actually do it in bpf.c, e.g. > bpf_prog_test_run_xattr() and bpf_load_program_xattr() return -EINVAL > for error conditions unrelated to sys_bpf. I also added similar > thing in patch 4 (libbpf part) for `!OPTS_VALID` scenario. > > But for vast majority of cases where wrappers simply return whatever > sys_bpf returned IMO it makes sense to preserve the semantics similar to > that of sys_bpf since it's just thin wrappers. Again, just my opinion. > can we have both? keep setting errno, but return -errno as a result? If it's already a hybrid approach we are using, I don't think this will break anything. > > > > > > + > > > > > + attach_opts.flags = BPF_F_REPLACE; > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > > > > + goto err; > > > > > + CHECK_FAIL(errno != EINVAL); > > > > > + > > > > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > > > > + attach_opts.replace_prog_fd = -1; > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > > > > + goto err; > > > > > + CHECK_FAIL(errno != EBADF); > > > > > + > > > > > + /* replacing a program that is not attached to cgroup should fail */ > > > > > + attach_opts.replace_prog_fd = allow_prog[3]; > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > > > > + goto err; > > > > > + CHECK_FAIL(errno != ENOENT); > > > > > + > > > > > + /* replace 1st from the top program */ > > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "prog_replace", "errno=%d\n", errno)) > > > > > + goto err; > > > > > + > > > > > + value = 0; > > > > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > + CHECK_FAIL(system(PING_CMD)); > > > > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > > > > + > > > > > /* detach 3rd from bottom program and ping again */ > > > > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > > > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > > > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > CHECK_FAIL(system(PING_CMD)); > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > - CHECK_FAIL(value != 1 + 2 + 16); > > > > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > > > > > > > /* detach 2nd from bottom program and ping again */ > > > > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > > > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > CHECK_FAIL(system(PING_CMD)); > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > - CHECK_FAIL(value != 1 + 2 + 4); > > > > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > > > > > > > prog_cnt = 4; > > > > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > > > > -- > > > > > 2.17.1 > > > > > > > > > > > -- > > > Andrey Ignatov > > -- > Andrey Ignatov ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi 2019-12-19 21:19 ` Andrii Nakryiko @ 2019-12-19 21:25 ` Andrey Ignatov 0 siblings, 0 replies; 18+ messages in thread From: Andrey Ignatov @ 2019-12-19 21:25 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko, Kernel Team Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2019-12-19 13:19 -0800]: > On Thu, Dec 19, 2019 at 1:09 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2019-12-19 12:44 -0800]: > > > On Wed, Dec 18, 2019 at 11:35 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > > > > > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > > > > > ... > > > > > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > > > > > (gdb) > > > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > > > ID AttachType AttachFlags Name > > > > > > 2133 egress multi > > > > > > 2134 egress multi > > > > > > # fg > > > > > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > > (gdb) c > > > > > > Continuing. > > > > > > > > > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > > > > > (gdb) > > > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > > > ID AttachType AttachFlags Name > > > > > > 2139 egress multi > > > > > > 2134 egress multi > > > > > > > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > > > > --- > > > > > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > > > > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > > index 4eaab7435044..2ff21dbce179 100644 > > > > > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > > > > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > > > > > + int allow_prog[7] = {-1}; > > > > > > unsigned long long value; > > > > > > __u32 duration = 0; > > > > > > int i = 0; > > > > > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > > > > > > > > > + /* test replace */ > > > > > > + > > > > > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "fail_prog_replace_override", "unexpected success\n")) > > > > > > + goto err; > > > > > > + CHECK_FAIL(errno != EINVAL); > > > > > > > > > > CHECK macro above can prints both in success and failure scenarios, > > > > > which means that errno of bpf_prog_attach_xattr can be overriden by a > > > > > bunch of other functions. So if this check is critical, you'd have to > > > > > remember errno before calling CHECK. Same for all the check below. > > > > > > > > If bpf_prog_attach_xattr finishes successfully (what is unexpected > > > > here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't > > > > be run at all so "success" case is not a problem. > > > > > > > > If bpf_prog_attach_xattr fails (what is expected) it has to set errno > > > > and this is the errno that will be checked by CHECK_FAIL, i.e. failure > > > > case is not a problem at all. > > > > > > > > If you mean printf() that is called from "PASS" branch of CHECK then I > > > > don't actually see a way to distinguish errno from failed > > > > bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and > > > > printf() from the CHECK() w/o changing CHECK() itself. > > > > > > well, of course you can do that: > > > > > > err = bpf_xxx(...); > > > saved_errno = errno; > > > if (CHECK(!err, ...)) > > > goto handle_err; > > > if (CHECK_FAIL(saved_errno)) { ... } > > > > > > It's just more cumbersome, but nothing impossible. > > > > Oh, yeah, that would work, but I agree that it doesn't look great :) > > > > > > I think CHECK() can be improved wrt errno so that it saves errno before > > > > calling anything that can affect it and restore it afterwards. But this > > > > is not specific to this patch so IMO it should be done separately with, > > > > ideally, checking that it doesn't break some other tests. > > > > > > That might be a way to do this, of course. We can add that later. > > > > Sounds good. I can follow-up on the CHECK() change separately so that > > every test that does similar checks doesn't have to have additional code > > to deal with errno. > > great > > > > > > I > > > think that errno for error code is quite inconvenient, I wonder if it > > > might be possible to retrofit kernel-style error code as a result > > > instead. That's what we do for high-level libbpf API, it might be too > > > late for low-level one though. > > > > There are a bunch of places that actually do it in bpf.c, e.g. > > bpf_prog_test_run_xattr() and bpf_load_program_xattr() return -EINVAL > > for error conditions unrelated to sys_bpf. I also added similar > > thing in patch 4 (libbpf part) for `!OPTS_VALID` scenario. > > > > But for vast majority of cases where wrappers simply return whatever > > sys_bpf returned IMO it makes sense to preserve the semantics similar to > > that of sys_bpf since it's just thin wrappers. Again, just my opinion. > > > > can we have both? keep setting errno, but return -errno as a result? > If it's already a hybrid approach we are using, I don't think this > will break anything. I'd be hesitant to do it for all existing interfaces in bpf.h, since it's quite common patter to have code like: if (some_func() == -1) /* handle_error */ , instead of `if (some_func() < 0)`. Even `git grep ' == -1' tools/testing/selftests/bpf` shows a lot of such call sites, so it may break a lot of stuff. Anyway IMO it's out of the scope of my patch set :) > > > > > > + > > > > > > + attach_opts.flags = BPF_F_REPLACE; > > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > > > > > + goto err; > > > > > > + CHECK_FAIL(errno != EINVAL); > > > > > > + > > > > > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > > > > > + attach_opts.replace_prog_fd = -1; > > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > > > > > + goto err; > > > > > > + CHECK_FAIL(errno != EBADF); > > > > > > + > > > > > > + /* replacing a program that is not attached to cgroup should fail */ > > > > > > + attach_opts.replace_prog_fd = allow_prog[3]; > > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > > > > > + goto err; > > > > > > + CHECK_FAIL(errno != ENOENT); > > > > > > + > > > > > > + /* replace 1st from the top program */ > > > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "prog_replace", "errno=%d\n", errno)) > > > > > > + goto err; > > > > > > + > > > > > > + value = 0; > > > > > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > > + CHECK_FAIL(system(PING_CMD)); > > > > > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > > > > > + > > > > > > /* detach 3rd from bottom program and ping again */ > > > > > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > > > > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > > > > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > > CHECK_FAIL(system(PING_CMD)); > > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > > - CHECK_FAIL(value != 1 + 2 + 16); > > > > > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > > > > > > > > > /* detach 2nd from bottom program and ping again */ > > > > > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > > > > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > > CHECK_FAIL(system(PING_CMD)); > > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > > - CHECK_FAIL(value != 1 + 2 + 4); > > > > > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > > > > > > > > > prog_cnt = 4; > > > > > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > > > > > -- > > > > > > 2.17.1 > > > > > > > > > > > > > > -- > > > > Andrey Ignatov > > > > -- > > Andrey Ignatov -- Andrey Ignatov ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-12-19 21:25 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-19 1:55 [PATCH v3 bpf-next 0/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov 2019-12-19 1:55 ` [PATCH v3 bpf-next 1/6] bpf: Simplify __cgroup_bpf_attach Andrey Ignatov 2019-12-19 1:55 ` [PATCH v3 bpf-next 2/6] bpf: Remove unused new_flags in hierarchy_allows_attach() Andrey Ignatov 2019-12-19 1:56 ` [PATCH v3 bpf-next 3/6] bpf: Support replacing cgroup-bpf program in MULTI mode Andrey Ignatov 2019-12-19 1:56 ` [PATCH v3 bpf-next 4/6] libbpf: Introduce bpf_prog_attach_xattr Andrey Ignatov 2019-12-19 5:49 ` Andrii Nakryiko 2019-12-19 7:03 ` Andrey Ignatov 2019-12-19 1:56 ` [PATCH v3 bpf-next 5/6] selftests/bpf: Convert test_cgroup_attach to prog_tests Andrey Ignatov 2019-12-19 6:11 ` Andrii Nakryiko 2019-12-19 7:20 ` Andrey Ignatov 2019-12-19 20:40 ` Andrii Nakryiko 2019-12-19 1:56 ` [PATCH v3 bpf-next 6/6] selftests/bpf: Test BPF_F_REPLACE in cgroup_attach_multi Andrey Ignatov 2019-12-19 5:58 ` Andrii Nakryiko 2019-12-19 7:35 ` Andrey Ignatov 2019-12-19 20:44 ` Andrii Nakryiko 2019-12-19 21:09 ` Andrey Ignatov 2019-12-19 21:19 ` Andrii Nakryiko 2019-12-19 21:25 ` Andrey Ignatov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).