Re: [PATCH 8/9] rv: Add automatic cleanup handlers for per-task HA monitors

[Wen Yang <wen.yang@linux.dev>]

ha_cancel_timer_sync() is the right choice for task exit; the
ha_mon_initializing guard correctly handles the init-window race.

One issue: after ha_monitor_disable_hook(), an in-flight
ha_handle_sched_process_exit() handler may still be executing.  It
reads task_mon_slot via da_get_monitor() (&p->rv[task_mon_slot]);
da_monitor_sync_hook() = synchronize_rcu() cannot drain it because
tracepoint handlers run outside any RCU read-side section.  If
rv_put_task_monitor_slot() writes RV_PER_TASK_MONITOR_INIT to
task_mon_slot first, the handler dereferences an OOB index.

This is the same race Patch 5 closes for PER_OBJ with
tracepoint_synchronize_unregister(); the PER_TASK da_monitor_destroy()
needs the same call (and so does every other PER_TASK monitor, not only
the new exit handler).

Could you add tracepoint_synchronize_unregister() to the PER_TASK
da_monitor_destroy() ?  Alternatively, we can carry the fix on top of
your series.

--
Best wishes,
Wen


On 5/12/26 22:02, Gabriele Monaco wrote:
> Hybrid automata monitors may start timers, depending on the model, these
> may remain active on an exiting task and cause false positives or even
> access freed memory.
> 
> Add an enable/disable hook in the HA code, currently only populated by
> the per-task handler for registration and deregistration.
> This hooks to the sched_process_exit event and ensures the timer is
> stopped for every exiting task. The handler is enabled automatically but
> may be disabled, for instance if the monitor uses the event for another
> purpose (but should still manually ensure timers are stopped).
> 
> Fixes: f5587d1b6ec9 ("rv: Add Hybrid Automata monitor type")
> Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
> ---
>   include/rv/ha_monitor.h | 44 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
> 
> diff --git a/include/rv/ha_monitor.h b/include/rv/ha_monitor.h
> index 11ae85bad492..1bdf866e9c63 100644
> --- a/include/rv/ha_monitor.h
> +++ b/include/rv/ha_monitor.h
> @@ -28,6 +28,7 @@ static inline void ha_monitor_init_env(struct da_monitor *da_mon);
>   static inline void ha_monitor_reset_env(struct da_monitor *da_mon);
>   static inline void ha_setup_timer(struct ha_monitor *ha_mon);
>   static inline bool ha_cancel_timer(struct ha_monitor *ha_mon);
> +static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon);
>   static bool ha_monitor_handle_constraint(struct da_monitor *da_mon,
>   					 enum states curr_state,
>   					 enum events event,
> @@ -38,6 +39,26 @@ static bool ha_monitor_handle_constraint(struct da_monitor *da_mon,
>   #define da_monitor_reset_hook ha_monitor_reset_env
>   #define da_monitor_sync_hook() synchronize_rcu()
>   
> +#if !defined(HA_SKIP_AUTO_CLEANUP) && RV_MON_TYPE == RV_MON_PER_TASK
> +/*
> + * Automatic cleanup handlers for per-task HA monitors, only skip if you know
> + * what you are doing (e.g. you want to implement cleanup manually in another
> + * handler doing more things).
> + */
> +static void ha_handle_sched_process_exit(void *data, struct task_struct *p,
> +					 bool group_dead);
> +
> +#define ha_monitor_enable_hook()                                             \
> +	rv_attach_trace_probe(__stringify(MONITOR_NAME), sched_process_exit, \
> +			      ha_handle_sched_process_exit)
> +#define ha_monitor_disable_hook()                                            \
> +	rv_detach_trace_probe(__stringify(MONITOR_NAME), sched_process_exit, \
> +			      ha_handle_sched_process_exit)
> +#else
> +#define ha_monitor_enable_hook()
> +#define ha_monitor_disable_hook()
> +#endif
> +
>   #include <rv/da_monitor.h>
>   #include <linux/seq_buf.h>
>   
> @@ -124,12 +145,14 @@ static int ha_monitor_init(void)
>   
>   	ha_mon_initializing = true;
>   	ret = da_monitor_init();
> +	ha_monitor_enable_hook();
>   	ha_mon_initializing = false;
>   	return ret;
>   }
>   
>   static void ha_monitor_destroy(void)
>   {
> +	ha_monitor_disable_hook();
>   	da_monitor_destroy();
>   }
>   
> @@ -230,6 +253,18 @@ static inline void ha_trace_error_env(struct ha_monitor *ha_mon,
>   {
>   	CONCATENATE(trace_error_env_, MONITOR_NAME)(id, curr_state, event, env);
>   }
> +
> +#if !defined(HA_SKIP_AUTO_CLEANUP) && RV_MON_TYPE == RV_MON_PER_TASK
> +static void ha_handle_sched_process_exit(void *data, struct task_struct *p,
> +					 bool group_dead)
> +{
> +	struct da_monitor *da_mon = da_get_monitor(p);
> +
> +	if (likely(!ha_monitor_uninitialized(da_mon)))
> +		ha_cancel_timer_sync(to_ha_monitor(da_mon));
> +}
> +#endif
> +
>   #endif /* RV_MON_TYPE */
>   
>   /*
> @@ -455,6 +490,10 @@ static inline bool ha_cancel_timer(struct ha_monitor *ha_mon)
>   {
>   	return timer_delete(&ha_mon->timer);
>   }
> +static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon)
> +{
> +	timer_delete_sync(&ha_mon->timer);
> +}
>   #elif HA_TIMER_TYPE == HA_TIMER_HRTIMER
>   /*
>    * Helper functions to handle the monitor timer.
> @@ -506,6 +545,10 @@ static inline bool ha_cancel_timer(struct ha_monitor *ha_mon)
>   {
>   	return hrtimer_try_to_cancel(&ha_mon->hrtimer) == 1;
>   }
> +static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon)
> +{
> +	hrtimer_cancel(&ha_mon->hrtimer);
> +}
>   #else /* HA_TIMER_NONE */
>   /*
>    * Start function is intentionally not defined, monitors using timers must
> @@ -516,6 +559,7 @@ static inline bool ha_cancel_timer(struct ha_monitor *ha_mon)
>   {
>   	return false;
>   }
> +static inline void ha_cancel_timer_sync(struct ha_monitor *ha_mon) { }
>   #endif
>   
>   #endif