Re: [PATCH kvmtool 0/2] Fix virtio/rng handling in low entropy situations

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 19 Apr 2023 16:10:13 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:

Hi Jean-Philippe,

thanks for having a look!

> On Wed, Apr 19, 2023 at 02:58:32PM +0100, Jean-Philippe Brucker wrote:
> > On Thu, Apr 13, 2023 at 05:57:55PM +0100, Andre Przywara wrote:  
> > > I am not sure we now really need patch 2 anymore (originally I had this
> > > one before I switched to /dev/urandom). I *think* even a read from
> > > /dev/urandom can return early (because of a signal, for instance), so
> > > a return with 0 bytes read seems possible.  
> > 
> > Given that this should be very rare, maybe a simple loop would be better
> > than switching the blocking mode?  It's certainly a good idea to apply the
> > "MUST" requirements from virtio.  

So originally I had this patch 2/2 on its own, still using /dev/random.
And there a read() on the O_NONBLOCKed fd would return -EAGAIN immediately
for the next 30 seconds straight, so doing this in a loop sounds very
wrong. After all blocking fd's are there to solve exactly that problem.

But indeed with /dev/urandom being much nicer to us already, and with the
below mentioned special behaviour, just a simple second try (no loop) is
sufficient.

> Digging a bit more, the manpage [1] is helpful:
> 
> 	The O_NONBLOCK flag has no effect when opening /dev/urandom.
> 	When calling read(2) for the device /dev/urandom, reads of up to
> 	256 bytes will return as many bytes as are requested and will not
> 	be interrupted by a signal handler. Reads with a buffer over
> 	this limit may return less than the requested number of bytes or
> 	fail with the error EINTR, if interrupted by a signal handler.

Right, I saw references to that behaviour on the Internet(TM), but missed
the manpage stanza. It still feels a bit awkward since this seems to rely
on some Linux implementation detail, but that's certainly fine for kvmtool
(being Linux only anyway).

> So I guess you can also drop the O_NONBLOCK flag in patch 1. And for the
> second one, maybe we could fallback to a 256 bytes read if the first one
> fails

Yes, that's certainly better and simplifies that patch.

Thanks for digging this out!

Cheers,
Andre

> 
> [1] https://man7.org/linux/man-pages/man4/urandom.4.html
>