Re: [Qemu-devel] [PATCH v2] rng-builtin: add an RNG backend that uses qemu_guest_getrandom()

[Amit Shah]

On (Fri) 10 May 2019 [17:19:12], Markus Armbruster wrote:
> Laurent Vivier <address@hidden> writes:
> 
> > On 10/05/2019 14:27, Markus Armbruster wrote:
> >> Laurent Vivier <address@hidden> writes:
> >>
> >>> Add a new RNG backend using QEMU builtin getrandom function.
> >>>
> >>> It can be created and used with something like:
> >>>
> >>>      ... -object rng-builtin,id=rng0 -device virtio-rng,rng=rng0 ...
> >>>
> >>> Reviewed-by: Richard Henderson <address@hidden>
> >>> Signed-off-by: Laurent Vivier <address@hidden>
> >>> ---
> >>>
> >>> Notes:
> >>>      This patch applies on top of
> >>>      "[PATCH v5 00/24] Add qemu_getrandom and ARMv8.5-RNG etc"
> >>>      Based-on: address@hidden
> >>>           v2: Update qemu-options.hx
> >>>          describe the new backend and specify virtio-rng uses the
> >>>          rng-random by default (do we want to change this?)
> >>>
> >>>   backends/Makefile.objs |  2 +-
> >>>   backends/rng-builtin.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> >>>   qemu-options.hx        | 10 +++++++-
> >>>   3 files changed, 66 insertions(+), 2 deletions(-)
> >>>   create mode 100644 backends/rng-builtin.c
> >>>
> >>> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> >>> index ff619d31b461..8da4a508d97b 100644
> >>> --- a/backends/Makefile.objs
> >>> +++ b/backends/Makefile.objs
> >>> @@ -1,4 +1,4 @@
> >>> -common-obj-y += rng.o rng-egd.o
> >>> +common-obj-y += rng.o rng-egd.o rng-builtin.o
> >>>   common-obj-$(CONFIG_POSIX) += rng-random.o
> >>>     common-obj-$(CONFIG_TPM) += tpm.o
> >>> diff --git a/backends/rng-builtin.c b/backends/rng-builtin.c
> >>> new file mode 100644
> >>> index 000000000000..b1264b745407
> >>> --- /dev/null
> >>> +++ b/backends/rng-builtin.c
> >>> @@ -0,0 +1,56 @@
> >>> +/*
> >>> + * QEMU Builtin Random Number Generator Backend
> >>> + *
> >>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
> >>> later.
> >>> + * See the COPYING file in the top-level directory.
> >>> + */
> >>> +
> >>> +#include "qemu/osdep.h"
> >>> +#include "sysemu/rng.h"
> >>> +#include "qapi/error.h"
> >>> +#include "qapi/qmp/qerror.h"
> >>> +#include "qemu/main-loop.h"
> >>> +#include "qemu/guest-random.h"
> >>> +
> >>> +#define TYPE_RNG_BUILTIN "rng-builtin"
> >>> +#define RNG_BUILTIN(obj) OBJECT_CHECK(RngBuiltin, (obj), 
> >>> TYPE_RNG_BUILTIN)
> >>> +
> >>> +typedef struct RngBuiltin {
> >>> +    RngBackend parent;
> >>> +} RngBuiltin;
> >>> +
> >>> +static void rng_builtin_request_entropy(RngBackend *b, RngRequest *req)
> >>> +{
> >>> +    RngBuiltin *s = RNG_BUILTIN(b);
> >>> +
> >>> +    while (!QSIMPLEQ_EMPTY(&s->parent.requests)) {
> >>> +        RngRequest *req = QSIMPLEQ_FIRST(&s->parent.requests);
> >>> +
> >>> +        qemu_guest_getrandom_nofail(req->data, req->size);
> >>> +
> >>> +        req->receive_entropy(req->opaque, req->data, req->size);
> >>> +
> >>> +        rng_backend_finalize_request(&s->parent, req);
> >>> +    }
> >>> +}
> >>> +
> >>> +static void rng_builtin_class_init(ObjectClass *klass, void *data)
> >>> +{
> >>> +    RngBackendClass *rbc = RNG_BACKEND_CLASS(klass);
> >>> +
> >>> +    rbc->request_entropy = rng_builtin_request_entropy;
> >>> +}
> >>> +
> >>> +static const TypeInfo rng_builtin_info = {
> >>> +    .name = TYPE_RNG_BUILTIN,
> >>> +    .parent = TYPE_RNG_BACKEND,
> >>> +    .instance_size = sizeof(RngBuiltin),
> >>> +    .class_init = rng_builtin_class_init,
> >>> +};
> >>> +
> >>> +static void register_types(void)
> >>> +{
> >>> +    type_register_static(&rng_builtin_info);
> >>> +}
> >>> +
> >>> +type_init(register_types);
> >>> diff --git a/qemu-options.hx b/qemu-options.hx
> >>> index 0191ef8b1eb7..3e2a51c691b0 100644
> >>> --- a/qemu-options.hx
> >>> +++ b/qemu-options.hx
> >>> @@ -4280,13 +4280,21 @@ other options.
> >>>     The @option{share} boolean option is @var{on} by default with
> >>> memfd.
> >>>   address@hidden -object rng-builtin,address@hidden
> >>> +
> >>> +Creates a random number generator backend which obtains entropy from
> >>> +QEMU builtin functions. The @option{id} parameter is a unique ID that
> >>> +will be used to reference this entropy backend from the 
> >>> @option{virtio-rng}
> >>> +device.
> >>> +
> >>>   @item -object rng-random,address@hidden,address@hidden/dev/random}
> >>>     Creates a random number generator backend which obtains entropy
> >>> from
> >>>   a device on the host. The @option{id} parameter is a unique ID that
> >>>   will be used to reference this entropy backend from the 
> >>> @option{virtio-rng}
> >>>   device.
> >>
> >> There's also the "spapr-rng" device, I think.
> >
> > spapr-rng doesn't have default. You must specify one to be able to use it:
> >    qemu-system-ppc64: -device spapr-rng: spapr-rng needs an RNG backend!
> 
> You're right.
> 
> >>>           The @option{filename} parameter specifies which file to obtain
> >>> -entropy from and if omitted defaults to @option{/dev/random}.
> >>> +entropy from and if omitted defaults to @option{/dev/random}. By default,
> >>> +the @option{virtio-rng} device uses this RNG backend.
> >>>     @item -object rng-egd,address@hidden,address@hidden
> >>
> >> Trivial conflict with Kashyap's "[PATCH v2] VirtIO-RNG: Update default
> >> entropy source to `/dev/urandom`".
> >>
> >> virtio-rng indeed creates an rng-random backend when the user doesn't
> >> specify one.  I consider having device model frontends create backends a
> >> bad idea.  Not this patch's fault, of course.
> >>
> >> That said, would rng-builtin be a better default?  For starters, it's
> >> available when !CONFIG_POSIX.  I suspect virtio-rng crashes when it
> >> tries to create an rng-random that isn't available.
> >
> > I will send a v3 with rng-builtin as a default. Maintainer will be
> > able to pick one of his choice, v2 or v3.
> >
> >>
> >> The new rng-builtin is considerably simpler than both rng-random and
> >> rng-egd.  Moreover, it just works, whereas rng-random is limited to
> >> CONFIG_POSIX, and rng-egd needs egd running (which I suspect basically
> >> nobody does).  Have we considered deprecating these two backends in
> >> favor of rng-builtin?
> >
> > I have several bugzilla involving these backends: as there are
> > blocking, the virtio-rng device in the guest can hang, or crash during
> > hot-unplug. From my point of view, life would be easier without
> > them...
> 
> Sounds like perfectly fine reasons for deprecating them.  Amit, what do
> you think?

The egd backend wasn't too useful - so I don't mind deprecating it
using the usual deprecation notice.

The rng-random backend can stay with the multiple fallbacks as
discussed in the other threads - with getrandom() being the most
preferred one on Linux.  BSDs have /dev/srandom which is quite similar
to getrandom(), but that can be an additional backend that can come later.

Overall, I like the way these series turned out..


                Amit
-- 
http://amitshah.net/