Message ID | 20190510102637.10209-1-lvivier@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v2] rng-builtin: add an RNG backend that usesqemu_guest_getrandom() | expand |
On Fri, May 10, 2019 at 12:26:37PM +0200, Laurent Vivier wrote: > 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 <richard.henderson@linaro.org> > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > > Notes: > This patch applies on top of > "[PATCH v5 00/24] Add qemu_getrandom and ARMv8.5-RNG etc" > Based-on: 20190510012458.22706-1-richard.henderson@linaro.org > > 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?) Yeah, I think we could change the default backend, as it won't affect migration in any way > > 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 Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
Laurent Vivier <lvivier@redhat.com> 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 <richard.henderson@linaro.org> > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > > Notes: > This patch applies on top of > "[PATCH v5 00/24] Add qemu_getrandom and ARMv8.5-RNG etc" > Based-on: 20190510012458.22706-1-richard.henderson@linaro.org > > 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. > > +@item -object rng-builtin,id=@var{id} > + > +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,id=@var{id},filename=@var{/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. > 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,id=@var{id},chardev=@var{chardevid} 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. 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?
On 10/05/2019 14:27, Markus Armbruster wrote: > Laurent Vivier <lvivier@redhat.com> 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 <richard.henderson@linaro.org> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> >> Notes: >> This patch applies on top of >> "[PATCH v5 00/24] Add qemu_getrandom and ARMv8.5-RNG etc" >> Based-on: 20190510012458.22706-1-richard.henderson@linaro.org >> >> 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. >> >> +@item -object rng-builtin,id=@var{id} >> + >> +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,id=@var{id},filename=@var{/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! > >> 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,id=@var{id},chardev=@var{chardevid} > > 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... Thanks, Laurent
On Fri, May 10, 2019 at 02:37:41PM +0200, Laurent Vivier wrote: > On 10/05/2019 14:27, Markus Armbruster wrote: > > Laurent Vivier <lvivier@redhat.com> writes: > > 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... Are you sure about that ? The EGD impl looks like it is requesting entropy in an async manner. Any problem with rng-random would also affect rng-builtin, as depending on platform / build options, rng-builtin may just use /dev/urandom directly. It should only block with /dev/random really and that's only with Linux's impl of /dev/random - some OS effectively have /dev/random behave identically to /dev/urandom. Regards, Daniel
On 10/05/2019 17:32, Daniel P. Berrangé wrote: > On Fri, May 10, 2019 at 02:37:41PM +0200, Laurent Vivier wrote: >> On 10/05/2019 14:27, Markus Armbruster wrote: >>> Laurent Vivier <lvivier@redhat.com> writes: >>> 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... > > Are you sure about that ? > > The EGD impl looks like it is requesting entropy in an async manner. The virtio-rng driver waits until it receives enough entropy from the RNG backend while a mutex is taken. If the EGD daemon doesn't provide enough data to the RNG backend, virtio-rng driver can hang. It's easy to have if we start EGD backend with a socket in server,nowait mode and no EGD daemon connects to the port. Thanks, Laurent
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. +@item -object rng-builtin,id=@var{id} + +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,id=@var{id},filename=@var{/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. 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,id=@var{id},chardev=@var{chardevid}