Re: [PATCH] rv: Fix wrong type cast in enabled_monitors_next()

[Nam Cao <namcao@linutronix.de>]

Hi Nathan,

Nathan Chancellor <nathan@kernel.org> writes:
> I am seeing a crash when reading from /sys/kernel/tracing/rv/enabled_monitors
> on a couple of my arm64 boxes running Fedora after this change, which
> landed in mainline in 6.17-rc7. I can reproduce this in QEMU pretty
> easily.
...
> With this change reverted, there is no crash. As this change seems to
> have proper justification, is there some other latent bug here?

Thanks for the report.

Yes, this patch is broken, because argument 'p' of
enabled_monitors_next() *is* a pointer to struct rv_monitor. I'm not
sure how did I even test this patch... Steven is right, we really need
something in kselftest for RV, another thing in my RV TODO list.

But reverting is not the real fix, because monitors_show() still expects
a pointer to list_head. Changing monitors_show() is not an option,
because it is shared with the 'available_monitors' interface.

So the real fix is completely changing the iterator to be list_head
instead of rv_monitor.

Best regards,
Nam

diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index 48338520376f..43e9ea473cda 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -501,7 +501,7 @@ static void *enabled_monitors_next(struct seq_file *m, void *p, loff_t *pos)
 
 	list_for_each_entry_continue(mon, &rv_monitors_list, list) {
 		if (mon->enabled)
-			return mon;
+			return &mon->list;
 	}
 
 	return NULL;
@@ -509,7 +509,7 @@ static void *enabled_monitors_next(struct seq_file *m, void *p, loff_t *pos)
 
 static void *enabled_monitors_start(struct seq_file *m, loff_t *pos)
 {
-	struct rv_monitor *mon;
+	struct list_head *head;
 	loff_t l;
 
 	mutex_lock(&rv_interface_lock);
@@ -517,15 +517,15 @@ static void *enabled_monitors_start(struct seq_file *m, loff_t *pos)
 	if (list_empty(&rv_monitors_list))
 		return NULL;
 
-	mon = list_entry(&rv_monitors_list, struct rv_monitor, list);
+	head = &rv_monitors_list;
 
 	for (l = 0; l <= *pos; ) {
-		mon = enabled_monitors_next(m, mon, &l);
-		if (!mon)
+		head = enabled_monitors_next(m, head, &l);
+		if (!head)
 			break;
 	}
 
-	return mon;
+	return head;
 }
 
 /*