Re: [PATCH v2 12/15] tools/rv: Add reactors support to BPF monitors

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 9098 bytes --]

> tools/rv: Add reactors support to BPF monitors

TL;DR:
- bpf_reactors install dir variable clashes, so make install may fail or
  misplace reactors
- void global bpf_rv_react() may break monitor loading on older kernels
- bpf_linker__new_fd() may break builds on libbpf before 1.6
- unused DESCRIPTION nit

> diff --git a/tools/verification/rv/Makefile b/tools/verification/rv/Makefile
> index bb81593acd71..44b0e3af1488 100644
> --- a/tools/verification/rv/Makefile
> +++ b/tools/verification/rv/Makefile
> @@ -73,8 +73,11 @@ static: $(RV_IN)
>  	$(QUIET_LINK)$(CC) $(LDFLAGS) -o $(RV)-static $(RV_IN) $(EXTLIBS)
>
>  BPF_DIR		:= bpf_monitors
> +BPF_REACT_DIR   := bpf_reactors
>  BPF_SOURCES	:= $(wildcard $(BPF_DIR)/*.c)

[ ... ]

> diff --git a/tools/verification/rv/Makefile.rv b/tools/verification/rv/Makefile.rv
> index 418748bc3aa1..3ac329851914 100644
> --- a/tools/verification/rv/Makefile.rv
> +++ b/tools/verification/rv/Makefile.rv
> @@ -34,6 +34,7 @@ MKDIR		:= mkdir
>  STRIP		:= strip
>  BINDIR		:= /usr/bin
>  BPF_MON_DIR	:= /usr/share/rv/bpf_monitors
> +BPF_REACT_DIR	:= /usr/share/rv/bpf_reactors
>
>  .PHONY: install
>  install: doc_install
> @@ -44,6 +45,10 @@ ifneq ($(wildcard bpf_monitors/*.o),)
>  	$(Q)$(MKDIR) -p $(DESTDIR)$(BPF_MON_DIR)
>  	$(call QUIET_INSTALL,bpf_monitors)$(INSTALL) bpf_monitors/*.o -m 644 $(DESTDIR)$(BPF_MON_DIR)
>  endif
> +ifneq ($(wildcard bpf_reactors/*.o),)
> +	$(Q)$(MKDIR) -p $(DESTDIR)$(BPF_REACT_DIR)
> +	$(call QUIET_INSTALL,bpf_reactors)$(INSTALL) bpf_reactors/*.o -m 644 $(DESTDIR)$(BPF_REACT_DIR)
> +endif

Is BPF_REACT_DIR defined twice with different meanings?  Makefile.rv sets it
to the install destination (/usr/share/rv/bpf_reactors), but Makefile
includes Makefile.rv first and then redefines it as the source directory:

  include Makefile.rv
  ...
  BPF_REACT_DIR   := bpf_reactors

make expands recipe lines after the whole makefile has been parsed, so the
install recipe sees the last value, "bpf_reactors".  The monitors are not
affected because they use two different names (BPF_DIR for the source,
BPF_MON_DIR for the destination).

Once the reactors are built (the default whenever clang and bpftool are
found), the wildcard check is true and make install runs:

  mkdir -p bpf_reactors
  install bpf_reactors/*.o -m 644 bpf_reactors

With an empty DESTDIR this copies each file onto itself, which GNU install
rejects with "are the same file", so make install fails.  With
DESTDIR=/tmp/pkg the reactors land in /tmp/pkgbpf_reactors instead of
/tmp/pkg/usr/share/rv/bpf_reactors.  Either way the installed rv cannot find
reactors under the /usr/share/rv base path that bpf_monitor.c searches.

Would renaming one of the variables (for example BPF_REACT_INST_DIR,
matching the BPF_DIR/BPF_MON_DIR split) fix this?  I didn't see a later patch
in this series that changes Makefile or Makefile.rv.

> diff --git a/tools/verification/rv/bpf_monitors/da_monitor_bpf.h b/tools/verification/rv/bpf_monitors/da_monitor_bpf.h
> index 12f0f78b9c9d..a37d11fef6f1 100644
> --- a/tools/verification/rv/bpf_monitors/da_monitor_bpf.h
> +++ b/tools/verification/rv/bpf_monitors/da_monitor_bpf.h

[ ... ]

> @@ -75,8 +74,15 @@ static const struct rv_trace_entry __used *_btf_trace;
>
>  static inline void da_monitor_reset(struct da_monitor *da_mon);
>
> -/* BPF monitors do not use reactors */
> -static inline void react(enum states curr_state, enum events event) { }
> +__noinline __weak void bpf_rv_react(char *msg) { }
> +
> +#define rv_react(mon, fmt, ...)							  \
> +	({									  \
> +		char ___msg[256];						  \
> +										  \
> +		if (BPF_SNPRINTF(___msg, sizeof(___msg), fmt, ##__VA_ARGS__) > 0) \
> +			bpf_rv_react(___msg);					  \
> +	})

Can this break loading of every BPF monitor on older kernels?

bpf_rv_react() is a non-static (weak) function, so clang gives it
BTF_FUNC_GLOBAL linkage and the verifier checks it as a global subprog.
Every BPF monitor now calls it on the da_event() -> react() path, even with
no reactor or with -r nop.

Global subprogs returning void have only been accepted since commit
8446ded1e1a0 ("bpf: Allow void global functions in the verifier").  Before
that commit, btf_prepare_func_args() rejects them with:

  Global function bpf_rv_react() doesn't return scalar. Only those are
  supported.

So after this patch, monitors that only use long-standing attach points
(nohz, for example) fail to load on kernels older than that commit.  Before
this patch they had no global subprog and did not have that requirement.

Would declaring the hook as "int bpf_rv_react(char *msg)", returning 0 in
the weak default and in the printk and panic reactors, keep the monitors
loadable on those kernels?

> diff --git a/tools/verification/rv/bpf_reactors/panic.c b/tools/verification/rv/bpf_reactors/panic.c
> new file mode 100644
> index 000000000000..775f91a300f0
> --- /dev/null
> +++ b/tools/verification/rv/bpf_reactors/panic.c

[ ... ]

> +char LICENSE[] SEC("license") = "GPL";
> +static char DESCRIPTION[] SEC(".rodata.description") =
> +	"panic the system if an exception is found.";
> diff --git a/tools/verification/rv/bpf_reactors/printk.c b/tools/verification/rv/bpf_reactors/printk.c
> new file mode 100644
> index 000000000000..580410d2a1fc
> --- /dev/null
> +++ b/tools/verification/rv/bpf_reactors/printk.c

[ ... ]

> +char LICENSE[] SEC("license") = "GPL";
> +static char DESCRIPTION[] SEC(".rodata.description") =
> +	"prints the exception msg to the trace buffer.";

Is the static DESCRIPTION in these two reactors ever used?

The monitors define DESCRIPTION as a non-static global (nohz.c, tqueue.c),
but here it has internal linkage and no "used" attribute.  That triggers
-Wunused-variable under the -Wall in BPF_CFLAGS, and the optimizer can drop
it at -O2, so the reactor objects may end up with no .rodata.description
section.

Nothing in the series reads a reactor description either, since
list_reactor_action() in bpf_monitor.c prints only the name.  Should the
variable be dropped, or made non-static like the monitors' if reactor
descriptions are meant to be listed?

> diff --git a/tools/verification/rv/src/bpf_monitor.c b/tools/verification/rv/src/bpf_monitor.c
> index d43ee75d3904..35bd1f40a109 100644
> --- a/tools/verification/rv/src/bpf_monitor.c
> +++ b/tools/verification/rv/src/bpf_monitor.c

[ ... ]

> @@ -795,6 +818,65 @@ static int attach_bpf_handlers(const char *monitor_name, struct bpf_object *obj,
>  	return enable_mon_fd;
>  }
>
> +static int find_bpf_reactor(const char *reactor_name, char *path_out, size_t path_len)
> +{
> +	return find_bpf_file("bpf_reactors", reactor_name, path_out, path_len);
> +}
> +
> +/*
> + * link_bpf_reactor - link the reactor function to the monitor
> + *
> + * Reactors are objects defining the BPF_REACTOR function, link that over the
> + * weak definition present in the monitor and return a file descriptor to the
> + * final linked object in memory.
> + *
> + * Returns memfd of final object on success, -1 on error.
> + */
> +static int link_bpf_reactor(const char *monitor_path, const char *reactor_path)
> +{
> +	struct bpf_linker *linker = NULL;
> +	int memfd = -1;
> +	int err = 0;
> +
> +	memfd = memfd_create("linked_bpf", 0);
> +	if (memfd < 0) {
> +		err_msg("bpf: failed to create memfd: %s\n", strerror(errno));
> +		return -1;
> +	}
> +
> +	linker = bpf_linker__new_fd(memfd, NULL);

Does this raise the minimum libbpf version needed to build rv?

bpf_linker__new_fd() was added in libbpf 1.6 (it is in the LIBBPF_1.6.0
block of libbpf.map).  rv builds against the system libbpf through
pkg-config, and the only libbpf gate is the feature test, which only
requires LIBBPF_MAJOR_VERSION >= 1:

    #if !defined(LIBBPF_MAJOR_VERSION) || (LIBBPF_MAJOR_VERSION < 1)
    #error At least libbpf 1.0 is required for Linux tools.

Before this patch, the newest libbpf API used by bpf_monitor.c was
bpf_map_get_info_by_fd() (LIBBPF_1.2.0).  With libbpf 1.2 to 1.5 installed
(for example Ubuntu 24.04 ships 1.3, RHEL 9 ships 1.3/1.4, Debian 13 and
Fedora 42 ship 1.5), feature-libbpf still passes, so HAVE_LIBBPF is set and
bpf_monitor.c is compiled.

The build then fails: GCC 14 and later reject the implicit declaration of
bpf_linker__new_fd(), and older compilers stop at link time with an
undefined reference.  On those systems 'make -C tools/verification/rv' used
to build rv with BPF monitor support and now fails outright, instead of
falling back to the "building without BPF monitor support" path.

Could this use bpf_linker__new() (LIBBPF_0.4.0) on a path such as
"/proc/self/fd/<memfd>" or a temporary file, or add a libbpf >= 1.6 version
check to Makefile.config, the way libtraceevent and libtracefs are already
handled there?  I didn't see a later patch in this series that touches
tools/verification/rv/src/, the rv Makefiles or tools/build/feature/.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36944413541