Re: [PATCH 2/6] rv: Update monitors through a quiesce helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] rv: unconditional disable in rv_update_single() resets monitors on no-op
- [Medium] rv: partial child update in rv_update_container() and ignored error return
--

commit 5225ae9ebfb18889b8c1d478e3a3b6a70311b8ec
Author: Wei Li <liwei391@huawei.com>

rv: Update monitors through a quiesce helper

This commit introduces the rv_update_monitor() helper to quiesce, update, and
re-enable runtime verification monitors via a callback while preserving the
enabled state of container children. It updates monitor_swap_reactors() to use
this helper when changing a monitor's reactor.

> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index 29f155c6968b7..4e689d4590aee 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
> @@ -289,6 +289,19 @@ static int rv_enable_single(struct rv_monitor *mon)
>  	return retval;
>  }
>  
> +static int rv_update_single(struct rv_monitor *mon, rvmon_update_fn_t update, void *data)
> +{
> +	int retval = 0;
> +	int enabled;
> +
> +	enabled = __rv_disable_monitor(mon, true);
> +	update(mon, data);
> +	if (enabled)
> +		retval = rv_enable_single(mon);
> +
> +	return retval;
> +}
> +
>  static void rv_disable_container(struct rv_monitor *mon)
>  {
>  	struct rv_monitor *p = mon;
> @@ -321,6 +334,22 @@ static int rv_enable_container(struct rv_monitor *mon)
>  	return retval;
>  }
>  
> +static int rv_update_container(struct rv_monitor *mon, rvmon_update_fn_t update, void *data)
> +{
> +	struct rv_monitor *p = mon;
> +	int retval = 0;
> +
> +	list_for_each_entry_continue(p, &rv_monitors_list, list) {
> +		if (retval || p->parent != mon)
> +			break;
> +		retval = rv_update_single(p, update, data);
> +	}
> +	if (retval)
> +		rv_disable_container(mon);
> +	update(mon, data);
> +	return retval;
> +}

[Severity: Medium]
When re-enabling an active child monitor fails in rv_update_single() ->
rv_enable_single() -> mon->enable() (for instance, if enable_nomiss() returns
-ENOMEM when allocating storage), rv_update_container() breaks out of the
loop over rv_monitors_list immediately.

Because update(p, data) has already run for the failing child and all prior
children, breaking out of the loop skips calling update() on the remaining
children of the container, while still calling update(mon, data) on the
parent container itself after rv_disable_container(mon).

When called via monitor_reactors_write() -> monitor_swap_reactors() ->
rv_update_monitor(), doesn't this leave the container and earlier child
monitors pointing to the new reactor while later children still point to the
old reactor?

[ ... ]

> diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
> index ea52f50e64532..06f21bcdd6cf8 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -177,45 +177,25 @@ static const struct seq_operations monitor_reactors_seq_ops = {
>  	.show	= monitor_reactor_show
>  };
>  
> -static void monitor_swap_reactors_single(struct rv_monitor *mon,
> -					 struct rv_reactor *reactor,
> -					 bool nested)
> +static void monitor_update_reactor(struct rv_monitor *mon, void *data)
>  {
> -	bool monitor_enabled;
> +	struct rv_reactor *reactor = data;
>  
> -	/* nothing to do */
> -	if (mon->reactor == reactor)
> +	if (mon->reactor == data)
>  		return;
>  
> -	monitor_enabled = mon->enabled;
> -	if (monitor_enabled)
> -		rv_disable_monitor(mon);
> -
>  	mon->reactor = reactor;
>  	mon->react = reactor->react;
> -
> -	/* enable only once if iterating through a container */
> -	if (monitor_enabled && !nested)
> -		rv_enable_monitor(mon);
>  }

[Severity: Medium]
Previously, monitor_swap_reactors_single() checked mon->reactor == reactor
before calling rv_disable_monitor(mon), making a redundant reactor write a
no-op:

	if (mon->reactor == reactor)
		return;

	monitor_enabled = mon->enabled;
	if (monitor_enabled)
		rv_disable_monitor(mon);

With the move to rv_update_monitor() -> rv_update_single(),
__rv_disable_monitor(mon, true) now runs unconditionally before invoking
monitor_update_reactor().

If user space writes the already-active reactor to a monitor's reactors file
(or updates a container monitor where some enabled child monitors already use
that reactor), won't __rv_disable_monitor() still detach tracepoints, wait on
tracepoint_synchronize_unregister(), and invoke mon->disable(), resetting all
in-flight automaton verification state even when mon->reactor == data?

>  
>  static void monitor_swap_reactors(struct rv_monitor *mon, struct rv_reactor *reactor)
>  {
> -	struct rv_monitor *p = mon;
> -
> -	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);
> -		}
>  	/*
> -	 * This call enables and disables the monitor if they were active.
> -	 * In case of a container, we already disabled all and will enable all.
> -	 * All nested monitors are enabled also if they were off, we may refine
> -	 * this logic in the future.
> +	 * Update the reactor with the monitor quiesced: enabled monitors are
> +	 * disabled, updated and re-enabled, disabled ones are only updated.
> +	 * Children of a container keep their enabled state.
>  	 */
> -	monitor_swap_reactors_single(mon, reactor, false);
> +	rv_update_monitor(mon, monitor_update_reactor, reactor);
>  }

[Severity: Medium]
rv_update_monitor() returns an error code when re-enabling a monitor fails in
rv_enable_single(), but monitor_swap_reactors() ignores the return value and
monitor_reactors_write() unconditionally returns count to user space.

Should monitor_swap_reactors() propagate the error from rv_update_monitor()
back to monitor_reactors_write() so user space is notified when re-enabling
the monitor fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/CY8PR19MB70098BF493A3B67418C7B9ABB7682@CY8PR19MB7009.namprd19.prod.outlook.com?part=2