[Qemu-devel] [PATCH 0/4] memfd fixes.

[Qemu-devel] [PATCH 0/4] memfd fixes.

[Ilya Maximets]

Ilya Maximets (4):
  hostmem-memfd: enable seals only if supported
  memfd: always check for MFD_CLOEXEC
  memfd: set up correct errno if not supported
  memfd: improve error messages

 backends/hostmem-memfd.c |  4 ++--
 util/memfd.c             | 10 ++++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.17.1

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

[Ilya Maximets]

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 <i.maximets@samsung.com>
---
 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

[Qemu-devel] [PATCH 2/4] memfd: always check for MFD_CLOEXEC

[Ilya Maximets]

QEMU always sets this flag unconditionally. We need to
check if it's supported.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 util/memfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/memfd.c b/util/memfd.c
index 8debd0d037..d74ce4d793 100644
--- a/util/memfd.c
+++ b/util/memfd.c
@@ -188,7 +188,7 @@ bool qemu_memfd_alloc_check(void)
 bool qemu_memfd_check(unsigned int flags)
 {
 #ifdef CONFIG_LINUX
-    int mfd = memfd_create("test", flags);
+    int mfd = memfd_create("test", flags | MFD_CLOEXEC);
 
     if (mfd >= 0) {
         close(mfd);
-- 
2.17.1

[Qemu-devel] [PATCH 3/4] memfd: set up correct errno if not supported

[Ilya Maximets]

qemu_memfd_create() prints the value of 'errno' which is not
set in this case.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 util/memfd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/memfd.c b/util/memfd.c
index d74ce4d793..393d23da96 100644
--- a/util/memfd.c
+++ b/util/memfd.c
@@ -40,6 +40,7 @@ static int memfd_create(const char *name, unsigned int flags)
 #ifdef __NR_memfd_create
     return syscall(__NR_memfd_create, name, flags);
 #else
+    errno = ENOSYS;
     return -1;
 #endif
 }
-- 
2.17.1

[Qemu-devel] [PATCH 4/4] memfd: improve error messages

[Ilya Maximets]

This gives more information about the failure.
Additionally 'ENOSYS' returned for a non-Linux platforms instead of
'errno', which is not initilaized in this case.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 util/memfd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/util/memfd.c b/util/memfd.c
index 393d23da96..00334e5b21 100644
--- a/util/memfd.c
+++ b/util/memfd.c
@@ -71,14 +71,18 @@ int qemu_memfd_create(const char *name, size_t size, bool hugetlb,
     }
     mfd = memfd_create(name, flags);
     if (mfd < 0) {
+        error_setg_errno(errp, errno,
+                         "failed to create memfd with flags 0x%x", flags);
         goto err;
     }
 
     if (ftruncate(mfd, size) == -1) {
+        error_setg_errno(errp, errno, "failed to resize memfd to %zu", size);
         goto err;
     }
 
     if (seals && fcntl(mfd, F_ADD_SEALS, seals) == -1) {
+        error_setg_errno(errp, errno, "failed to add seals 0x%x", seals);
         goto err;
     }
 
@@ -88,8 +92,9 @@ err:
     if (mfd >= 0) {
         close(mfd);
     }
+#else
+    error_setg_errno(errp, ENOSYS, "failed to create memfd");
 #endif
-    error_setg_errno(errp, errno, "failed to create memfd");
     return -1;
 }
 
-- 
2.17.1

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

[Marc-André Lureau]

Hi
On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> 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 <i.maximets@samsung.com>


This will change the default behaviour of memfd backend, and may thus
me considered a break.

Instead, memfd vhost-user-test should skipped (or tuned with
sealed=false if unsupported)

> ---
>  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
>

Re: [Qemu-devel] [PATCH 2/4] memfd: always check for MFD_CLOEXEC

[Marc-André Lureau]

On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> QEMU always sets this flag unconditionally. We need to
> check if it's supported.
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

I don't think that flag is optional, but it doesn't hurt to check it as well
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  util/memfd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/memfd.c b/util/memfd.c
> index 8debd0d037..d74ce4d793 100644
> --- a/util/memfd.c
> +++ b/util/memfd.c
> @@ -188,7 +188,7 @@ bool qemu_memfd_alloc_check(void)
>  bool qemu_memfd_check(unsigned int flags)
>  {
>  #ifdef CONFIG_LINUX
> -    int mfd = memfd_create("test", flags);
> +    int mfd = memfd_create("test", flags | MFD_CLOEXEC);
>
>      if (mfd >= 0) {
>          close(mfd);
> --
> 2.17.1
>

Re: [Qemu-devel] [PATCH 3/4] memfd: set up correct errno if not supported

[Marc-André Lureau]

On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> qemu_memfd_create() prints the value of 'errno' which is not
> set in this case.
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  util/memfd.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/util/memfd.c b/util/memfd.c
> index d74ce4d793..393d23da96 100644
> --- a/util/memfd.c
> +++ b/util/memfd.c
> @@ -40,6 +40,7 @@ static int memfd_create(const char *name, unsigned int flags)
>  #ifdef __NR_memfd_create
>      return syscall(__NR_memfd_create, name, flags);
>  #else
> +    errno = ENOSYS;
>      return -1;
>  #endif
>  }
> --
> 2.17.1
>

Re: [Qemu-devel] [PATCH 4/4] memfd: improve error messages

[Marc-André Lureau]

On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> This gives more information about the failure.
> Additionally 'ENOSYS' returned for a non-Linux platforms instead of
> 'errno', which is not initilaized in this case.
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  util/memfd.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/util/memfd.c b/util/memfd.c
> index 393d23da96..00334e5b21 100644
> --- a/util/memfd.c
> +++ b/util/memfd.c
> @@ -71,14 +71,18 @@ int qemu_memfd_create(const char *name, size_t size, bool hugetlb,
>      }
>      mfd = memfd_create(name, flags);
>      if (mfd < 0) {
> +        error_setg_errno(errp, errno,
> +                         "failed to create memfd with flags 0x%x", flags);
>          goto err;
>      }
>
>      if (ftruncate(mfd, size) == -1) {
> +        error_setg_errno(errp, errno, "failed to resize memfd to %zu", size);
>          goto err;
>      }
>
>      if (seals && fcntl(mfd, F_ADD_SEALS, seals) == -1) {
> +        error_setg_errno(errp, errno, "failed to add seals 0x%x", seals);
>          goto err;
>      }
>
> @@ -88,8 +92,9 @@ err:
>      if (mfd >= 0) {
>          close(mfd);
>      }
> +#else
> +    error_setg_errno(errp, ENOSYS, "failed to create memfd");
>  #endif
> -    error_setg_errno(errp, errno, "failed to create memfd");
>      return -1;
>  }
>
> --
> 2.17.1
>

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

[Ilya Maximets]

On 27.11.2018 14:49, Marc-André Lureau wrote:
> Hi
> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> 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 <i.maximets@samsung.com>
> 
> 
> 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.

> 
>> ---
>>  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
>>
> 
> 

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

[Marc-André Lureau]

Hi
On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> On 27.11.2018 14:49, Marc-André Lureau wrote:
> > Hi
> > On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> 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 <i.maximets@samsung.com>
> >
> >
> > 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?

>
> >
> >> ---
> >>  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

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

[Ilya Maximets]

On 27.11.2018 15:00, Marc-André Lureau wrote:
> Hi
> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>>
>> On 27.11.2018 14:49, Marc-André Lureau wrote:
>>> Hi
>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> 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 <i.maximets@samsung.com>
>>>
>>>
>>> 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

> 
>>
>>>
>>>> ---
>>>>  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
>>>>
>>>
>>>
>>
> 
> 

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

[Marc-André Lureau]

Hi

On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> On 27.11.2018 15:00, Marc-André Lureau wrote:
> > Hi
> > On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> >>
> >> On 27.11.2018 14:49, Marc-André Lureau wrote:
> >>> Hi
> >>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> 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 <i.maximets@samsung.com>
> >>>
> >>>
> >>> 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)

>
> >
> >>
> >>>
> >>>> ---
> >>>>  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

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

[Ilya Maximets]

On 27.11.2018 15:29, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>>
>> On 27.11.2018 15:00, Marc-André Lureau wrote:
>>> Hi
>>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>
>>>> On 27.11.2018 14:49, Marc-André Lureau wrote:
>>>>> Hi
>>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> 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 <i.maximets@samsung.com>
>>>>>
>>>>>
>>>>> 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

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 ?

>>
>>>
>>>>
>>>>>
>>>>>> ---
>>>>>>  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
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
> 
> 
> 

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

[Gerd Hoffmann]

  Hi,

> > > 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)

Or just drop support for memfd without sealing.

cheers,
  Gerd

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

[Marc-André Lureau]

Hi

On Tue, Nov 27, 2018 at 4:37 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> On 27.11.2018 15:29, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> >>
> >> On 27.11.2018 15:00, Marc-André Lureau wrote:
> >>> Hi
> >>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> >>>>
> >>>> On 27.11.2018 14:49, Marc-André Lureau wrote:
> >>>>> Hi
> >>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> 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 <i.maximets@samsung.com>
> >>>>>
> >>>>>
> >>>>> 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).

>
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> ---
> >>>>>>  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

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

[Ilya Maximets]

On 27.11.2018 15:56, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Nov 27, 2018 at 4:37 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>>
>> On 27.11.2018 15:29, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>
>>>> On 27.11.2018 15:00, Marc-André Lureau wrote:
>>>>> Hi
>>>>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>>>
>>>>>> On 27.11.2018 14:49, Marc-André Lureau wrote:
>>>>>>> Hi
>>>>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> 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 <i.maximets@samsung.com>
>>>>>>>
>>>>>>>
>>>>>>> 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);

---

?

> 
>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>>  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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
> 

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 <i.maximets@samsung.com> wrote:
>
> On 27.11.2018 15:56, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Nov 27, 2018 at 4:37 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> >>
> >> On 27.11.2018 15:29, Marc-André Lureau wrote:
> >>> Hi
> >>>
> >>> On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> >>>>
> >>>> On 27.11.2018 15:00, Marc-André Lureau wrote:
> >>>>> Hi
> >>>>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote:
> >>>>>>
> >>>>>> On 27.11.2018 14:49, Marc-André Lureau wrote:
> >>>>>>> Hi
> >>>>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> 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 <i.maximets@samsung.com>
> >>>>>>>
> >>>>>>>
> >>>>>>> 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