Re: [PATCH v2] virtio-rng: return available data with O_NONBLOCK

[Laurent Vivier]

On 11/08/2020 15:14, Michael S. Tsirkin wrote:
> On Tue, Aug 11, 2020 at 03:00:14PM +0200, Laurent Vivier wrote:
>> No problem. This code is tricky and it took me several months to really
>> start to understand it ...
> 
> Oh great, we actually have someone who understands the code!
> Maybe you can help me understand: virtio_read
> takes the buf pointer and puts it in the vq.
> It can then return to caller (e.g. on a signal).
> Device can meanwhile write into the buffer.
> 
> It looks like if another call then happens, and that
> other call uses a different buffer, virtio rng
> will happily return the data written into the
> original buf pointer, confusing the caller.
> 
> Is that right?
> 

Yes.

hw_random core uses two bufers:

- rng_fillbuf that is used with a blocking access and protected by the
reading_mutex. I think this cannot be interrupted by a kill because it's
in  hwrng_fillfn() and it's kthread.

- rng_buffer that is used in rng_dev_read() and can be interrupted (it
is also protected by reading_mutex)

But if rng_dev_read() is called with O_NONBLOCK or interrupted and then
rng_fillbuf starts they can be mixed.

We have also the first use of rng_buffer in add_early_randomness() that
use a different size than in rng_dev_read() with the same buffer (and
this size is 16 whereas the hwrng read API says it must be at least 32...).

The problem here is core has been developped with synchronicity in mind,
whereas virtio is asynchronous by definition.

I think we should add some internal buffers in virtio-rng backend. This
would improve performance (we are at 1 MB/s, I sent a patch to improve
that, but this doesn't fix the problems above), and allows hw_random
core to use memory that doesn't need to be compatible with virt_to_page().

Thanks,
Laurent