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

[David Hildenbrand]

On 17.08.23 15:37, Daniel P. Berrangé 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);
>
> IIUC, from the description, the goal is that usage of a readonly
> backing store is intented to be an explicitly supported deployment
> configuration. At the time time though, this scenario could also be
> a deployment mistake that we want to diagnose

FWIW, I abandoned this approach here and instead will look into making

memory-backend-file,readonly=on,share=off

create RAM instead of ROM.

The fallback was wrong after realizing what "readonly" actually is 
supposed to do.

I stared at libvirt, an even it never seems to set readonly=on for R/O 
DIMMs, so you always get RAM and then tell the nvdimm device to not 
perform any writes (unarmed=on)

--
Cheers,

David / dhildenb