Re: [Qemu-devel] [PATCH v8 7/7] virtio-console: Enable port throttling when chardev is slow to consume data

[Paul Brook]

> > As with the DMA interface added a while ago, I believe it's important to
> > get these APIs right.  Quick hacks to support limited use-cases just end
> > up needing a complete rewrite (or even worse multiple concurrent
> > APIs/implementations) once we actually start using them seriously.
> 
> Sure.  My proposal is for qemu_chr_write() to succeed all the time.  If
> the backend can block, and the caller can handle it, it can get a
> -EAGAIN (or WSAEWOULDBLOCK) return.  When the backend becomes writable,
> the chardev can call the ->writes_unblocked() callback for that caller.
> Individual callers need not bother about re-submitting partial writes,
> since the buffering can be done in common code in one place
> (qemu-char.c).

That's only OK if you assume it's OK to buffer all but one byte of the 
transmit request.

> > I'm extremely reluctant to add a new layer of buffering that is not
> > visible to ether the kernel or the device.  In practice we still need to
> > be able to split oversize requests, so handling small split requests
> > should be pretty much free.
> 
> So do you propose to propagate this -EAGAIN error all the way to the
> guest?  That won't work for older virtio guests (for virtio, host and
> guest changes will be needed).

Huh? That doesn't make any sense. The guest is already using an asyncronous 
submission mechanism.  
With a virtio device the status of a buffer becomes indeterminate once it has 
been placed into the queue. Only when it is removed from the queue do we know 
that it has completed.  The device may transfer all or part of that buffer at 
any time in between.
 
> > > > b) Store the data on the side somewhere. Tell the device all data has
> > > > been sent, and arrange for this data to be flushed before accepting
> > > > any more data. This is bad because it allows the guest to allocate
> > > > arbitrarily large[1] buffers on the host. i.e. a fairly easily
> > > > exploitable DoS attack.
> > > 
> > > With virtio-serial, this is what's in use.  The buffer is limited to
> > > the length of the vq (which is a compile-time constant) and there also
> > > is the virtio_serial_throttle_port() call that tells the guest to not
> > > send any more data to the host till the char layer indicates it's OK
> > > to send more data.
> > 
> > No.
> > 
> > Firstly you're assuming all users are virtio based. That may be all you
> > care about, but is not acceptable if you want to get this code merged.
> 
> OK, but it's assumed that once a -EAGAIN is returned, the caller will
> take appropriate actions to restrict the data sent.  Especially,
> send_all has:
> 
>     if (chr->write_blocked) {
>         /*
>          * We don't handle this situation: the caller should not send
>          * us data while we're blocked.
>          *
>          * We could buffer this data here but that'll only encourage
>          * bad behaviour on part of the callers.

>          */
>         return -1;
>     }

If you're being draconian about this then do it properly and make this an 
abort. Otherwise return -EAGAIN. Returning a random error seems like the worst 
of both worlds.  Your code results in spurious guest errors (or lost data) 
with real indication that this is actually a qemu device emulation bug.
 
> > Secondly, the virtqueue only restricts the number of direct ring buffer
> > entries. It does not restrict the quantity of data each ring entry points
> > to.
> 
> But that's entirely in guest memory, so it's limited to the amount of
> RAM that has been allocated to the guest.

Exactly. The guest can cause ram_size * nr_ports of additional host memory to 
be allocated.  Not acceptable. 

> > > > c) Return a partial write to the guest. The guest already has to
> > > > handle retries due to EAGAIN, and DMA capable devices already have
> > > > to handle partial mappings, so this doesn't seem too onerous a
> > > > requirement. This is not a new concept, it's the same as the unix
> > > > write(2)/send(2) functions.
> > > 
> > > This isn't possible with the current vq design.
> > 
> > You need to fix that then.  I'm fairly sure it must be possible as
> > virtio-blk has to handle similar problems.
> 
> This was one of the items that was debated during the lead-up to
> virtio-serial merge:  whether a write() call in the guest means data has
> been flushed out to the host chardev or if the guest has just passed it
> on to the host to take care of it.  We merged the latter approach.

Ensuring that data has actually reached the endpoint (v.s. being in a queue 
for transmission at the next available point) is a different problem, and 
probably one better solved by higher level protocols.


Char devices are fundamentally stream based devices with no frame boundaries 
(or alternatively a fixed frame size of 1 byte).

Your device only reports progress to the guest in virtqueue-entry sized 
chunks. However that places no real restrictions on how the data is passed to 
the host. You can choose to pass a single queue entry to the host in several 
smaller chunks, or even pass several queue entries to the host in a single 
request.

Paul