Re: [PATCH v5 3/5] rv/reactors: export rv_register_reactor() and rv_unregister_reactor()

[Gabriele Monaco]

On Mon, 2026-09-07 at 01:10 +0800, wen.yang@xxxxxxxxx wrote:
> From: Wen Yang <wen.yang@xxxxxxxxx>
> 
> rv_react() is exported to modules, but the reactor registration helpers
> are not.  Export them with EXPORT_SYMBOL_GPL() so reactor modules and
> the tristate KUnit test module can register and unregister reactors
> without hitting undefined symbol errors at link time(modpost).
> 
> Commit 3d3800b4f7f4 ("rv: Remove reactor's reference counter") noted
> that if module-based reactors are supported, try_module_get()/module_put()
> should be used. Add struct module *owner to struct rv_reactor so a module
> cat set owner = THIS_MODULE; pin the module in monitor_swap_reactors_gingle()
> and release it when a monitor detaches or is unregistered.

You needed these symbols in KUnit and we are exporting them for /potential/
future support of reactors as modules. I don't see any technical reason why we
shouldn't support this, but they are currently /not/ supported.

I know sashiko and other LLMs complain about this, and they have a point, but
you can ignore them. At most state in this commit message that this does NOT add
support for reactors as modules.

Let's focus this series on its original intent (fix a lockdep warning and add
some KUnit tests that expose a reproducer), then if adding support for reactors
as modules is so simple, you can do it in another series.

If you really want to /also/ add support for reactors as modules in this series,
you need to make that very explicit (not just a vague line in the changelog, but
rather rewrite the entire cover letter and commit message).
And mind that this would mean your series needs to go through another round of
review and serious testing: you are adding a new feature.

> In-tree reactors leave owner = NULL and are unaffected.
> 
> Reviewed-by: Gabriele Monaco <gmonaco@xxxxxxxxxx>

Please, whenever you significantly change an already reviewed patch, remove the
reviewed-by, so I can quickly see I need to review it again.

Thanks,
Gabriele

> Signed-off-by: Wen Yang <wen.yang@xxxxxxxxx>
> ---
>  include/linux/rv.h            |  3 +++
>  kernel/trace/rv/rv.c          |  5 +++++
>  kernel/trace/rv/rv_reactors.c | 38 +++++++++++++++++++++++++++++------
>  3 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/rv.h b/include/linux/rv.h
> index 541ba404926a..ff3289ba4f02 100644
> --- a/include/linux/rv.h
> +++ b/include/linux/rv.h
> @@ -128,10 +128,13 @@ union rv_task_monitor {
>  };
>  
>  #ifdef CONFIG_RV_REACTORS
> +struct module;
> +
>  struct rv_reactor {
>  	const char		*name;
>  	const char		*description;
>  	__printf(1, 0) void	(*react)(const char *msg, va_list args);
> +	struct module		*owner;
>  	struct list_head	list;
>  };
>  #endif
> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index 29f155c6968b..458b17c005b3 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
> @@ -803,6 +803,11 @@ int rv_unregister_monitor(struct rv_monitor *monitor)
>  	guard(mutex)(&rv_interface_lock);
>  
>  	rv_disable_monitor(monitor);
> +#ifdef CONFIG_RV_REACTORS
> +	if (monitor->reactor)
> +		module_put(monitor->reactor->owner);
> +
> +#endif
>  	list_del(&monitor->list);
>  	destroy_monitor_dir(monitor);
>  
> diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
> index ff7d478227c3..136eb7f47c4a 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -62,6 +62,7 @@
>   */
>  
>  #include <linux/lockdep.h>
> +#include <linux/module.h>
>  #include <linux/slab.h>
>  
>  #include "rv.h"
> @@ -159,7 +160,7 @@ static const struct seq_operations
> monitor_reactors_seq_ops = {
>  	.show	= monitor_reactor_show
>  };
>  
> -static void monitor_swap_reactors_single(struct rv_monitor *mon,
> +static int monitor_swap_reactors_single(struct rv_monitor *mon,
>  					 struct rv_reactor *reactor,
>  					 bool nested)
>  {
> @@ -167,29 +168,39 @@ static void monitor_swap_reactors_single(struct
> rv_monitor *mon,
>  
>  	/* nothing to do */
>  	if (mon->reactor == reactor)
> -		return;
> +		return 0;
> +
> +	if (reactor->owner && !try_module_get(reactor->owner))
> +		return -EBUSY;
>  
>  	monitor_enabled = mon->enabled;
>  	if (monitor_enabled)
>  		rv_disable_monitor(mon);
>  
> +	if (mon->reactor)
> +		module_put(mon->reactor->owner);
>  	mon->reactor = reactor;
>  	mon->react = reactor->react;
>  
>  	/* enable only once if iterating through a container */
>  	if (monitor_enabled && !nested)
>  		rv_enable_monitor(mon);
> +
> +	return 0;
>  }
>  
> -static void monitor_swap_reactors(struct rv_monitor *mon, struct rv_reactor
> *reactor)
> +static int monitor_swap_reactors(struct rv_monitor *mon, struct rv_reactor
> *reactor)
>  {
>  	struct rv_monitor *p = mon;
> +	int ret;
>  
>  	if (rv_is_container_monitor(mon))
>  		list_for_each_entry_continue(p, &rv_monitors_list, list) {
>  			if (p->parent != mon)
>  				break;
> -			monitor_swap_reactors_single(p, reactor, true);
> +			ret = monitor_swap_reactors_single(p, reactor, true);
> +			if (ret)
> +				return ret;
>  		}
>  	/*
>  	 * This call enables and disables the monitor if they were active.
> @@ -197,7 +208,7 @@ static void monitor_swap_reactors(struct rv_monitor *mon,
> struct rv_reactor *rea
>  	 * All nested monitors are enabled also if they were off, we may
> refine
>  	 * this logic in the future.
>  	 */
> -	monitor_swap_reactors_single(mon, reactor, false);
> +	return monitor_swap_reactors_single(mon, reactor, false);
>  }
>  
>  static ssize_t
> @@ -236,10 +247,14 @@ monitor_reactors_write(struct file *file, const char
> __user *user_buf,
>  	guard(mutex)(&rv_interface_lock);
>  
>  	list_for_each_entry(reactor, &rv_reactors_list, list) {
> +		int ret;
> +
>  		if (strcmp(ptr, reactor->name) != 0)
>  			continue;
>  
> -		monitor_swap_reactors(mon, reactor);
> +		ret = monitor_swap_reactors(mon, reactor);
> +		if (ret)
> +			return ret;
>  
>  		return count;
>  	}
> @@ -314,6 +329,7 @@ int rv_register_reactor(struct rv_reactor *reactor)
>  	guard(mutex)(&rv_interface_lock);
>  	return __rv_register_reactor(reactor);
>  }
> +EXPORT_SYMBOL_GPL(rv_register_reactor);
>  
>  /**
>   * rv_unregister_reactor - unregister a rv reactor.
> @@ -327,6 +343,7 @@ int rv_unregister_reactor(struct rv_reactor *reactor)
>  	list_del(&reactor->list);
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(rv_unregister_reactor);
>  
>  /*
>   * reacting_on interface.
> @@ -421,6 +438,15 @@ int reactor_populate_monitor(struct rv_monitor *mon,
> struct dentry *root)
>  	 * Configure as the rv_nop reactor.
>  	 */
>  	mon->reactor = get_reactor_rdef_by_name("nop");
> +	if (WARN_ON(!mon->reactor)) {
> +		rv_remove(tmp);
> +		return -EINVAL;
> +	}
> +
> +	if (mon->reactor->owner && !try_module_get(mon->reactor->owner)) {
> +		rv_remove(tmp);
> +		return -EBUSY;
> +	}
>  
>  	return 0;
>  }