[PATCH 0/6] rv: Expose and preserve nested monitor states

[PATCH 0/6] rv: Expose and preserve nested monitor states

[Wei Li]

From: Wei Li <liwei391@huawei.com>

A container groups nested monitors. The user could not see their
divergences: the reactors file did not show when a nested monitor
uses a different reactor, and the enable file reported only 0 or 1
even when just some nested monitors were on. Switching the reactor
of a container also re-enabled every nested monitor, including the
ones the user left off.

This series makes the states visible and the updates consistent:

- print a "*" next to the selected reactor of a diverged container
- show "X" in the enable file of a partially enabled container
- update the reactor with the monitor quiesced, so every monitor
  keeps its enabled state across the swap


Wei Li (6):
  rv: Mark diverged child reactors with a star
  rv: Update monitors through a quiesce helper
  rv: Show the mixed enable state of containers
  Documentation/rv: Document the enable and reactor states
  tools/rv: Parse the mixed enable state of containers
  rv: Do not send the trailing NUL to userspace

 .../trace/rv/runtime-verification.rst         |   6 +-
 include/linux/rv.h                            |  12 ++-
 kernel/trace/rv/rv.c                          | 101 ++++++++++++++++--
 kernel/trace/rv/rv.h                          |   2 +
 kernel/trace/rv/rv_reactors.c                 |  56 +++++-----
 tools/verification/rv/src/in_kernel.c         |  32 ++++--
 tools/verification/rv/tests/rv_list.t         |   2 +-
 7 files changed, 164 insertions(+), 47 deletions(-)

-- 
2.55.0

[PATCH 1/6] rv: Mark diverged child reactors with a star

[Wei Li]

From: Wei Li <liwei391@huawei.com>

Each nested monitor carries its own reactor pointer. Writing a reactor
to a single nested monitor leaves the container view unchanged. The user
cannot see the divergence.

Print a "*" after the selected reactor of a container when a nested
monitor uses a different one.

Signed-off-by: Wei Li <liwei391@huawei.com>
---
 kernel/trace/rv/rv_reactors.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 8e57a95d446b..ea52f50e6453 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -134,6 +134,21 @@ static const struct file_operations available_reactors_ops = {
 	.release = seq_release
 };
 
+static bool monitor_reactor_is_dirty(struct rv_monitor *mon)
+{
+	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;
+			if (p->reactor != mon->reactor)
+				return true;
+		}
+
+	return false;
+}
+
 /*
  * Monitor's reactor file.
  */
@@ -143,7 +158,10 @@ static int monitor_reactor_show(struct seq_file *m, void *p)
 	struct rv_reactor *reactor = container_of(p, struct rv_reactor, list);
 
 	if (mon->reactor == reactor)
-		seq_printf(m, "[%s]\n", reactor->name);
+		if (monitor_reactor_is_dirty(mon))
+			seq_printf(m, "[%s]*\n", reactor->name);
+		else
+			seq_printf(m, "[%s]\n", reactor->name);
 	else
 		seq_printf(m, "%s\n", reactor->name);
 	return 0;
-- 
2.55.0

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

[Wei Li]

From: Wei Li <liwei391@huawei.com>

The reactor swap inlines the disable-update-re-enable pattern in
monitor_swap_reactors_single(). The nested flag splits one helper into
two behaviors. The container path re-enables every nested monitor,
including the ones the user left off. This abnormal logic seems to be a
temporary strategy from the comments.

Add rv_update_monitor() helper with callback for generic update
processing. A monitor is updated with it quiesced and keeps its enabled
state, including the children of a container.

Signed-off-by: Wei Li <liwei391@huawei.com>
---
 kernel/trace/rv/rv.c          | 49 +++++++++++++++++++++++++++++++++++
 kernel/trace/rv/rv.h          |  2 ++
 kernel/trace/rv/rv_reactors.c | 34 +++++-------------------
 3 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index 29f155c6968b..4e689d4590ae 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;
+}
+
 /**
  * rv_disable_monitor - disable a given runtime monitor
  * @mon: Pointer to the monitor definition structure.
@@ -355,6 +384,26 @@ int rv_enable_monitor(struct rv_monitor *mon)
 	return retval;
 }
 
+/**
+ * rv_update_monitor - update and reenable a given runtime monitor
+ * @mon: Pointer to the monitor definition structure.
+ * @update: Pointer to the monitor update callback.
+ * @data: Context data passed to the update().
+ *
+ * Returns 0 on success, error otherwise.
+ */
+int rv_update_monitor(struct rv_monitor *mon, rvmon_update_fn_t update, void *data)
+{
+	int retval;
+
+	if (rv_is_container_monitor(mon))
+		retval = rv_update_container(mon, update, data);
+	else
+		retval = rv_update_single(mon, update, data);
+
+	return retval;
+}
+
 /*
  * interface for enabling/disabling a monitor.
  */
diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
index 2c0f51ff9d5c..d0e89b8a66ac 100644
--- a/kernel/trace/rv/rv.h
+++ b/kernel/trace/rv/rv.h
@@ -26,8 +26,10 @@ extern struct mutex rv_interface_lock;
 extern struct list_head rv_monitors_list;
 
 struct dentry *get_monitors_root(void);
+typedef void (rvmon_update_fn_t)(struct rv_monitor *mon, void *data);
 int rv_disable_monitor(struct rv_monitor *mon);
 int rv_enable_monitor(struct rv_monitor *mon);
+int rv_update_monitor(struct rv_monitor *mon, rvmon_update_fn_t update, void *data);
 bool rv_is_container_monitor(struct rv_monitor *mon);
 bool rv_is_nested_monitor(struct rv_monitor *mon);
 
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index ea52f50e6453..06f21bcdd6cf 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);
 }
 
 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);
 }
 
 static ssize_t
-- 
2.55.0

[PATCH 3/6] rv: Show the mixed enable state of containers

[Wei Li]

From: Wei Li <liwei391@huawei.com>

The enable file of a container reports 0 or 1 only currently. A
partially enabled container is indistinguishable from a fully enabled
one. Tracefs draws this distinction with the "X" value in
system_enable_read().

Turn the enabled field into enum rv_monitor_state.  Refresh the
container state when a nested monitor is enabled or disabled.

Signed-off-by: Wei Li <liwei391@huawei.com>
---
 include/linux/rv.h   | 12 ++++++++++-
 kernel/trace/rv/rv.c | 50 +++++++++++++++++++++++++++++++++++++-------
 2 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/include/linux/rv.h b/include/linux/rv.h
index 541ba404926a..37ca94d3c4a4 100644
--- a/include/linux/rv.h
+++ b/include/linux/rv.h
@@ -136,10 +136,20 @@ struct rv_reactor {
 };
 #endif
 
+/*
+ * Monitor state. A plain monitor is either disabled or enabled. A container
+ * monitor tracks its children, may be mixed if only some are.
+ */
+enum rv_monitor_state {
+	RV_MON_DISABLED		= 0,
+	RV_MON_ENABLED		= 1,
+	RV_MON_MIXED		= 2,
+};
+
 struct rv_monitor {
 	const char		*name;
 	const char		*description;
-	bool			enabled;
+	enum rv_monitor_state	enabled;
 	int			(*enable)(void);
 	void			(*disable)(void);
 	void			(*reset)(void);
diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index 4e689d4590ae..7846e4092b9c 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -235,12 +235,15 @@ bool rv_is_container_monitor(struct rv_monitor *mon)
 static ssize_t monitor_enable_read_data(struct file *filp, char __user *user_buf, size_t count,
 					loff_t *ppos)
 {
+	const char set_to_char[3] = { '0', '1', 'X' };
 	struct rv_monitor *mon = filp->private_data;
-	const char *buff;
+	char buff[2];
 
-	buff = mon->enabled ? "1\n" : "0\n";
+	guard(mutex)(&rv_interface_lock);
+	buff[0] = set_to_char[mon->enabled];
+	buff[1] = '\n';
 
-	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff)+1);
+	return simple_read_from_buffer(user_buf, count, ppos, buff, 2);
 }
 
 /*
@@ -269,10 +272,22 @@ static int __rv_disable_monitor(struct rv_monitor *mon, bool sync)
 
 static void rv_disable_single(struct rv_monitor *mon)
 {
+	struct rv_monitor *cntr = mon->parent;
+	struct rv_monitor *p = cntr;
+
 	__rv_disable_monitor(mon, true);
+	if (!cntr)
+		return;
+	cntr->enabled = 0;
+	list_for_each_entry_continue(p, &rv_monitors_list, list) {
+		if (p->parent != cntr)
+			break;
+		if (p->enabled)
+			cntr->enabled = 2;
+	}
 }
 
-static int rv_enable_single(struct rv_monitor *mon)
+static int __rv_enable_monitor(struct rv_monitor *mon)
 {
 	int retval;
 
@@ -289,6 +304,27 @@ static int rv_enable_single(struct rv_monitor *mon)
 	return retval;
 }
 
+static int rv_enable_single(struct rv_monitor *mon)
+{
+	struct rv_monitor *cntr = mon->parent;
+	struct rv_monitor *p = cntr;
+	int retval;
+
+	retval = __rv_enable_monitor(mon);
+
+	if (!retval && cntr) {
+		cntr->enabled = 1;
+		list_for_each_entry_continue(p, &rv_monitors_list, list) {
+			if (p->parent != cntr)
+				break;
+			if (!p->enabled)
+				cntr->enabled = 2;
+		}
+	}
+
+	return retval;
+}
+
 static int rv_update_single(struct rv_monitor *mon, rvmon_update_fn_t update, void *data)
 {
 	int retval = 0;
@@ -297,7 +333,7 @@ static int rv_update_single(struct rv_monitor *mon, rvmon_update_fn_t update, vo
 	enabled = __rv_disable_monitor(mon, true);
 	update(mon, data);
 	if (enabled)
-		retval = rv_enable_single(mon);
+		retval = __rv_enable_monitor(mon);
 
 	return retval;
 }
@@ -325,7 +361,7 @@ static int rv_enable_container(struct rv_monitor *mon)
 	list_for_each_entry_continue(p, &rv_monitors_list, list) {
 		if (retval || p->parent != mon)
 			break;
-		retval = rv_enable_single(p);
+		retval = __rv_enable_monitor(p);
 	}
 	if (retval)
 		rv_disable_container(mon);
@@ -533,7 +569,7 @@ static void *enabled_monitors_next(struct seq_file *m, void *p, loff_t *pos)
 	(*pos)++;
 
 	list_for_each_entry_continue(mon, &rv_monitors_list, list) {
-		if (mon->enabled)
+		if (mon->enabled == RV_MON_ENABLED)
 			return &mon->list;
 	}
 
-- 
2.55.0

[PATCH 4/6] Documentation/rv: Document the enable and reactor states

[Wei Li]

From: Wei Li <liwei391@huawei.com>

The "X" value of the enable file and the "*" marker of the reactors file
are undocumented. Document both in the monitors section.

Signed-off-by: Wei Li <liwei391@huawei.com>
---
 Documentation/trace/rv/runtime-verification.rst | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/trace/rv/runtime-verification.rst b/Documentation/trace/rv/runtime-verification.rst
index c700dde9259c..0d9edb687836 100644
--- a/Documentation/trace/rv/runtime-verification.rst
+++ b/Documentation/trace/rv/runtime-verification.rst
@@ -210,12 +210,16 @@ For example::
 
 - Writing "0" disables the *MONITOR*
 - Writing "1" enables the *MONITOR*
-- Reading return the current status of the *MONITOR*
+- Reading returns "0" if the *MONITOR* is disabled, and "1" if it is
+  enabled. For a container, "X" is returned when only some of its
+  nested monitors are enabled.
 
 **monitors/MONITOR/reactors**
 
 - List available reactors, with the select reaction for the given *MONITOR*
   inside "[]". The default one is the nop (no operation) reactor.
+- A "*" after the selected reactor of a container indicates that some
+  nested monitors use a different reactor.
 - Writing the name of a reactor enables it to the given MONITOR.
 
 For example::
-- 
2.55.0

[PATCH 5/6] tools/rv: Parse the mixed enable state of containers

[Wei Li]

From: Wei Li <liwei391@huawei.com>

A container reports "X" in its enable file when its children have mixed
states. The rv tool fails to list or start such a container.

__ikm_read_enable() cannot parse "X" now and returns -1. 'rv list'
skips the container line and reports "error reading". 'rv mon' takes -1
as a true value and reports a spurious "already enabled" info.

Add support of parsing the mixed enable state of containers. Show a
mixed container as [MIX] in 'rv list'.

Signed-off-by: Wei Li <liwei391@huawei.com>
---
 tools/verification/rv/src/in_kernel.c | 32 +++++++++++++++++++++------
 tools/verification/rv/tests/rv_list.t |  2 +-
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/tools/verification/rv/src/in_kernel.c b/tools/verification/rv/src/in_kernel.c
index e6dea4040f8f..0f2e9f1eca51 100644
--- a/tools/verification/rv/src/in_kernel.c
+++ b/tools/verification/rv/src/in_kernel.c
@@ -29,21 +29,38 @@ static char *config_reactor;
  *
  * __does not log errors.
  *
- * Returns the current status, or -1 if the monitor does not exist,
- * __hence not logging errors.
+ * Returns 0 if disabled, 1 if enabled, 2 if a container with children
+ * in mixed states, or -1 if the monitor does not exist, __hence not
+ * logging errors.
  */
 static int __ikm_read_enable(char *monitor_name)
 {
 	char path[MAX_PATH];
-	long long enabled;
-	int retval;
+	char *buffer;
+	int enabled;
 
 	snprintf(path, MAX_PATH, "rv/monitors/%s/enable", monitor_name);
 
-	retval = tracefs_instance_file_read_number(NULL, path, &enabled);
-	if (retval < 0)
+	buffer = tracefs_instance_file_read(NULL, path, NULL);
+	if (!buffer)
 		return -1;
 
+	switch (buffer[0]) {
+	case '0':
+		enabled = 0;
+		break;
+	case '1':
+		enabled = 1;
+		break;
+	case 'X':
+		enabled = 2;
+		break;
+	default:
+		enabled = -1;
+	}
+
+	free(buffer);
+
 	return enabled;
 }
 
@@ -370,7 +387,8 @@ int ikm_list_monitors(char *container)
 			list_monitor = 1;
 			printf("%s%-*s %s %s\n", indent ? " - " : "",
 			       indent ? MAX_DA_NAME_LEN - 3 : MAX_DA_NAME_LEN,
-			       ikm.name, ikm.desc, ikm.enabled ? "[ON]" : "[OFF]");
+			       ikm.name, ikm.desc,
+			       ikm.enabled == 2 ? "[MIX]" : ikm.enabled ? "[ON]" : "[OFF]");
 		}
 		curr = ++next;
 
diff --git a/tools/verification/rv/tests/rv_list.t b/tools/verification/rv/tests/rv_list.t
index 201af33a52cc..f63697a903c1 100644
--- a/tools/verification/rv/tests/rv_list.t
+++ b/tools/verification/rv/tests/rv_list.t
@@ -17,7 +17,7 @@ check "verify list subcommand help" \
 all_nested=$(grep : $RVDIR/available_monitors | cut -d: -f2 | paste -s | sed 's/\t/\\|/g')
 all_non_nested=$(grep -v : $RVDIR/available_monitors | cut -d: -f2 | paste -s | sed 's/\t/\\|/g')
 sched_monitors=$(grep sched: $RVDIR/available_monitors | cut -d: -f2 | paste -s | sed 's/\t/\\|/g')
-description_state="[[:space:]]\+[[:print:]]\+\[\(OFF\|ON\)\]"
+description_state="[[:space:]]\+[[:print:]]\+\[\(OFF\|ON\|MIX\)\]"
 line_nested=" - \($all_nested\)${description_state}"
 line_non_nested="\($all_non_nested\)${description_state}"
 
-- 
2.55.0

[PATCH 6/6] rv: Do not send the trailing NUL to userspace

[Wei Li]

From: Wei Li <liwei391@huawei.com>

In the rv module, there are three read handlers that pass
'strlen(buff) + 1' to simple_read_from_buffer(). The terminator byte is
not data, so the read returns one extra byte. Pass strlen(buff)
instead.

Signed-off-by: Wei Li <liwei391@huawei.com>
---
 kernel/trace/rv/rv.c          | 4 ++--
 kernel/trace/rv/rv_reactors.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index 7846e4092b9c..835d53978961 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -483,7 +483,7 @@ static ssize_t monitor_desc_read_data(struct file *filp, char __user *user_buf,
 
 	snprintf(buff, sizeof(buff), "%s\n", mon->description);
 
-	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff) + 1);
+	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff));
 }
 
 static const struct file_operations interface_desc_fops = {
@@ -752,7 +752,7 @@ static ssize_t monitoring_on_read_data(struct file *filp, char __user *user_buf,
 
 	buff = rv_monitoring_on() ? "1\n" : "0\n";
 
-	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff) + 1);
+	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff));
 }
 
 static void turn_monitoring_off(void)
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 06f21bcdd6cf..8a4f0740e3ab 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -353,7 +353,7 @@ static ssize_t reacting_on_read_data(struct file *filp,
 
 	buff = rv_reacting_on() ? "1\n" : "0\n";
 
-	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff)+1);
+	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff));
 }
 
 static void turn_reacting_off(void)
-- 
2.55.0

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

[sashiko-bot]

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

Re: [PATCH 3/6] rv: Show the mixed enable state of containers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rv: stale parent container state on re-enable error in rv_update_single()
--

commit 2db3de084bc5c2af773f028e5905ec5c314ba488
Author: Wei Li <liwei391@huawei.com>

rv: Show the mixed enable state of containers

This commit converts struct rv_monitor::enabled into enum rv_monitor_state
so container monitors can report a mixed ("X") enable state when only a
subset of their nested monitors are enabled. It updates rv_enable_single()
and rv_disable_single() to refresh the parent container state.

> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index 4e689d4590aee..7846e4092b9c4 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
[ ... ]
> @@ -269,10 +272,22 @@ static int __rv_disable_monitor(struct rv_monitor *mon, bool sync)
>  
>  static void rv_disable_single(struct rv_monitor *mon)
>  {
> +	struct rv_monitor *cntr = mon->parent;
> +	struct rv_monitor *p = cntr;
> +
>  	__rv_disable_monitor(mon, true);
> +	if (!cntr)
> +		return;
> +	cntr->enabled = 0;
> +	list_for_each_entry_continue(p, &rv_monitors_list, list) {
> +		if (p->parent != cntr)
> +			break;
> +		if (p->enabled)
> +			cntr->enabled = 2;
> +	}
>  }
>  
> -static int rv_enable_single(struct rv_monitor *mon)
> +static int __rv_enable_monitor(struct rv_monitor *mon)
>  {
>  	int retval;
>  
> @@ -289,6 +304,27 @@ static int rv_enable_single(struct rv_monitor *mon)
>  	return retval;
>  }
>  
> +static int rv_enable_single(struct rv_monitor *mon)
> +{
> +	struct rv_monitor *cntr = mon->parent;
> +	struct rv_monitor *p = cntr;
> +	int retval;
> +
> +	retval = __rv_enable_monitor(mon);
> +
> +	if (!retval && cntr) {
> +		cntr->enabled = 1;
> +		list_for_each_entry_continue(p, &rv_monitors_list, list) {
> +			if (p->parent != cntr)
> +				break;
> +			if (!p->enabled)
> +				cntr->enabled = 2;
> +		}
> +	}
> +
> +	return retval;
> +}
> +
>  static int rv_update_single(struct rv_monitor *mon, rvmon_update_fn_t update, void *data)
>  {
>  	int retval = 0;
> @@ -297,7 +333,7 @@ static int rv_update_single(struct rv_monitor *mon, rvmon_update_fn_t update, vo
>  	enabled = __rv_disable_monitor(mon, true);
>  	update(mon, data);
>  	if (enabled)
> -		retval = rv_enable_single(mon);
> +		retval = __rv_enable_monitor(mon);

[Severity: Medium]
Can this leave mon->parent->enabled stale if re-enabling a nested monitor
fails in rv_update_single()?

When updating a single nested monitor (for example via
monitor_reactors_write() -> monitor_swap_reactors() -> rv_update_monitor() ->
rv_update_single()), __rv_disable_monitor() sets mon->enabled = 0 without
updating mon->parent->enabled.

If the subsequent __rv_enable_monitor(mon) call fails (such as
enable_nomiss() failing in init_storage(false)), mon remains disabled while
mon->parent->enabled still holds its previous enabled or mixed state.

Also, this isn't a bug, but would it be cleaner to use a shared helper with
the enum rv_monitor_state constants instead of open-coding the parent state
loops with raw 0, 1, and 2 literals in rv_disable_single() and
rv_enable_single()?

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

Re: [PATCH 5/6] tools/rv: Parse the mixed enable state of containers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tools/rv: ikm_run_monitor() still treats mixed-state container as enabled
--

commit 46a912d6c838892960e060f3a3369c5f213319f7
Author: Wei Li <liwei391@huawei.com>

tools/rv: Parse the mixed enable state of containers

This commit updates __ikm_read_enable() to parse the "X" mixed enable state
of containers as 2 and displays such containers as [MIX] in rv list.

> diff --git a/tools/verification/rv/src/in_kernel.c b/tools/verification/rv/src/in_kernel.c
> index e6dea4040f8f4..0f2e9f1eca51f 100644
> --- a/tools/verification/rv/src/in_kernel.c
> +++ b/tools/verification/rv/src/in_kernel.c
> @@ -29,21 +29,38 @@ static char *config_reactor;
>   *
>   * __does not log errors.
>   *
> - * Returns the current status, or -1 if the monitor does not exist,
> - * __hence not logging errors.
> + * Returns 0 if disabled, 1 if enabled, 2 if a container with children
> + * in mixed states, or -1 if the monitor does not exist, __hence not
> + * logging errors.
>   */
>  static int __ikm_read_enable(char *monitor_name)
>  {
>  	char path[MAX_PATH];
> -	long long enabled;
> -	int retval;
> +	char *buffer;
> +	int enabled;
>  
>  	snprintf(path, MAX_PATH, "rv/monitors/%s/enable", monitor_name);
>  
> -	retval = tracefs_instance_file_read_number(NULL, path, &enabled);
> -	if (retval < 0)
> +	buffer = tracefs_instance_file_read(NULL, path, NULL);
> +	if (!buffer)
>  		return -1;
>  
> +	switch (buffer[0]) {
> +	case '0':
> +		enabled = 0;
> +		break;
> +	case '1':
> +		enabled = 1;
> +		break;
> +	case 'X':
> +		enabled = 2;
> +		break;
> +	default:
> +		enabled = -1;
> +	}
> +
> +	free(buffer);
> +
>  	return enabled;
>  }

[Severity: Medium]
The commit message notes that rv fails to start a container in a mixed state
because rv mon takes the return value of __ikm_read_enable() as a true value
and reports a spurious "already enabled" message.

However, in tools/verification/rv/src/in_kernel.c, ikm_run_monitor() still
checks for any non-zero return value from __ikm_read_enable():

	retval = __ikm_read_enable(full_name);
	if (retval) {
		err_msg("ikm: monitor %s (in-kernel) is already enabled\n", nested_name);
		return -1;
	}

When a container has children in mixed states, __ikm_read_enable() now
returns 2, which is still non-zero (as is -1 on read errors). Won't
ikm_run_monitor() still treat a mixed-state container as already enabled and
refuse to start it?

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