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

[Gabriele Monaco <gmonaco@redhat.com>]

On Sun, 2026-05-17 at 17:40 +0800, Wen Yang wrote:
> 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.

Yeah you're right, that's the neatest way to solve it.

Indeed, any in-flight handler would do da_get_monitor() not only the newly added
exit hook, so we're going to need tracepoint_synchronize_unregister() before
touching task_mon_slot in any per-task monitor (not only HA, also DA and even
LTL).

Feel free to send your patch and I'll apply it to this series, including also
ltl_monitor_destroy().

Thanks,
Gabriele

> 
> --
> 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