Paths

Table of Contentst

LinuxKPI: Update seq_file to properly implement the iterator interface
Needs ReviewPublic
Actions

Authored by jhb on Tue, Mar 17, 6:04 PM.

Details

Reviewers

Summary

The seq_file.rst documentation in the Linux kernel documents the
iterator interface for the seq_file structure. In particular, the
ppos passed to seq_read is a logical offset into a seq_file managed by
the iterator interface, not an offset into the generated data. For
example, if a seq_file outputs state for each node in a linked-list or
array, *ppos might be used as the index of the node to output, not a
byte offset.

Rewrite seq_read to honor this contract which fixes a few bugs:

Treat *ppos as a logical iterator offset that is only updated by the next callback after outputting a single item via the show method.

Note that when outputting the description for a single item, a dummy buffer offset of 0 is passed to simple_read_from_buffer.

Use a loop to permit outputting descriptions of multiple items if the user buffer is large enough.

Always invoke the stop method after terminating the loop to cleanup any state setup by start (e.g. if start allocated a buffer or obtained a lock, the stop method is called to cleanup).

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 71523
Build 68406: arc lint + arc unit

Event Timeline

jhb created this revision.Tue, Mar 17, 6:04 PM

Herald added subscribers: linuxkpi, emaste, imp. · View Herald TranscriptTue, Mar 17, 6:04 PM

jhb requested review of this revision.Tue, Mar 17, 6:04 PM

Harbormaster completed remote builds in B71462: Diff 173816.Tue, Mar 17, 6:04 PM

jhb added a parent revision: D55845: LinuxKPI: Better handling of zero-size and off-the-end reads.Tue, Mar 17, 6:04 PM

So seq_file was even more broken :-P This commit is stacked on top of the other fixes.

sys/compat/linuxkpi/common/src/linux_seq_file.c
110	This could maybe just be a single call to copy_to_user here which would avoid the need for the dummy_pos.
120	Possibly we shouldn't call next if this was a short read (i.e. `if (rc < sbuf_len(sbuf))`)

bz added inline comments.Wed, Mar 18, 3:12 PM

sys/compat/linuxkpi/common/src/linux_seq_file.c
105	Thanks for fixing also that one. I still had in my tree from the debug session but I had caused too much confusion already.
120	There is data left in the sbuf not copied to ubuf in that case. If that understanding is correct, I believe we should error and break as I cannot see how the current code could recover from that. But that leaves the question below then.
130	If you experience an error in a second or later iteration, rc is negative but we still return a success value. Does that match the callers expectations to recover/continue/abort? I think leaving a comment about this (why one takes precedence over the other) would be helpful

bz requested changes to this revision.Wed, Mar 18, 3:56 PM

bz added inline comments.

sys/compat/linuxkpi/common/src/linux_seq_file.c
102	On second iteration this will operate on an finished sbuf; the sbuf_clear(sbuf) needs to move.

This revision now requires changes to proceed.Wed, Mar 18, 3:56 PM

bz added inline comments.Wed, Mar 18, 4:42 PM

sys/compat/linuxkpi/common/src/linux_seq_file.c
101	This is not right; we come in with a 4k ubuf and a 4k size; we will iterate n times (printing the same thing possibly) until the page is full. Then we return with a partial read (currently no effect) out of the function with nread=4k. Then the entire function is called again and again and again and again... I hit ^c when *ppos was 1190. I believe if sbuf_len < size, we should just break somewhere below before ->next ?

Humm, I guess I need the loop to just do show/next. I do think on a "short read" you need to return success, but yeah, the simple_read_from_buffer should perhaps be outside of the loop.

Actually, to support SEQ_SKIP (which is in the docs but we don't currently implement), I do indeed need to keep the sbuf_clear/finish in the loop. Will upload a new version in a bit.

Rewrite loop

Harbormaster completed remote builds in B71522: Diff 173911.Thu, Mar 19, 3:52 PM

Actually, the documentation doesn't permit sleeping between the start and stop callbacks, so it will need to be a single copyout() at the end regardless.

Possibly the sbufs' need to be created with SB_NOWAIT to honor this btw, but I will punt on that for now.

Tweak sbuf overflow case

Harbormaster completed remote builds in B71523: Diff 173912.Thu, Mar 19, 3:58 PM

jhb added inline comments.Thu, Mar 19, 7:34 PM

sys/compat/linuxkpi/common/src/linux_seq_file.c
74	This works because we won't update *ppos when we break here, so the next read() call on the file will start at the item whose description was truncated.

bz added inline comments.Fri, Mar 20, 12:41 AM

sys/compat/linuxkpi/common/src/linux_seq_file.c
110	Is there a reason to use the FreeBSD native copyout and not copy_to_user/linux_copyout here?

jhb added inline comments.Fri, Mar 20, 1:55 PM

sys/compat/linuxkpi/common/src/linux_seq_file.c
110	Because the error handling sucks less in that you get the error back (e.g. for CHERI copyout can return EPROT not just EFAULT). We already use malloc(9) in this file instead of kmalloc(). I originally did use copy_from_user here and had written it as rc = copy_to_user(ubuf, sbut_data(sbuf), todo); if (rc != todo) return (-EFAULT)

bz added inline comments.Fri, Mar 20, 2:03 PM

sys/compat/linuxkpi/common/src/linux_seq_file.c
110	I was asking because linux_copyout has that linux_remap_address() magic before it tries copyout; I don't know why that is there but a direct copyout circumvents it. If that is fine, I am fine. [Y/n]

Revision Contents
Changeset List

Path

Size

sys/

compat/

linuxkpi/

common/

include/

linux/

seq_file.h

2 lines

src/

linux_seq_file.c

64 lines

Diff 173912

View Options

sys/compat/linuxkpi/common/include/linux/seq_file.h