Linux Virtualization - Aug 2020 - [PATCH v2] virtio-rng: return available data with O_NONBLOCK

On 11/08/2020 14:53, Martin Wilck wrote:
> On Tue, 2020-08-11 at 14:39 +0200, Laurent Vivier wrote:
>> On 11/08/2020 14:22, Martin Wilck wrote:
>>> On Tue, 2020-08-11 at 14:02 +0200, Laurent Vivier wrote:
>>>>>>  drivers/char/hw_random/virtio-rng.c | 14
++++++++++++++
>>>>>>  1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/char/hw_random/virtio-rng.c
>>>>>> b/drivers/char/hw_random/virtio-rng.c
>>>>>> index 79a6e47b5fbc..984713b35892 100644
>>>>>> --- a/drivers/char/hw_random/virtio-rng.c
>>>>>> +++ b/drivers/char/hw_random/virtio-rng.c
>>>>>> @@ -59,6 +59,20 @@ static int virtio_read(struct hwrng
*rng,
>>>>>> void
>>>>>> *buf, size_t size, bool wait)
>>>>>>  	if (vi->hwrng_removed)
>>>>>>  		return -ENODEV;
>>>>>>  
>>>>>> +	/*
>>>>>> +	 * If the previous call was non-blocking, we may have
>>>>>> got some
>>>>>> +	 * randomness already.
>>>>>> +	 */
>>>>>> +	if (vi->busy &&
completion_done(&vi->have_data)) {
>>>>>> +		unsigned int len;
>>>>>> +
>>>>>> +		vi->busy = false;
>>>>>> +		len = vi->data_avail > size ? size : vi-
>>>>>>> data_avail;
>>>>>> +		vi->data_avail -= len;
>>>>
>>>> You don't need to modify data_avail. As busy is set to
false, the
>>>> buffer
>>>> will be reused. and it is always overwritten by
>>>> virtqueue_get_buf().
>>>> And moreover, if it was reused it would be always the
beginning.
>>>
>>> Ok.
>>>
>>>>>> +		if (len)
>>>>>> +			return len;
>>>>>> +	}
>>>>>> +
>>>>>>  	if (!vi->busy) {
>>>>>>  		vi->busy = true;
>>>>>>  		reinit_completion(&vi->have_data);
>>>>>>
>>>>
>>>> Why don't you modify only the wait case?
>>>>
>>>> Something like:
>>>>
>>>> 	if (!wait && !completion_done(&vi->have_data))
{
>>>> 		return 0;
>>>>         }
>>>>
>>>> then at the end you can do "return min(size,
vi->data_avail);".
>>>
>>> Sorry, I don't understand what you mean. Where would you insert
the
>>> above "if" clause? Are you saying I should call
>>> wait_for_completion_killable() also in the (!wait) case?
>>
>> Yes, but only if a the completion is done, so it will not wait.
>>
> 
> Slowly getting there, thanks for your patience. Yes, I guess this would
> work, too. I'll test and get back to you.
No problem. This code is tricky and it took me several months to really
start to understand it ...

Thanks,
Laurent

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?

-- 
MST

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