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