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