bpf: Support replacing cgroup-bpf program in MULTI mode

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 moves DECLARE_LIBBPF_OPTS from libbpf.h to bpf.h Patch 5 extends libbpf API to support new set of attach attributes. Patch 6 adds selftest coverage for the new API.

DECLARE_LIBBPF_OPTS is the way to use option structures in a backward compatible and extensible way. It's available only in libbpf.h. Though public interfaces in bpf.h have the same requirement to accept options in a way that can be simply extended w/o introducing new xattr-functions every time.

libbpf.c depends on bpf.h, hence to share the macros a couple of options exist:
* either a common header should be introduced;
* or the macro can be moved to bpf.h and used by libbpf.h from there;

The former seems to be an overkill, so do the latter:
* move DECLARE_LIBBPF_OPTS from libbpf.h to bpf.h;
* move #include "bpf.h" from libbpf.c to libbpf.h not to break users of the macro who already include only libbpf.h.

That makes the macro available to use in bpf.{h,c} and should not break those who already use it.

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

---

 tools/lib/bpf/bpf.h    | 22 ++++++++++++++++++++++
 tools/lib/bpf/libbpf.c |  1 -
 tools/lib/bpf/libbpf.h | 24 ++----------------------
 3 files changed, 24 insertions(+), 23 deletions(-)

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

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

The struct bpf_prog_attach_opts is supposed to be used with DECLARE_LIBBPF_OPTS framework.

The opts argument is used directly in bpf_prog_attach_xattr implementation since at the time of adding all fields already exist in the kernel. New fields, if added, will need to be used via OPTS_* macros from libbpf_internal.h.

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

---
 tools/lib/bpf/bpf.c      | 21 +++++++++++++++++----
 tools/lib/bpf/bpf.h      | 12 ++++++++++++
 tools/lib/bpf/libbpf.map |  2 ++
 3 files changed, 31 insertions(+), 4 deletions(-)

tools/lib/bpf/bpf.c +2 -0

index 98596e15390f..9f4e42abd185 100644
@@ -466,14 +466,27 @@ 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,

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

 {
        union bpf_attr attr;
 
        memset(&attr, 0, sizeof(attr));
-       attr.target_fd     = target_fd;
-       attr.attach_bpf_fd = prog_fd;
-       attr.attach_type   = type;
-       attr.attach_flags  = flags;
+       attr.target_fd     = opts->target_fd;
+       attr.attach_bpf_fd = opts->prog_fd;
+       attr.attach_type   = opts->type;
+       attr.attach_flags  = opts->flags;
+       attr.replace_bpf_fd = opts->replace_prog_fd;
 
        return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr));
 }

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

index 5cfe6e0a1aef..5b5f9b374074 100644
@@ -150,8 +150,20 @@ 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 */
+       int target_fd;
+       int prog_fd;
+       enum bpf_attach_type type;
+       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(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);

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

index 495df575f87f..42b065454031 100644
@@ -210,4 +210,6 @@ LIBBPF_0.0.6 {
 } LIBBPF_0.0.5;
 
 LIBBPF_0.0.7 {
+       global:
+               bpf_prog_attach_xattr;
 } LIBBPF_0.0.6;
--

2.17.1

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

Example of program replacing:

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

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

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

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

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

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

+
+       attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE;
+       if (!bpf_prog_attach_xattr(&attach_opts)) {
+               log_err("Unexpected success with OVERRIDE | REPLACE");
+               goto err;
+       }
+       assert(errno == EINVAL);
+

Quoted 35 lines

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

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

The struct bpf_prog_attach_opts is supposed to be used with DECLARE_LIBBPF_OPTS framework.

The opts argument is used directly in bpf_prog_attach_xattr implementation since at the time of adding all fields already exist in the kernel. New fields, if added, will need to be used via OPTS_* macros from libbpf_internal.h.

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

---
 tools/lib/bpf/bpf.c      | 21 +++++++++++++++++----
 tools/lib/bpf/bpf.h      | 12 ++++++++++++
 tools/lib/bpf/libbpf.map |  2 ++
 3 files changed, 31 insertions(+), 4 deletions(-)

tools/lib/bpf/bpf.c +2 -0

index 98596e15390f..9f4e42abd185 100644
@@ -466,14 +466,27 @@ 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,

tools/lib/bpf/bpf.c int bpf_obj_get(const char *pathname) · line 480

Quoted 10 lines

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

+               .target_fd = target_fd,
+               .prog_fd = prog_fd,
+               .type = type,
+               .flags = flags,
+       );
+
+       return bpf_prog_attach_xattr(&opts);
+}
+
+int bpf_prog_attach_xattr(const struct bpf_prog_attach_opts *opts)

When we discussed this whole OPTS idea, we agreed that specifying mandatory arguments as is makes for better usability. All the optional stuff then is moved into opts (and then extended indefinitely, because all the newly added stuff has to be optional, at least for some subset of arguments).

So if we were to follow those unofficial "guidelines", bpf_prog_attach_xattr would be defined as:

int bpf_prog_attach_xattr(int prog_fd, int target_fd, enum bpf_attach_type type,

                          const struct bpf_prog_attach_opts *opts);

, where opts has flags and replace_bpf_fd right now.

Naming wise, it's quite departure from xattr approach, so I'd probably would go with bpf_prog_attach_opts, but I won't insist.

WDYT?

Quoted 55 lines

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

 {
        union bpf_attr attr;
 
        memset(&attr, 0, sizeof(attr));
-       attr.target_fd     = target_fd;
-       attr.attach_bpf_fd = prog_fd;
-       attr.attach_type   = type;
-       attr.attach_flags  = flags;
+       attr.target_fd     = opts->target_fd;
+       attr.attach_bpf_fd = opts->prog_fd;
+       attr.attach_type   = opts->type;
+       attr.attach_flags  = opts->flags;
+       attr.replace_bpf_fd = opts->replace_prog_fd;
 
        return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr));
 }

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

index 5cfe6e0a1aef..5b5f9b374074 100644
@@ -150,8 +150,20 @@ 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 */
+       int target_fd;
+       int prog_fd;
+       enum bpf_attach_type type;
+       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(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);

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

index 495df575f87f..42b065454031 100644
@@ -210,4 +210,6 @@ LIBBPF_0.0.6 {
 } LIBBPF_0.0.5;
 
 LIBBPF_0.0.7 {
+       global:
+               bpf_prog_attach_xattr;
 } LIBBPF_0.0.6;
--

2.17.1

Andrii Nakryiko <[email protected]> [Thu, 2019-12-12 22:58 -0800]:

Quoted 53 lines · Andrey Ignatov

Quoted 35 lines

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

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

The struct bpf_prog_attach_opts is supposed to be used with DECLARE_LIBBPF_OPTS framework.

The opts argument is used directly in bpf_prog_attach_xattr implementation since at the time of adding all fields already exist in the kernel. New fields, if added, will need to be used via OPTS_* macros from libbpf_internal.h.

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

---
 tools/lib/bpf/bpf.c      | 21 +++++++++++++++++----
 tools/lib/bpf/bpf.h      | 12 ++++++++++++
 tools/lib/bpf/libbpf.map |  2 ++
 3 files changed, 31 insertions(+), 4 deletions(-)

tools/lib/bpf/bpf.c +2 -0

index 98596e15390f..9f4e42abd185 100644
@@ -466,14 +466,27 @@ 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,

tools/lib/bpf/bpf.c int bpf_obj_get(const char *pathname) · line 480

Quoted 10 lines

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

+               .target_fd = target_fd,
+               .prog_fd = prog_fd,
+               .type = type,
+               .flags = flags,
+       );
+
+       return bpf_prog_attach_xattr(&opts);
+}
+
+int bpf_prog_attach_xattr(const struct bpf_prog_attach_opts *opts)

When we discussed this whole OPTS idea, we agreed that specifying mandatory arguments as is makes for better usability. All the optional stuff then is moved into opts (and then extended indefinitely, because all the newly added stuff has to be optional, at least for some subset of arguments).

Quoted 7 lines

So if we were to follow those unofficial "guidelines", bpf_prog_attach_xattr would be defined as:

int bpf_prog_attach_xattr(int prog_fd, int target_fd, enum bpf_attach_type type,

                          const struct bpf_prog_attach_opts *opts);

, where opts has flags and replace_bpf_fd right now.

Oh, I see, I think I missed the "mandatory vs optional" part of your comment and took only the "switching to options" as the main idea, but now I see it. Sorry.

Though thinking more about it, I'm not sure it'd buy us much in this specific case. "Required" arguments are set in stone and can't be changed, but the API already has a version of function with this same list of required arguments:

LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,

                               enum bpf_attach_type type, unsigned int flags);

As a user, I'd rather use bpf_prog_attach() if I don't need optional arguments (what, also, has shorted name).

Adding another very similar one with same list of arguments + optional ones would make it so that it'd never be used in the case when no optional arguments are needed.

Yeah, I saw you comment on the flags, but flags are needed quite often (not BPF_F_REPLACE, but BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI), so I'm not sure about moving flags to optional.

The last point brings another point that such a separation, "required" / "optional", may be quite biased according to use-cases users mostly deal with and may start making less sense over time when more arguments are added to optional that are "highly recommended to use".

On the other hand if we just do one single struct argument, there won't be this problem how to separate required and optional wht both the current set of arguments and whatever is added in the future.

Quoted 4 lines

Naming wise, it's quite departure from xattr approach, so I'd probably would go with bpf_prog_attach_opts, but I won't insist.

Yeah, agree that it's not quite "xattr". I don't have strong preferences here, just used the prefix that is already used in the API. I can't rename it to bpf_prog_attach_opts though, because there is a structure with the same name :) but if there is a better name, happy to rename it.

I had an option bpf_prog_attach2 (similar to existing bpf_prog_detach2) but IMO it's worse.

Quoted 50 lines

WDYT?

Quoted 48 lines

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

 {
        union bpf_attr attr;
 
        memset(&attr, 0, sizeof(attr));
-       attr.target_fd     = target_fd;
-       attr.attach_bpf_fd = prog_fd;
-       attr.attach_type   = type;
-       attr.attach_flags  = flags;
+       attr.target_fd     = opts->target_fd;
+       attr.attach_bpf_fd = opts->prog_fd;
+       attr.attach_type   = opts->type;
+       attr.attach_flags  = opts->flags;
+       attr.replace_bpf_fd = opts->replace_prog_fd;
 
        return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr));
 }

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

index 5cfe6e0a1aef..5b5f9b374074 100644
@@ -150,8 +150,20 @@ 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 */
+       int target_fd;
+       int prog_fd;
+       enum bpf_attach_type type;
+       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(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);

tools/lib/bpf/libbpf.map +0 -0

index 495df575f87f..42b065454031 100644
@@ -210,4 +210,6 @@ LIBBPF_0.0.6 {
 } LIBBPF_0.0.5;
 

Quoted 7 lines

Quoted 7 lines

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

 LIBBPF_0.0.7 {
+       global:
+               bpf_prog_attach_xattr;
 } LIBBPF_0.0.6;
--

2.17.1

--
Andrey Ignatov

Quoted 32 lines

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

Example of program replacing:

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

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

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

Quoted 5 lines

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

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

I second Alexei's sentiment. Having this as part of test_progs would certainly be better in terms of ensuring this doesn't accidentally breaks.

[...]

tools/testing/selftests/bpf/test_cgroup_attach.c static int test_multiprog(void) · line 411

Quoted 10 lines

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

+       /* invalid input */
+
+       DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts,
+               .target_fd              = cg1,
+               .prog_fd                = allow_prog[6],
+               .replace_prog_fd        = allow_prog[0],
+               .type                   = BPF_CGROUP_INET_EGRESS,
+               .flags                  = BPF_F_ALLOW_MULTI | BPF_F_REPLACE,
+       );

This might cause compiler warnings (depending on compiler settings, of course). DECLARE_LIBBPF_OPTS does declare variable, so this is a situation of mixing code and variable declarations, which under C89 (or whatever it's named, the older standard that kernel is trying to stick to for the most part) is not allowed.

Quoted 8 lines

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

+
+       attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE;
+       if (!bpf_prog_attach_xattr(&attach_opts)) {
+               log_err("Unexpected success with OVERRIDE | REPLACE");
+               goto err;
+       }
+       assert(errno == EINVAL);
+

Andrii Nakryiko <[email protected]> [Thu, 2019-12-12 23:01 -0800]:

Quoted 39 lines · Andrey Ignatov

Quoted 37 lines

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

Example of program replacing:

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

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

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

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

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

Quoted 3 lines

I second Alexei's sentiment. Having this as part of test_progs would certainly be better in terms of ensuring this doesn't accidentally breaks.

OK, I converted both existing test_cgroup_attach and my test for BPF_F_REPLACE to test_progs and will send v3 with this change.

Quoted 14 lines

[...]

Quoted 10 lines

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

+       /* invalid input */
+
+       DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts,
+               .target_fd              = cg1,
+               .prog_fd                = allow_prog[6],
+               .replace_prog_fd        = allow_prog[0],
+               .type                   = BPF_CGROUP_INET_EGRESS,
+               .flags                  = BPF_F_ALLOW_MULTI | BPF_F_REPLACE,
+       );

Quoted 5 lines

This might cause compiler warnings (depending on compiler settings, of course). DECLARE_LIBBPF_OPTS does declare variable, so this is a situation of mixing code and variable declarations, which under C89 (or whatever it's named, the older standard that kernel is trying to stick to for the most part) is not allowed.

Yeah, I know about such a warning and expected it but didn't get it with the current setup (what surprised me btw) and decided to keep it.

The main reason I kept it is it's not actually clear how to separate declaration and initialization of opts structure when DECLARE_LIBBPF_OPTS is used since the macro does both things at once. In selftests I can just switch to direct initialization (w/o the macro) since libbpf and selftests are in sync, but for real use-cases there should be smth else (e.g. INIT_LIBBPF_OPTS macro that does only initialization of already declared struct).

Quoted 10 lines

tools/testing/selftests/bpf/test_cgroup_attach.c static int test_multiprog(void) · line 419

Quoted 10 lines

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

+
+       attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE;
+       if (!bpf_prog_attach_xattr(&attach_opts)) {
+               log_err("Unexpected success with OVERRIDE | REPLACE");
+               goto err;
+       }
+       assert(errno == EINVAL);
+

--
Andrey Ignatov

Quoted 41 lines

Andrii Nakryiko <[email protected]> [Thu, 2019-12-12 23:01 -0800]:

Quoted 39 lines · Andrey Ignatov

Quoted 37 lines

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

Example of program replacing:

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

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

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

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

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

Quoted 7 lines

Quoted 3 lines

I second Alexei's sentiment. Having this as part of test_progs would certainly be better in terms of ensuring this doesn't accidentally breaks.

OK, I converted both existing test_cgroup_attach and my test for BPF_F_REPLACE to test_progs and will send v3 with this change.

Great, thanks!

Quoted 20 lines

Quoted 19 lines

[...]

tools/testing/selftests/bpf/test_cgroup_attach.c static int test_multiprog(void) · line 411

Quoted 10 lines

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

+       /* invalid input */
+
+       DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts,
+               .target_fd              = cg1,
+               .prog_fd                = allow_prog[6],
+               .replace_prog_fd        = allow_prog[0],
+               .type                   = BPF_CGROUP_INET_EGRESS,
+               .flags                  = BPF_F_ALLOW_MULTI | BPF_F_REPLACE,
+       );

This might cause compiler warnings (depending on compiler settings, of course). DECLARE_LIBBPF_OPTS does declare variable, so this is a situation of mixing code and variable declarations, which under C89 (or whatever it's named, the older standard that kernel is trying to stick to for the most part) is not allowed.

Quoted 2 lines

Yeah, I know about such a warning and expected it but didn't get it with the current setup (what surprised me btw) and decided to keep it.

yeah, selftests compiler flags must not be as strict as kernel's, I guess?...

Quoted 7 lines

The main reason I kept it is it's not actually clear how to separate declaration and initialization of opts structure when DECLARE_LIBBPF_OPTS is used since the macro does both things at once. In selftests I can just switch to direct initialization (w/o the macro) since libbpf and selftests are in sync, but for real use-cases there should be smth else (e.g. INIT_LIBBPF_OPTS macro that does only initialization of already declared struct).

For compiler, DECLARE_LIBBPF_OPTS is purely declaration, in the same sense as struct declaration+initialization is still just declaration. If you need to postpone some of the field initialization, then you can do that by assinging that field explicitly:

DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts,
.target_fd = cgl,
);

... some code here ...
attach_opts.prog_fd = allow_prog[6];

It is just a struct, DECLARE_LIBBPF_OPTS just hides memset to 0 and setting .sz correctly, nothing more.

Quoted 11 lines

Quoted 8 lines

tools/testing/selftests/bpf/test_cgroup_attach.c static int test_multiprog(void) · line 419

Quoted 8 lines

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

+
+       attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE;
+       if (!bpf_prog_attach_xattr(&attach_opts)) {
+               log_err("Unexpected success with OVERRIDE | REPLACE");
+               goto err;
+       }
+       assert(errno == EINVAL);
+

--
Andrey Ignatov