Re: [RFC PATCH 13/20] tools/rv: Implement BPF monitor loading and tracing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] A failure to reset one BPF map aborts the reset process for all subsequent maps.
- [Medium] Memory and file descriptor leak when `bpf_object__load` fails.
- [Medium] Reused BPF links are not unpinned during cleanup, leaving their BPF programs permanently attached to the kernel.
- [Low] Calling `get_state_name` twice in the same printf argument list causes both fields to print the same value if BTF lookup fails.
- [Low] `extract_enum_names` fails to strip the monitor name suffix if the monitor name contains an underscore.
- [Low] Missing bounds check on `data_sz` in `handle_event` can lead to out-of-bounds memory reads.
--

commit 18d2a63fa7007876f35f76e7406322321e0a0eb2
Author: Gabriele Monaco <gmonaco@redhat.com>

tools/rv: Implement BPF monitor loading and tracing

Implement BPF monitor loading, execution, and optional tracing
functionality.

> diff --git a/tools/verification/rv/src/bpf_monitor.c b/tools/verification/rv/src/bpf_monitor.c
> new file mode 100644
> index 0000000000000..e13ae35678cb8
> --- /dev/null
> +++ b/tools/verification/rv/src/bpf_monitor.c
[ ... ]
> +static int handle_event(void *ctx, void *data, size_t data_sz)
> +{

[Severity: Low]
Should this callback validate that data_sz is large enough to cover the
fields accessed by read_field()?

Without verifying that the BTF metadata offsets fall within the provided
data_sz, could a truncated or malformed ring buffer event lead to an
out-of-bounds memory read?

[ ... ]
> +	if (entry[FIELD_EVENT_TYPE] == RV_TRACE_ERROR) {
> +		printf("error ");
> +		if (config.has_id)
> +			printf(" %8"PRIu64"", entry[FIELD_ID]);
> +		printf(" %24s x %-24s\n",
> +		       get_state_name(mon_ctx, entry[FIELD_CURR_STATE]),
> +		       get_event_name(mon_ctx, entry[FIELD_EVENT]));
> +	} else {
> +		printf("event ");
> +		read_field(entry, FIELD_IS_FINAL, raw, mon_ctx);
> +		read_field(entry, FIELD_NEXT_STATE, raw, mon_ctx);
> +
> +		if (config.has_id)
> +			printf(" %8"PRIu64"", entry[FIELD_ID]);
> +		printf(" %24s x %-24s -> %-24s %c\n",
> +		       get_state_name(mon_ctx, entry[FIELD_CURR_STATE]),
> +		       get_event_name(mon_ctx, entry[FIELD_EVENT]),
> +		       get_state_name(mon_ctx, entry[FIELD_NEXT_STATE]),
> +		       entry[FIELD_IS_FINAL] ? 'Y' : 'N');

[Severity: Low]
Since get_state_name() returns a pointer to a single static buffer when
the state ID is not found, will calling it twice in the same printf
argument list cause both fields to print the exact same string as the
buffer is overwritten?

> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int extract_enum_names(const struct btf *btf, const char *enum_kind,
> +			       char dest[][MAX_DA_NAME_LEN], struct bpf_monitor_ctx *ctx)
> +{
[ ... ]
> +		padding = strrchr(name, '_');
> +		name_len = strlen(name);
> +		if (padding && !strcmp(ctx->monitor_name, padding + 1))
> +			name_len = (size_t)(padding - name);

[Severity: Low]
Could this fail to strip the suffix if the monitor name itself contains
an underscore?

Since strrchr() finds the last occurrence of '_', if ctx->monitor_name
is something like my_monitor, padding + 1 will evaluate to monitor,
causing the strcmp check to fail.

[ ... ]
> +static void reset_monitor_maps(struct bpf_object *obj)
> +{
> +	struct bpf_map *map;
> +	bool err = false;
> +
> +	bpf_object__for_each_map(map, obj) {
[ ... ]
> +		case BPF_MAP_TYPE_HASH:
> +		case BPF_MAP_TYPE_PERCPU_HASH: {
> +			void *key = malloc(bpf_map__key_size(map));
> +
> +			if (!key) {
> +				err = true;
> +				break;
> +			}
> +
> +			while (!err && bpf_map_get_next_key(fd, NULL, key) == 0)
> +				err |= bpf_map_delete_elem(fd, key);

[Severity: Medium]
Since the err flag is declared outside the loop and not reset to false
per map, does a failure to clear one map cause the while loop to be
skipped for all subsequent maps? Stale data might be left behind,
potentially causing incorrect monitor behavior.

[ ... ]
> +static struct bpf_object *open_bpf_monitor(const char *monitor_name, const char *path)
> +{
[ ... ]
> +	obj = bpf_object__open_file(path, &opts);
> +	if (!obj) {
> +		err_msg("bpf: error opening object file: %s\n", strerror(errno));
> +		return NULL;
> +	}
> +
> +	res = bpf_object__load(obj);
> +	if (res) {
> +		err_msg("bpf: error loading object file: %s\n", strerror(-res));
> +		return NULL;
> +	}

[Severity: Medium]
If bpf_object__load() fails, should this error path call
bpf_object__close() to prevent leaking the bpf_object and its open
file descriptors?

[ ... ]
> +static int attach_bpf_handlers(const char *monitor_name, struct bpf_object *obj,
> +				struct bpf_link **links, int *link_count)
> +{
[ ... ]
> +		if (access(pin_path, F_OK) == 0)
> +			link = bpf_link__open(pin_path);
[ ... ]
> +int bpf_run_monitor(char *monitor_name, int argc, char **argv)
> +{
[ ... ]
> +	for (int i = 0; i < link_count; i++) {
> +		bpf_link__unpin(links[i]);

[Severity: Medium]
If a link is reused and reopened via bpf_link__open(), will
bpf_link__unpin() fail during cleanup?

Libbpf does not populate the internal pin_path property for reopened
links, which means the pin file could remain on the filesystem and the
BPF program might stay attached to the kernel tracepoint indefinitely
after the tool exits.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831090524.106845-1-gmonaco@redhat.com?part=13