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 <[email protected]> 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/[email protected]?part=14
