Hi,
Thanks for your quick review!
On Sun, Jul 07, 2024 at 08:28:50PM GMT, Marc-André Lureau wrote:
> Hi
>
> On Sun, Jul 7, 2024 at 3:26 PM Ruihan Li <lrh2000@pku.edu.cn> wrote:
>
> > This commit fixes a bug that causes incorrect results when pasting more
> > than 32 bytes, the size of the receive buffer b->buffer, into the virtio
> > console.
> >
> > Example (note that the last 32 bytes are always correct, but something
> > goes wrong just before the last 32 bytes):
> >
> > Pasting
> > abcdefghijklmnopqrstuvwxyz1234567890ABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()
> > Received
> > abcdefg)EFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()EFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()
> >
> > The root cause of this bug is as follows:
> >
> > The mux_chr_read function passes the data to the backend via
> > be->chr_read one byte at a time, either directly or via another
> > mux_chr_accept_input method. However, if the receive buffer is full,
> > there is a chance that the mux_chr_can_read method will return more than
> > one byte, because in this case the method directly returns whatever
> > be->chr_can_read returns.
> >
> > This is problematic because if mux_chr_read passes a byte to the backend
> > by calling be->chr_read, it will consume the entire backend buffer, at
> > least in the case of virtio. Once all backend buffers are used,
> > mux_chr_read writes all remaining bytes to the receive buffer d->buffer,
> >
>
> My understanding of the code execution is:
> - mux_chr_can_read() returns be->chr_can_read(), say N, because d->buffer
> is already MUX_BUFFER_SIZE.
> - mux_chr_read() is called with N bytes
> - mux_chr_accept_input() flushes d->buffer, writing MUX_BUFFER_SIZE
> - be should still accept N-MUX_BUFFER_SIZE
> - mux_proc_byte() loops for N bytes
> - chr_read() should accept the N-MUX_BUFFER_SIZE
> - d->buffer is then filled with the remaining MUX_BUFFER_SIZE
Note this:
[..] if mux_chr_read passes a byte to the backend by calling
be->chr_read, it will consume the entire backend buffer, at
least in the case of virtio [..]
At least in the case of virtio, if the guest provides a buffer of length
4096, be->chr_can_read will report 4096. But if you then call
be->chr_read with one byte, the whole 4096 buffer will be used. After
that, be->chr_can_read will return zero instead of 4095.
This should make sense since the device cannot change the number of
bytes in the buffer after it has made the buffer available to the CPU.
Thanks, that helps explaining the incorrect behaviour.
I think the concept of extra buffer as introduced in commit bd9bdce694ccb76facc882363e4c337e8a88c918 ("Add input buffer to mux chr (patch by Tristan Gingold)") is flawed, as Jan Kiszka explained in commit a80bf99fa3dd829ecea88b9bfb4f7cf146208f07 ("char-mux: Use separate input buffers (Jan Kiszka)"):
Note: In contrast to the original author's claim, the buffering concept
still breaks down when the fifo of the currently active sub-device is
full. As we cannot accept futher data from this point on without risking
to loose it, we will also miss escape sequences, just like without all
that buffering. In short: There is no reliable escape sequence handling
without infinite buffers or the risk of loosing some data.
Maybe the best course is to remove the cycle buffer and either:
- drop the data that be can't accept, but have always responsive mux (by default)
- blocking, including mux, until the be can accept more data (not friendly)
- or allow unlimited buffering?
Given that mux is meant for developers and qemu CLI users, I guess any of this would be acceptable.
>
>
> > but the number of remaining bytes can be larger than the buffer size.
> >
>
> By the above description, I don't see how it happens.
>
> This does not lead to security problems since it is a ring buffer, but
> > it does mess up the receive data.
> >
> > This can be fixed by having mux_chr_can_read return either zero or one.
> > This fix is not very efficient, but it is quite reasonable since
> > mux_chr_read also passes the data to the backend one byte at a time.
> >
>
> Could you share your testing setup? Even better if you could write a test!
This happens in https://github.com/asterinas/asterinas. Sorry, but I
don't have a minimal reproducible example, and I don't think I can make
one anytime soon.
As for the tests, I'd like to know how to write such tests in QEMU. I
checked the documentation but didn't find anything, maybe I'm missing
something?
>
>
> thanks
>
>
> > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > ---
> > chardev/char-mux.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> > index ee2d47b..5c6eea2 100644
> > --- a/chardev/char-mux.c
> > +++ b/chardev/char-mux.c
> > @@ -210,8 +210,8 @@ static int mux_chr_can_read(void *opaque)
> > return 1;
> > }
> >
> > - if (be && be->chr_can_read) {
> > - return be->chr_can_read(be->opaque);
> > + if (be && be->chr_can_read && be->chr_can_read(be->opaque)) {
> > + return 1;
> > }
> >
> > return 0;
> > --
> > 2.45.2
> >
> >
> >
>
> --
> Marc-André Lureau
Thanks,
Ruihan Li