Re: [PATCH v2 10/15] tools/rv: Add BPF monitors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: PER_TASK map leak via premature sched_process_exit cleanup
--

commit 9e1db08be07757261ff0e098cd3dda05c445d860
Author: Gabriele Monaco <gmonaco@redhat.com>

tools/rv: Add BPF monitors

This commit adds the code for two initial BPF monitors (queue and nohz) and
adapts the in-kernel da_monitor.h to share code and conditionally compile BPF
or kernel-specific components. It also integrates BPF monitor building into
the main rv Makefile.

> diff --git a/tools/verification/rv/bpf_monitors/da_monitor_bpf.h b/tools/verification/rv/bpf_monitors/da_monitor_bpf.h
> new file mode 100644
> index 0000000000000..12f0f78b9c9d3
> --- /dev/null
> +++ b/tools/verification/rv/bpf_monitors/da_monitor_bpf.h
[ ... ]
> +#if RV_MON_TYPE == RV_MON_PER_TASK
> +/*
> + * BPF Per-Task monitor - uses BPF hash map indexed by PID
> + *
> + * Just a special case of per-object monitor with API consistent with in-kernel
> + * monitors (no need to pass the id).
> + */
> +
> +#define da_get_monitor(tsk) da_get_monitor_bpf(BPF_CORE_READ(tsk, pid), tsk)
> +#define da_handle_event(tsk, event) \
> +	da_handle_event_bpf(BPF_CORE_READ(tsk, pid), tsk, event)
> +#define da_handle_start_event(tsk, event) \
> +	da_handle_start_event_bpf(BPF_CORE_READ(tsk, pid), tsk, event)
> +#define da_handle_start_run_event(tsk, event) \
> +	da_handle_start_run_event_bpf(BPF_CORE_READ(tsk, pid), tsk, event)
> +
> +typedef struct task_struct *monitor_target_bpf;
> +static inline void da_destroy_storage(da_id_type id);
> +
> +SEC("tp_btf/sched_process_exit")
> +int BPF_PROG(handle_obj_cleanup, struct task_struct *p, bool group_dead)
> +{
> +	da_destroy_storage(p->pid);
> +	return 0;
> +}

[Severity: Medium]
Could this lead to a map leak if trace events occur during task teardown after
sched_process_exit?

If an active PER_TASK monitor traces an event that occurs in do_exit() after
sched_process_exit has already executed (for example, sched_dequeue, page
faults, or other events in exit_mm() or schedule()), the event handler will
call da_handle_start_event_bpf(). This function recreates the map element via
da_create_storage().

Since handle_obj_cleanup has already run for this task, the newly recreated
map element will never be deleted. Over time, this could exhaust the BPF map,
and a future task reusing the same PID could inherit this stale automaton
state.

Would it be safer to attach this cleanup routine to sched_process_free
instead, ensuring the storage is only destroyed when the task struct is
actually being freed?

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