Re: [Qemu-devel] [PATCH 1/4] hostmem-memfd: enable seals only if supported

[Marc-André Lureau]

On Tue, Nov 27, 2018 at 5:07 PM Ilya Maximets <address@hidden> wrote:
>
> On 27.11.2018 15:56, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Nov 27, 2018 at 4:37 PM Ilya Maximets <address@hidden> wrote:
> >>
> >> On 27.11.2018 15:29, Marc-André Lureau wrote:
> >>> Hi
> >>>
> >>> On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <address@hidden> wrote:
> >>>>
> >>>> On 27.11.2018 15:00, Marc-André Lureau wrote:
> >>>>> Hi
> >>>>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <address@hidden> wrote:
> >>>>>>
> >>>>>> On 27.11.2018 14:49, Marc-André Lureau wrote:
> >>>>>>> Hi
> >>>>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <address@hidden> wrote:
> >>>>>>>>
> >>>>>>>> If seals are not supported, memfd_create() will fail.
> >>>>>>>> Furthermore, there is no way to disable it in this case because
> >>>>>>>> '.seal' property is not registered.
> >>>>>>>>
> >>>>>>>> This issue leads to vhost-user-test failures on RHEL 7.2:
> >>>>>>>>
> >>>>>>>>   qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \
> >>>>>>>>                       failed to create memfd: Invalid argument
> >>>>>>>>
> >>>>>>>> Signed-off-by: Ilya Maximets <address@hidden>
> >>>>>>>
> >>>>>>>
> >>>>>>> This will change the default behaviour of memfd backend, and may thus
> >>>>>>> me considered a break.
> >>>>>>
> >>>>>> This will change the default behaviour only on systems without sealing
> >>>>>> support. But current implementation is broken there anyway and does not
> >>>>>> work.
> >>>>>>
> >>>>>>>
> >>>>>>> Instead, memfd vhost-user-test should skipped (or tuned with
> >>>>>>> sealed=false if unsupported)
> >>>>>>
> >>>>>> vhost-user-test is just an example here. In fact memfd could not be
> >>>>>> used at all on system without sealing support. And there is no way
> >>>>>> to disable seals.
> >>>>>
> >>>>> which system supports memfd without sealing?
> >>>>
> >>>> RHEL 7.2. kernel version 3.10.0-327.el7.x86_64
> >>>
> >>> Correct, it was backported without sealing for some reason.
> >>>
> >>> I would rather have an explicit seal=off argument on such system
> >>> (because sealing is expected to be available with memfd in general)
> >>>
> >>
> >> But '.seal' property registering is guarded by 
> >> 'qemu_memfd_check(MFD_ALLOW_SEALING)'.
> >> And you can not disable it:
> >>
> >> qemu-system-x86_64: -object memory-backend-memfd,seal=off,id=mem,size=2M,: 
> >> Property '.seal' not found
> >
> > Right
> >
> >>
> >> Enabling this option on system that does not support sealing will
> >> probably break some libvirt feature discovering or something similar.
> >>
> >> What about adding some warning to 'memfd_backend_instance_init' if
> >> sealing not supported before disabling it ?
> >
> > What do you think of Gerd suggestion, and disable memfd backend if
> > sealing is not available? (the sealing property check can be removed
> > then).
>
> It's OK in general for me.
> What about something like this:
>
> ---
> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> index ee39bdbde6..a3455da9c9 100644
> --- a/backends/hostmem-memfd.c
> +++ b/backends/hostmem-memfd.c
> @@ -156,15 +156,13 @@ memfd_backend_class_init(ObjectClass *oc, void *data)
>                                                "Huge pages size (ex: 2M, 1G)",
>                                                &error_abort);
>      }
> -    if (qemu_memfd_check(MFD_ALLOW_SEALING)) {
> -        object_class_property_add_bool(oc, "seal",
> -                                       memfd_backend_get_seal,
> -                                       memfd_backend_set_seal,
> -                                       &error_abort);
> -        object_class_property_set_description(oc, "seal",
> -                                              "Seal growing & shrinking",
> -                                              &error_abort);
> -    }
> +    object_class_property_add_bool(oc, "seal",
> +                                   memfd_backend_get_seal,
> +                                   memfd_backend_set_seal,
> +                                   &error_abort);
> +    object_class_property_set_description(oc, "seal",
> +                                          "Seal growing & shrinking",
> +                                          &error_abort);
>  }
>
>  static const TypeInfo memfd_backend_info = {
> @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = {
>
>  static void register_types(void)
>  {
> -    if (qemu_memfd_check(0)) {
> +    if (qemu_memfd_check(MFD_ALLOW_SEALING)) {
>          type_register_static(&memfd_backend_info);
>      }
>  }
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 45d58d8ea2..e3e9a33580 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -169,7 +169,7 @@ static char *get_qemu_cmd(TestServer *s,
>                            int mem, enum test_memfd memfd, const char 
> *mem_path,
>                            const char *chr_opts, const char *extra)
>  {
> -    if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(0)) {
> +    if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(MFD_ALLOW_SEALING)) {
>          memfd = TEST_MEMFD_YES;
>      }
>
> @@ -903,7 +903,7 @@ static void test_multiqueue(void)
>      s->queues = 2;
>      test_server_listen(s);
>
> -    if (qemu_memfd_check(0)) {
> +    if (qemu_memfd_check(MFD_ALLOW_SEALING)) {
>          cmd = g_strdup_printf(
>              QEMU_CMD_MEMFD QEMU_CMD_CHR QEMU_CMD_NETDEV ",queues=%d "
>              "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d",
> @@ -963,7 +963,7 @@ int main(int argc, char **argv)
>      /* run the main loop thread so the chardev may operate */
>      thread = g_thread_new(NULL, thread_function, loop);
>
> -    if (qemu_memfd_check(0)) {
> +    if (qemu_memfd_check(MFD_ALLOW_SEALING)) {
>          qtest_add_data_func("/vhost-user/read-guest-mem/memfd",
>                              GINT_TO_POINTER(TEST_MEMFD_YES),
>                              test_read_guest_mem);
>

looks fine, waiting for your v2 series

> ---
>
> ?
>
> >
> >>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>> ---
> >>>>>>>>  backends/hostmem-memfd.c | 4 ++--
> >>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> >>>>>>>> index b6836b28e5..ee39bdbde6 100644
> >>>>>>>> --- a/backends/hostmem-memfd.c
> >>>>>>>> +++ b/backends/hostmem-memfd.c
> >>>>>>>> @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj)
> >>>>>>>>  {
> >>>>>>>>      HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj);
> >>>>>>>>
> >>>>>>>> -    /* default to sealed file */
> >>>>>>>> -    m->seal = true;
> >>>>>>>> +    /* default to sealed file if supported */
> >>>>>>>> +    m->seal = qemu_memfd_check(MFD_ALLOW_SEALING);
> >>>>>>>>  }
> >>>>>>>>
> >>>>>>>>  static void
> >>>>>>>> --
> >>>>>>>> 2.17.1
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >



-- 
Marc-André Lureau