Re:Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[ThinerLogoer]

At 2023-08-09 05:01:17, "Peter Xu" <peterx@redhat.com> wrote:
>On Mon, Aug 07, 2023 at 09:07:32PM +0200, David Hildenbrand wrote:
>> From: Thiner Logoer <logoerthiner1@163.com>
>> 
>> Users may specify
>> * "-mem-path" or
>> * "-object memory-backend-file,share=off,readonly=off"
>> and expect such COW (MAP_PRIVATE) mappings to work, even if the user
>> does not have write permissions to open the file.
>> 
>> For now, we would always fail in that case, always requiring file write
>> permissions. Let's detect when that failure happens and fallback to opening
>> the file readonly.
>> 
>> Warn the user, since there are other use cases where we want the file to
>> be mapped writable: ftruncate() and fallocate() will fail if the file
>> was not opened with write permissions.
>> 
>> Signed-off-by: Thiner Logoer <logoerthiner1@163.com>
>> Co-developed-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  softmmu/physmem.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>> 
>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index 3df73542e1..d1ae694b20 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -1289,8 +1289,7 @@ static int64_t get_file_align(int fd)
>>  static int file_ram_open(const char *path,
>>                           const char *region_name,
>>                           bool readonly,
>> -                         bool *created,
>> -                         Error **errp)
>> +                         bool *created)
>>  {
>>      char *filename;
>>      char *sanitized_name;
>> @@ -1334,10 +1333,7 @@ static int file_ram_open(const char *path,
>>              g_free(filename);
>>          }
>>          if (errno != EEXIST && errno != EINTR) {
>> -            error_setg_errno(errp, errno,
>> -                             "can't open backing store %s for guest RAM",
>> -                             path);
>> -            return -1;
>> +            return -errno;
>>          }
>>          /*
>>           * Try again on EINTR and EEXIST.  The latter happens when
>> @@ -1946,9 +1942,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, 
>> MemoryRegion *mr,
>>      bool created;
>>      RAMBlock *block;
>>  
>> -    fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created,
>> -                       errp);
>> +    fd = file_ram_open(mem_path, memory_region_name(mr), readonly, 
>> &created);
>> +    if (fd == -EACCES && !(ram_flags & RAM_SHARED) && !readonly) {
>> +        /*
>> +         * We can have a writable MAP_PRIVATE mapping of a readonly file.
>> +         * However, some operations like ftruncate() or fallocate() might 
>> fail
>> +         * later, let's warn the user.
>> +         */
>> +        fd = file_ram_open(mem_path, memory_region_name(mr), true, 
>> &created);
>> +        if (fd >= 0) {
>> +            warn_report("backing store %s for guest RAM (MAP_PRIVATE) 
>> opened"
>> +                        " readonly because the file is not writable", 
>> mem_path);
>
>I can understand the use case, but this will be slightly unwanted,
>especially the user doesn't yet have a way to predict when will it happen.
>
>Meanwhile this changes the behavior, is it a concern that someone may want
>to rely on current behavior of failing?
>

I am happy to learn if there is some solid evidence supporting this view.

The target of compatibility is "private+discard" which seems itself pathological
practice in early discussion. The only difference in behavior that might be 
unwanted
in your argument lies in "readonly file+private+discard" failure time. Before 
the
patch it fails early, after the patch it fails later and may does additional 
stuff.
Do you think that a bug reporting mechanism which relies on qemu failure
timing is valid?

If someone argues that "readonly file+private+discard" early failure behavior
is all where their system relies, I would be happy to learn why. Actually this
is much easier to solve outside qemu, by checking memory file permission,
compared to the "readonly+private" alternative plan that requires a btrfs.

In the long run the "private+discard" setup may itself be warned and
deprecated, rather than "private+readonly file" which is supported by
linux kernel.

Current patch is already a compromise considering compatibility, as it
always tries open the file in readwrite mode first, to persist behavior on
"readwrite+private+discard" case. Personally I prefer to try opening the file
readonly first, as this will reduce the risk of opening ram file accidentally
with write permission.

However I can accept a second level of compromise, aka adding 
"-disallow-private-discard"
to qemu argument for at least three qemu releases before "private+discard"
get deprecated and removed, if extreme compatibility enthusiast is involved.
With this argument, readonly private is enabled and private
discard fails immediately when discard request is detected, and without this
argument readonly private is disabled and the behavior is unchanged.
This argument is useful also because it helps
finding out existent "private+discard" behavior and assists debugging.
Do you think that this solution pays off?
(I am no expert qemu programmer, hope anyone else to handle the command
line arguments if this is preferred)

>To think from a higher level of current use case, the ideal solution seems
>to me that if the ram file can be put on a file system that supports CoW
>itself (like btrfs), we can snapshot that ram file and make it RW for the
>qemu instance. Then here it'll be able to open the file.  We'll be able to
>keep the interface working as before, meanwhile it'll work with fallocate
>or truncations too I assume.
>
>Would that be better instead of changing QEMU?
>

I am afraid that your alternative solution will make it still impossible
for the use case to be used on rootless system; while MAP_PRIVATE
based CoW works anywhere. For example this will not work on ext4 with
readonly file mode, or that the file is owned by root and you are
normal user, and on squashfs. Basically everything on the machine
should be readonly and you are also only a nobody, then
only the in-qemu choice is possible.

I believe the warning that the file is opened readonly is enough
for potential user to consider doing otherwise. Do you think that
looking solely at qemu exit status and pipe all qemu stderr log to /dev/null
all the time is one of the supported use cases?

--

Regards,

logoerthiner