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

[Paul Brook]

> On (Thu) Dec 02 2010 [17:31:36], Paul Brook wrote:
> > > > > when there's a partial write, it tries to do a write again, which
> > > > > will fail with -EAGAIN.
> > > > 
> > > > Doesn't that cause the first partial chunk to be incorrectly
> > > > transmitted twice? You may only return EAGAIN if no data was
> > > > transmitted.
> > > 
> > > Except for the fact that no caller of qemu_chr_write() resubmits (or
> > > even checks) partial writes.
> > 
> > I don't buy this argument. The current implementation of qemu_chr_write
> > never generates transient failures, so they don't need to.
> 
> And applying this patch won't change the situation.

Sure it will. The whole point of the patch is to allow transient failures 
(i.e. avoid blocking) when writing to char backends.  You should expect to 
have to modify the device code to cope with this.

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.

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.

> What I proposed in the earlier mail was to buffer only the data that has
> to be re-submitted in case the caller is capable of stopping further
> output till the char layer indicates it's free to start again.

That's case (b) below.

> > Once data has been transmitted, we have three options:
> > 
> > a) Block until the write completes. This makes the whole patch fairly
> > pointless as host and guest block boundaries are unlikely to align.
> 
> This is what currently happens and will remain so for callers of
> qemu_chr_write() which don't have a .write_unblocked() pointer assigned
> in the char dev struct.

Obviously if the device doesn't supply an unbocked() hook then the behavior is 
unchanged.  That's trivially uninteresting.  I'm talking about devices that do 
provide the unblocked() hook.

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

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.

As a side note, I notice that the virtio-serial-buf code is already allocating 
buffers and calling iov_to_buf on arbitrary sized requests. This is wrong for 
the same reason. Don't do it.

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

Paul