Re: [PATCH] Open file as read only on private mapping in qemu_ram_alloc_from_file

[David Hildenbrand <david@redhat.com>]

Hi,

patch subject should start with "softmmu/physmem: Open ..."

On 25.07.23 12:52, Thiner Logoer wrote:
> An read only file can be mapped with read write as long as the
> mapping is private, which is very common case. Make

At least in the environments I know, using private file mappings is a corner case ;)

What is you use case? VM templating?

> qemu_ram_alloc_from_file open file as read only when the
> mapping is private, otherwise open will fail when file
> does not allow write.
> 
> If this file does not exist or is a directory, the flag is not used,
> so it should be OK.
> 
> from https://gitlab.com/qemu-project/qemu/-/issues/1689
> 
> Signed-off-by: Thiner Logoer <logoerthiner1@163.com>
> ---
>   softmmu/physmem.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 3df73542e1..e8036ee335 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1945,8 +1945,15 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>       int fd;
>       bool created;
>       RAMBlock *block;
> +

^

.git/rebase-apply/patch:13: trailing whitespace.

> +    /*
> +     * If map is private, the fd does not need to be writable.
> +     * This only get effective when the file is existent.

"This will get ignored if the file does not yet exist."

> +     */
> +    bool open_as_readonly = readonly || !(ram_flags & RAM_SHARED);
>   
> -    fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created,
> +    fd = file_ram_open(mem_path, memory_region_name(mr),
> +                       open_as_readonly, &created,
>                          errp);
>       if (fd < 0) {
>           return NULL;


Opening a file R/O will also make operations like fallocate/ftruncate/ ... fail.

For example, this will make fallocate(FALLOC_FL_PUNCH_HOLE) stop working and in
turn make ram_block_discard_range() bail out.


There was a recent discussion/patch on that:

commit 1d44ff586f8a8e113379430750b5a0a2a3f64cf9
Author: David Hildenbrand <david@redhat.com>
Date:   Thu Jul 6 09:56:06 2023 +0200

     softmmu/physmem: Warn with ram_block_discard_range() on MAP_PRIVATE file mapping
     
     ram_block_discard_range() cannot possibly do the right thing in
     MAP_PRIVATE file mappings in the general case.
     
     To achieve the documented semantics, we also have to punch a hole into
     the file, possibly messing with other MAP_PRIVATE/MAP_SHARED mappings
     of such a file.
     
     For example, using VM templating -- see commit b17fbbe55cba ("migration:
     allow private destination ram with x-ignore-shared") -- in combination with
     any mechanism that relies on discarding of RAM is problematic. This
     includes:
     * Postcopy live migration
     * virtio-balloon inflation/deflation or free-page-reporting
     * virtio-mem
     
     So at least warn that there is something possibly dangerous is going on
     when using ram_block_discard_range() in these cases.


While it doesn't work "in the general case", it works in the "single file owner" case
where someone simply forgot to specify "share=on" -- "share=off" is the default for
memory-backend-file :( .


For example, with hugetlb+virtio-mem the following works if the file does not exists:

(note that virtio-mem will fallocate(FALLOC_FL_PUNCH_HOLE) the whole file upfront)

...
-object memory-backend-file,share=off,mem-path=/dev/hugepages/vmem0,id=mem2,size=2G \
-device virtio-mem-pci,id=vmem0,memdev=mem2,requested-size=1g,bus=root


With you patch, once the file already exists, we would now get

qemu-system-x86_64: -device virtio-mem-pci,id=vmem0,memdev=mem2,requested-size=1g,bus=root: ram_block_discard_range: Failed to fallocate :0 +80000000 (-9)
qemu-system-x86_64: -device virtio-mem-pci,id=vmem0,memdev=mem2,requested-size=1g,bus=root: Unexpected error discarding RAM: Bad file descriptor


So this has the potential to break existing setups.

The easy fix for these would be to configure "share=on" in these now-failing setups. Hmmmmm ....

-- 
Cheers,

David / dhildenb