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

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