LinuxKPI: Use simple_read_from_buffer in simple_attr_read and seq_read

[jhb]

Summary

Sponsored by: AFRL, DARPA

[bz]

I'll get back to you within the hour.

sys/compat/linuxkpi/common/src/linux_seq_file.c

63–64

Sorry for asking; I'll still give it a try in a moment and try to wrap my head around it again, but if using PFS_RAW, do we still have an sbuf here?
Oh this is the one from seq_open() or simple_open() not the one from pseudofs.
Sorry my fault; I got that wrong last night.

[jhb]

sys/compat/linuxkpi/common/src/linux_seq_file.c

63–64

No worries, took me a while to figure it out as well.

[bz]

sys/compat/linuxkpi/common/src/linux_seq_file.c

55

Can of works; unclear if this was working before; I'll go and see about it after dinner.

# cat bb_18 
panic: sbuf_vprintf called with finished or corrupt sbuf
cpuid = 0
time = 1773690739
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe0067a74600
vpanic() at vpanic+0x136/frame 0xfffffe0067a74730
panic() at panic+0x43/frame 0xfffffe0067a74790
sbuf_vprintf() at sbuf_vprintf+0xa9/frame 0xfffffe0067a747b0
lkpi_seq_printf() at lkpi_seq_printf+0x46/frame 0xfffffe0067a74810               (bz: multiple seq_printf calls)
rtw_debug_get_bb_page() at rtw_debug_get_bb_page+0x4f/frame 0xfffffe0067a74850   (bz: = op->show from rtw_debugfs_single_open_rw, single_open, ..)
seq_read() at seq_read+0x38/frame 0xfffffe0067a74890
debugfs_fill() at debugfs_fill+0x13e/frame 0xfffffe0067a74ba0
pfs_read() at pfs_read+0x29d/frame 0xfffffe0067a74c00

and it's not this:

--- sys/compat/linuxkpi/common/src/linux_seq_file.c
+++ sys/compat/linuxkpi/common/src/linux_seq_file.c
@@ -111,7 +111,7 @@ static void *
 single_start(struct seq_file *p, off_t *pos)
 {
 
-       return ((void *)(uintptr_t)(*pos == 0));
+       return ((void *)(uintptr_t)(*pos = 0));
 }
 
 static void *

[bz]

sys/compat/linuxkpi/common/src/linux_seq_file.c

55

hmm the *pos == 0 was intentional.

[bz]

sys/compat/linuxkpi/common/src/linux_seq_file.c

63–64

We are getting called twice with the same sbuf.

seq_read:53: m 0xfffff8001546b340 *ppos 0 sbuf 0xfffff80015347f00 { s_flags 0x00290001 }
seq_read:55: p 0x1 m 0xfffff8001546b340 sbuf 0xfffff80015347f00 { s_flags 0x00290001 }
seq_read:57: p 0x1 m 0xfffff8001546b340 *ppos 0 sbuf 0xfffff80015347f00 { s_flags 0x00290001 } rc 0
seq_read:53: m 0xfffff8001546b340 *ppos 945 sbuf 0xfffff80015347f00 { s_flags 0x002b0001 }
seq_read:55: p 0 m 0xfffff8001546b340 sbuf 0xfffff80015347f00 { s_flags 0x002b0001 }
panic: sbuf_vprintf called with finished or corrupt sbuf 0xfffff80015347f00 { s_flags 0x002b0001 }, state 0000000000

This helps:

@@ -59,8 +62,12 @@ seq_read(struct linux_file *f, char *ubuf, size_t size, off_t *ppos)
        if (rc)
                return (rc);
 
-       return (simple_read_from_buffer(ubuf, size, ppos, sbuf_data(sbuf),
-           sbuf_len(sbuf)));
+       rc = simple_read_from_buffer(ubuf, size, ppos, sbuf_data(sbuf),
+           sbuf_len(sbuf));
+
+       sbuf_clear(sbuf);
+
+       return (rc);
 }
 
 int

I am still getting a "Bad Address" error at the end of each "file" but I haven't figured out where that comes from yet.

[jhb]

sys/compat/linuxkpi/common/src/linux_seq_file.c

55

Is that just with this change? You should be able to try the stack in order, so starting with only this commit (and no other) on top of main it should still work. If that alone raises this panic then that points to some other pre-existing bug (but I can go look at it).

Looking at this code though, it does seem fundamentally broken. We should probably be resetting the sbuf back to empty before calling m->op->start() assuming that m->op->show is supposed to regenerate all of the output each time?

Yes. And sigh, I went and read the seq_file.rst documentation in Linux and we aren't doing _anything_ with the next callback in seq_read which is dumb. Our implementation is quite crap in that regard, but for now, just try this:

--- a/sys/compat/linuxkpi/common/src/linux_seq_file.c
+++ b/sys/compat/linuxkpi/common/src/linux_seq_file.c
@@ -49,6 +49,7 @@ seq_read(struct linux_file *f, char __user *ubuf, size_t size, off_t *ppos)
 
 	m = f->private_data;
 	sbuf = m->buf;
+	sbuf_clear(sbuf);
 
 	p = m->op->start(m, ppos);
 	rc = m->op->show(m, p);

[bz]

sys/compat/linuxkpi/common/src/linux_seq_file.c

55

Yeah, the more I look the more... I like your version better than mine. But yes, a bug from before your changes.

I am looking into the EFAULT atm.

63–64

I am still getting a "Bad Address" error at the end of each "file" but I haven't figured out where that comes from yet.

Missed this with my debugging...

debugfs_fill:194 read/write failed with -14

[bz]

So I think the only one thing here is the sbuf_clear(); In theory that should go in by itself upfront; be my guest to just do it; Reviewed by: bz

Then this one should be fine too. Though it will lead to a follow-up panic in D55883 until fixed there.

sys/compat/linuxkpi/common/src/linux_seq_file.c

63–64

The EFAULT is in D55833 (well or the comment to fix it is there).

[jhb]

sys/compat/linuxkpi/common/src/linux_seq_file.c

63–64

I bet we basically don't handle EOF correctly (see my comment about not making use of the "next" callback).