Skip to contentSkip to conversation

Fix wrong type cast in enabled_monitors_next()

PATCH In review Nam Cao 3 participants 8 messages Aug 6, 2025 – Sep 23, 2025

Subsystems (2)
Tracing Runtime verification (RV)
lore
Share & tools
lore

Nam Cao

author
#0 lore raw

PATCH 1/1 Fix wrong type cast in enabled_monitors_next()

Argument 'p' of enabled_monitors_next() is not a pointer to struct rv_monitor, it is actually a pointer to the list_head inside struct rv_monitor. Therefore it is wrong to cast 'p' to struct rv_monitor *.

This wrong type cast has been there since the beginning. But it still worked because the list_head was the first field in struct rv_monitor_def. This is no longer true since commit 24cbfe18d55a ("rv: Merge struct rv_monitor_def into struct rv_monitor") moved the list_head, and this wrong type cast became a functional problem.

Properly use container_of() instead.

Fixes 24cbfe18d55a ("rv: Merge struct rv_monitor_def into struct rv_monitor")
Signed-off-by Nam Cao <[email protected]>
---
 kernel/trace/rv/rv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/rv/rv.c +1 -1
index bd7d56dbf6c2..6ce3495164d8 100644
@@ -495,7 +495,7 @@ static void *available_monitors_next(struct seq_file *m, void *p, loff_t *pos)
  */
 static void *enabled_monitors_next(struct seq_file *m, void *p, loff_t *pos)
 {
-	struct rv_monitor *mon = p;
+	struct rv_monitor *mon = container_of(p, struct rv_monitor, list);
 
 	(*pos)++;
Signature · 1 line
2.39.5
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 4 lines · Nam Cao

Argument 'p' of enabled_monitors_next() is not a pointer to struct rv_monitor, it is actually a pointer to the list_head inside struct rv_monitor. Therefore it is wrong to cast 'p' to struct rv_monitor *.

Quoted 8 lines

This wrong type cast has been there since the beginning. But it still worked because the list_head was the first field in struct rv_monitor_def.
This is no longer true since commit 24cbfe18d55a ("rv: Merge struct rv_monitor_def into struct rv_monitor") moved the list_head, and this wrong type cast became a functional problem.

Properly use container_of() instead.

Good catch, thanks!

Reviewed-by Gabriele Monaco <[email protected]>
Quoted 24 lines
Fixes 24cbfe18d55a ("rv: Merge struct rv_monitor_def into struct

rv_monitor")

Signed-off-by Nam Cao <[email protected]>

---
 kernel/trace/rv/rv.c | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/rv/rv.c +1 -1
index bd7d56dbf6c2..6ce3495164d8 100644
@@ -495,7 +495,7 @@ static void *available_monitors_next(struct
 seq_file *m, void *p, loff_t *pos)
   */
  static void *enabled_monitors_next(struct seq_file *m, void *p,
 loff_t *pos)
  {
-	struct rv_monitor *mon = p;
+	struct rv_monitor *mon = container_of(p, struct rv_monitor,
 list);


  (*pos)++;

#2 lore raw
In reply to Nam Cao

Hi Nam,

Quoted 23 lines · Nam Cao

Argument 'p' of enabled_monitors_next() is not a pointer to struct rv_monitor, it is actually a pointer to the list_head inside struct rv_monitor. Therefore it is wrong to cast 'p' to struct rv_monitor *.

This wrong type cast has been there since the beginning. But it still worked because the list_head was the first field in struct rv_monitor_def. This is no longer true since commit 24cbfe18d55a ("rv: Merge struct rv_monitor_def into struct rv_monitor") moved the list_head, and this wrong type cast became a functional problem.

Properly use container_of() instead.

Fixes 24cbfe18d55a ("rv: Merge struct rv_monitor_def into struct rv_monitor")
Signed-off-by Nam Cao <[email protected]>
---
 kernel/trace/rv/rv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/rv/rv.c +0 -0
index bd7d56dbf6c2..6ce3495164d8 100644

kernel/trace/rv/rv.c static void *available_monitors_next(struct seq_file *m, void *p, loff_t *pos) · line 500

Quoted 7 lines
kernel/trace/rv/rv.c +1 -1
@@ -495,7 +495,7 @@ static void *available_monitors_next(struct seq_file *m, void *p, loff_t *pos)
  */
 static void *enabled_monitors_next(struct seq_file *m, void *p, loff_t *pos)
 {
-	struct rv_monitor *mon = p;
+	struct rv_monitor *mon = container_of(p, struct rv_monitor, list);
 
 	(*pos)++;

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.

  $ cat kernel/configs/repro.config
  CONFIG_FTRACE=y
  CONFIG_RV=y
  CONFIG_RV_MON_WWNR=y
  CONFIG_RV_PER_TASK_MONITORS=2
  CONFIG_RV_REACTORS=y
  CONFIG_RV_REACT_PANIC=y
  CONFIG_RV_REACT_PRINTK=y
  $ git show -s --format='%h ("%s")'
  097a6c336d00 ("Merge tag 'trace-rv-v6.17-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace")
  $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux- clean virtconfig repro.config Image.gz
  $ curl -LSs https://github.com/ClangBuiltLinux/boot-utils/releases/download/20241120-044434/arm64-rootfs.cpio.zst | zstd -d >rootfs.cpio
  $ qemu-system-aarch64 \
      -display none \
      -nodefaults \
      -cpu max,pauth-impdef=true \
      -machine virt,gic-version=max,virtualization=true \
      -append 'console=ttyAMA0 earlycon rdinit=/bin/sh'
      -kernel arch/arm64/boot/Image.gz \
      -initrd rootfs.cpio \
      -m 512m \
      -serial mon:stdio
  ...
  [    0.000000] Linux version 6.17.0-rc6-00143-g097a6c336d00 (nathan@aadp) (aarch64-linux-gcc (GCC) 15.2.0, GNU ld (GNU Binutils) 2.45) #1 SMP PREEMPT Mon Sep 22 17:03:48 MST 2025
  ...
  ~ # mount -t sysfs sysfs /sys &&
Quoted 2 lines

mount -t tracefs tracing /sys/kernel/tracing &&
cat /sys/kernel/tracing/rv/enabled_monitors

  [  134.672494] Unable to handle kernel paging request at virtual address ffffffffffffffd0
  [  134.675919] Mem abort info:
  [  134.677233]   ESR = 0x0000000096000006
  [  134.679118]   EC = 0x25: DABT (current EL), IL = 32 bits
  [  134.680880]   SET = 0, FnV = 0
  [  134.682180]   EA = 0, S1PTW = 0
  [  134.683788]   FSC = 0x06: level 2 translation fault
  [  134.685435] Data abort info:
  [  134.687096]   ISV = 0, ISS = 0x00000006, ISS2 = 0x00000000
  [  134.688973]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
  [  134.690889]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
  [  134.692687] swapper pgtable: 4k pages, 52-bit VAs, pgdp=00000000417a8000
  [  134.694969] [ffffffffffffffd0] pgd=1000000041c8d003, p4d=0000000041bf2403, pud=0000000041bf3403, pmd=0000000000000000
  [  134.700920] Internal error: Oops: 0000000096000006 [#1]  SMP
  [  134.702603] Modules linked in:
  [  134.703956] CPU: 0 UID: 0 PID: 53 Comm: cat Not tainted 6.17.0-rc6-00143-g097a6c336d00 #1 PREEMPT
  [  134.705506] Hardware name: linux,dummy-virt (DT)
  [  134.706481] pstate: a1402009 (NzCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
  [  134.707595] pc : enabled_monitors_start+0x6c/0xa0
  [  134.709324] lr : enabled_monitors_start+0x24/0xa0
  [  134.710047] sp : ffff80008043bbf0
  [  134.710557] x29: ffff80008043bbf0 x28: fff0000002e58000 x27: 0000000000000000
  [  134.711889] x26: ffff80008043bca0 x25: fff0000002b44038 x24: 0000000000000000
  [  134.712965] x23: 0000000000000001 x22: 0000000000000000 x21: ffff80008043bcc8
  [  134.714028] x20: ffffa8bf4243e000 x19: ffffa8bf4243ed00 x18: 0000000000000000
  [  134.714994] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
  [  134.716046] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
  [  134.717110] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
  [  134.718219] x8 : 0000000000000000 x7 : 0000000000000001 x6 : 0000000000000000
  [  134.719246] x5 : 0000000000001000 x4 : fff000001fdea760 x3 : 0000000000000000
  [  134.720195] x2 : 0000000000000000 x1 : fff0000002b44028 x0 : ffffffffffffffc0
  [  134.721411] Call trace:
  [  134.722173]  enabled_monitors_start+0x6c/0xa0 (P)
  [  134.723088]  seq_read_iter+0xd4/0x47c
  [  134.723752]  seq_read+0xec/0x12c
  [  134.724200]  vfs_read+0xc4/0x33c
  [  134.724791]  ksys_read+0x64/0x100
  [  134.725255]  __arm64_sys_read+0x18/0x24
  [  134.725860]  invoke_syscall.constprop.0+0x40/0xf0
  [  134.726466]  el0_svc_common.constprop.0+0x38/0xd8
  [  134.727092]  do_el0_svc+0x1c/0x28
  [  134.727558]  el0_svc+0x34/0xe8
  [  134.728112]  el0t_64_sync_handler+0xa0/0xe4
  [  134.728763]  el0t_64_sync+0x198/0x19c
  [  134.729790] Code: f9402001 d1010020 eb01027f 540000c0 (39404001)
  [  134.731074] ---[ end trace 0000000000000000 ]---
  Segmentation fault

With this change reverted, there is no crash. As this change seems to have proper justification, is there some other latent bug here?

If there is any other information I can provide or patches I can test, I am more than happy to do so.

Cheers,
Nathan

author
#3 lore raw

Hi Nathan,

Quoted 4 lines · Nathan Chancellor

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 2 lines

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

kernel/trace/rv/rv.c +6 -6
index 48338520376f..43e9ea473cda 100644
@@ -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;
 }
 
 /*
maintainer
#4 lore raw
Quoted 2 lines · Nam Cao

Hi Nathan,

Thanks for finding this!

Quoted 7 lines
Quoted 5 lines · Nathan Chancellor

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 8 lines
Quoted 2 lines

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: ..

Quoted 2 lines

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.

Quoted 7 lines

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 49 lines

Best regards,
Nam

kernel/trace/rv/rv.c +0 -0
index 48338520376f..43e9ea473cda 100644
@@ -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)

kernel/trace/rv/rv.c +1 -1
-			return mon;
+			return &mon->list;

  }

  return NULL;

kernel/trace/rv/rv.c +0 -0
@@ -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) {

kernel/trace/rv/rv.c +1 -1
-	struct rv_monitor *mon;
+	struct list_head *head;

  loff_t l;

  mutex_lock(&rv_interface_lock);

kernel/trace/rv/rv.c +0 -0
@@ -517,15 +517,15 @@ static void *enabled_monitors_start(struct seq_file *m,
 loff_t *pos)
  	if (list_empty(&rv_monitors_list))
  		return NULL;

kernel/trace/rv/rv.c +1 -1
-	mon = list_entry(&rv_monitors_list, struct rv_monitor, list);
+	head = &rv_monitors_list;


  for (l = 0; l <= *pos; ) {

kernel/trace/rv/rv.c +2 -2
-		mon = enabled_monitors_next(m, mon, &l);
-		if (!mon)
+		head = enabled_monitors_next(m, head, &l);
+		if (!head)

  break;
  }

kernel/trace/rv/rv.c +1 -1
-	return mon;
+	return head;

 }

 /*

author
#5 lore raw
Quoted 2 lines · Gabriele Monaco

Looks reasonable, can you work on the fix?
I see Steve is out for conferences so this won't be too urgent.

The fix is in my previous email.

I am guessing your email client is hiding the diff from you ;) (among wrapping quoted reply, but that's not too important).

Nam

maintainer
#6 lore raw
Quoted 8 lines · Nam Cao
Quoted 2 lines · Gabriele Monaco

Looks reasonable, can you work on the fix?
I see Steve is out for conferences so this won't be too urgent.

The fix is in my previous email.

I am guessing your email client is hiding the diff from you ;) (among wrapping quoted reply, but that's not too important).

Uh, that's my brain ignoring what's under the signature, need to look for an update to that..

Going to test it.

Thanks,
Gabriele

maintainer
#7 lore raw
In reply to Nam Cao
Quoted 5 lines · Nam Cao
Quoted 2 lines · Gabriele Monaco

Looks reasonable, can you work on the fix?
I see Steve is out for conferences so this won't be too urgent.

The fix is in my previous email.

I confirm enabled_monitors works with your fix. Feel free to post a patch and I'll battle test it and include it.

Thanks,
Gabriele

Quoted 5 lines

I am guessing your email client is hiding the diff from you ;) (among wrapping quoted reply, but that's not too important).

Nam