Re: [RFC PATCH 14/20] tools/rv: Implement BPF monitor registration logic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Missing directory creation for BPF_PIN_BASE_PATH causes immediate pinning failure on fresh systems.

Pre-existing issues:
- [High] A single failure when resetting map elements causes all subsequent maps to be silently skipped.
--

commit 540f0c8b5ced934d1d1110f3772b2b0284013689
Author: Gabriele Monaco <gmonaco@redhat.com>

tools/rv: Implement BPF monitor registration logic
    
Add support for registering and unregistering BPF monitors from
userspace via the struct_ops mechanism. The implementation scans known
paths for BPF monitor objects, loads them, and attaches their struct_ops
maps.

> diff --git a/tools/verification/rv/src/bpf_monitor.c b/tools/verification/rv/src/bpf_monitor.c
> index e13ae35678cb8..7e24a71dfac70 100644
> --- a/tools/verification/rv/src/bpf_monitor.c
> +++ b/tools/verification/rv/src/bpf_monitor.c
[ ... ]
> @@ -679,3 +680,340 @@ int bpf_run_monitor(char *monitor_name, int argc, char **argv)
>  
>  	return retval;
>  }
> +
> +/*
> + * register_monitor_from_file - register a single BPF monitor via struct_ops
> + *
> + * Opens, loads, and attaches a BPF monitor's struct_ops map, then pins the
> + * resulting link.
> + *
> + * Returns 0 on success, -1 on error
> + */
> +static int register_monitor_from_file(const char *path, const char *name)
> +{
[ ... ]
> +	link = bpf_map__attach_struct_ops(map);
> +	if (!link) {
> +		err_msg("bpf: error attaching struct_ops for %s: %s\n",
> +			name, strerror(errno));
> +		goto cleanup;
> +	}
> +
> +	snprintf(pin_path, sizeof(pin_path), "%s/%s", BPF_PIN_BASE_PATH, map_name);
> +	res = bpf_link__pin(link, pin_path);

[Severity: High]
How does this code handle the case where the BPF_PIN_BASE_PATH directory
does not exist yet?

It appears that if the parent directory has not been manually created by the
user beforehand, bpf_link__pin() will return -ENOENT. This causes a
regression out-of-the-box on fresh systems where the registration tool
systematically fails.

Neither the kernel RV subsystem nor libbpf auto-creates this namespace
directory, which breaks the functionality of the tool for local
administrators.

[ ... ]

[Severity: High]
This is a pre-existing issue, but could the error handling in
reset_monitor_maps() inadvertently skip resetting subsequent maps on failure?

tools/verification/rv/src/bpf_monitor.c:reset_monitor_maps() {
    ...
			while (!err && bpf_map_get_next_key(fd, NULL, key) == 0)
				err |= bpf_map_delete_elem(fd, key);
    ...
			for (uint32_t idx = 0; !err && idx < max_entries; idx++)
				err |= bpf_map_update_elem(fd, &idx, zero_value, BPF_ANY);
    ...
}

If an error occurs while clearing a map (e.g., malloc failure for the key, or
bpf_map_update_elem failure), the err flag is set to true. The inner loops
for all subsequent maps strictly require !err in their conditions, meaning
they will be silently skipped without being reset.

This leaves stale state from previous runs in the maps, which could corrupt
the monitor's internal state machine when bpf_run_monitor() is called.

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