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

[Paul Brook]

> > > > > 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.
> > > 
> > > OK -- so this is how it adds up:
> > > 
> > > - guest vq
> > > - virtio-serial-bus converts iov to buf
> > 
> > This is an unbelievably lame piece of code.
> 
> I doubt it's 'unbelievably lame' just because of the copy.  Care to
> expand?

Specifically that we are allocating a host buffer of guest-specified size to 
hold that copy.
 
> > There's absolutely no reason to
> > copy the data into a linear buffer. You should just be iterating over the
> > elements of the sglist.
> 
> OK, but that can be done in a separate patch series.

I suspect you'll actually find it easier to fix that first. Otherwise you're 
going end up having to rewrite your own code.

> > > - qemu-char stores the buf in case it wasn't able to send
> > > 
> > > but then, since it's all async, we have:
> > > 
> > > - virtio-serial-bus frees the buf
> > > - guest deletes the buf and removes it from the vq
> > > 
> > > So what's left is only the data in qemu-char's buf.  Now this can be
> > > (buf_size - 1) * nr_ports in the worst case.
> > 
> > Add at least another buf_size because you have to allocate the qemu-char
> > buffer before you free the virtio-serial buffer. We can expect that
> > buf_size ~= guest ram size [1], so for practical purposes it may as well
> > be unbounded.
> 
> Now this only happens when the host chardev is slow or isn't being read
> from.  So it's not really a guest causing a host DoS, but a guest
> causing itself some harm.  

No. It causes qemu to allocate and use an arbitrarily large amount of 
additional ram on the host. This is likely to effect the whole host machine, 
not just the problematic guest.  You can hope the OOM killer happens to pick 
the right guest, but I wouldn't bet on it.

> You're right that the allocations happen one
> after the other, and the freeing happens later, so there is a time when
> 2 or 3 times the buf_size is needed.
> 
> However, once qemu_chr_write() returns, there could be just one copy
> lying around, things are freed elsewhere.

One copy (multiplied by the number of ports) is more than enough to cause 
serious problems.

> > > but then that depends on qemu getting async support - separating out
> > > the qemu_chr_write() into a separate thread and allowing vcpu and chr
> > > io operations to be run simultaneously.
> > 
> > You don't need any special async char API or threads.  Normal unix write
> > semantics (i.e. short writes and EAGAIN) plus the unblock hook are
> > sufficient. As mentioned above, the virtio-serial code should be
> > iterating over the sglist.  If the host won't accept all the data
> > immediately then just remember how much has been sent, and resume
> > iteration when the unblock hook is called.
> 
> Yes I've been thinking about this as well.  But the problem is some
> kernel versions spin in the guest code till the buffer is placed back
> in the vq (signalling it's done using it).  This is a problem for the
> virtio-console (hvc) that does writes with spinlocks held, so allocating
> new buffers, etc., isn't really -- possible easily.

That's a guest bug, plain and simple.
I'm pretty sure such guests will still loose after your patch. All you're 
doing is delaying the inevitable slightly. i.e. if a guest happens to submit 
another block before the first has been flushed then it will spin in exactly 
the same way.

Paul