Re: [PATCH] rv: Fix wrong type cast in enabled_monitors_next()
From: Gabriele Monaco <gmonaco@redhat.com>
Date: 2025-09-23 08:30:24
Also in:
lkml
On Tue, 2025-09-23 at 07:28 +0200, Nam Cao wrote:
Hi Nathan,
Thanks for finding this!
Nathan Chancellor [off-list ref] writes:quoted
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....quoted
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...
Damn, I'm wondering the same :facepalm: ..
Steven is right, we really need something in kselftest for RV, another thing in my RV TODO list.
I can work on that, at least a few selftests for the sysfs, I think this gets the top priority now.
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.
Looks reasonable, can you work on the fix? I see Steve is out for conferences so this won't be too urgent. Thanks, Gabriele
quoted hunk ↗ jump to hunk
Best regards, Namdiff --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; } /*