LinuxKPI: Update seq_file to properly implement the iterator interface

[jhb]

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

[jhb]

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

118

This could maybe just be a single call to copy_to_user here which would avoid the need for the dummy_pos.

128

Possibly we shouldn't call next if this was a short read (i.e. if (rc < sbuf_len(sbuf)))

[bz]

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.

128

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.

138

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]

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.

[bz]

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 ?

[jhb]

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.

[jhb]

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.

[jhb]

Rewrite loop

[jhb]

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.

[jhb]

Tweak sbuf overflow case

[jhb]

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]

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

118

Is there a reason to use the FreeBSD native copyout and not copy_to_user/linux_copyout here?

[jhb]

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

118

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]

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

118

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]

[jhb]

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

118

Hummm, I suspect this will never be used as part of an ioctl request, but perhaps it is better to use copy_to_user after all. Blech.

[bz]

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

118

If you find the time to do this the next days I can still test it again.

[jhb]

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

118

Belatedly done.

[bz]

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

118

Sorry for asking, are we going to Blech change this to copy_to_user in the end or not or did you mark them done and forgot to update the review?
If we are not going to do it, this is fine to go in as-is I believe.

[jhb]

Use copy_to_user

[jhb]

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

118

I had changed it, but apparently failed to upload it. Hopefully now updated.

[bz]

If we do the one liner, this is approved in case you don't want to wait for me anymore.

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

119

I believe this needs to be a ==

linux_copyout returns 0 on success or != on error (-copyout(...))
copy_to_user returns n (todo) in case linux_copyout does not return 0 (error case) and 0 otherwise.

So either rc != 0 or rc == todo would indicate the error.

With the latter change applied I no longer get EFAULT and things work.

[jhb]

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

119

Oh, I believe I want to check against zero. IIRC, copy_to_user in Linux returns the number of bytes _not_ read (here I had misremembered it as the number of bytes _read_). I think I prefer rc != 0 as even a theoretical partial transfer should still be treated as a fault.

[bz]

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

119

ACK.

[jhb]

Fix test for copy_to_user result