Re: [PATCH v2] util/memfd: allow allocating 0 bytes

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, May 07, 2025 at 01:25:34AM +0300, Elisha Hollander wrote:
> Maybe an assert is really more appropriate, but technically doing so on
> actual hardware should run flawlessly so I think the emu should work too...
> Maybe I'm wrong though

I'm still not clear which specific hardware device you're talking
about, but even if we don't want to assert, there's likely scope
for addressing the problem in that specific device rather than
changing the memfd code which has a semantic impact across all
users in qemu.

> 
> On Tue, May 6, 2025, 19:48 Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Tue, May 06, 2025 at 07:41:32PM +0300, Elisha Hollander wrote:
> > > Gave an example for a case where QEMU would try to allocate 0 bytes thus
> > > fail here in the original version of the patch.
> > >
> > > > As I mentioned earlier, let's say you don't initialize the vertical
> > > display end registers, and set the minimum scanline register, the
> > emulation
> > > will then have to allocate some display buffer, but because the vertical
> > > display end is initilized as 0 the buffer will be empty and the program
> > > break.
> >
> > Isn't this an invalid hardware configuration that should be detected
> > in the emulation code, and either force the display end to a minimum
> > value, or trigger an assert ?
> >
> > Patching a bug in a specific HW impl, by changing the qemu_memfd_alloc
> > code feels like it is probably the wrong place to address this.
> >
> > >
> > > I have no idea as for why my emails are getting messed up... :/
> > >
> > > Have to go now, will try and send it again tomorrow probably...
> > >
> > > On Tue, May 6, 2025, 19:37 Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> > >
> > > > On Tue, May 06, 2025 at 07:17:25PM +0300, Elisha Hollander wrote:
> > > > > Sorry for former patch something is messed up with my email.
> > > >
> > > > The commit message needs to explain what problem is being solved by
> > > > making this change as allowing 0 bytes looks dubious on the surface.
> > > >
> > > > >
> > > > > Signed-off-by: donno2048 <just4now666666@gmail.com>
> > > > > ---
> > > > >  util/memfd.c | 10 ++++++----
> > > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/util/memfd.c b/util/memfd.c
> > > > > index 8a2e906..e96e5af 100644
> > > > > --- a/util/memfd.c
> > > > > +++ b/util/memfd.c
> > > > > @@ -108,7 +108,7 @@ err:
> > > > >  void *qemu_memfd_alloc(const char *name, size_t size, unsigned int
> > > > seals,
> > > > >                         int *fd, Error **errp)
> > > > >  {
> > > > > - void *ptr;
> > > > > + void *ptr = NULL;
> > > > >      int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);
> > > > >
> > > > >      /* some systems have memfd without sealing */
> > > > > @@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t
> > > > size,
> > > > > unsigned int seals,
> > > > >          }
> > > > >      }
> > > > >
> > > > > - ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > > > > - if (ptr == MAP_FAILED) {
> > > > > - goto err;
> > > > > + if (size != 0) {
> > > > > + ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > > > > + if (ptr == MAP_FAILED) {
> > > > > + goto err;
> > > > > + }
> > > > >      }
> > > >
> > > > This patch is mangled.
> > > >
> > > >
> > > > With regards,
> > > > Daniel
> > > > --
> > > > |: https://berrange.com      -o-
> > > > https://www.flickr.com/photos/dberrange :|
> > > > |: https://libvirt.org         -o-
> > > > https://fstop138.berrange.com :|
> > > > |: https://entangle-photo.org    -o-
> > > > https://www.instagram.com/dberrange :|
> > > >
> > > >
> >
> > With regards,
> > Daniel
> > --
> > |: https://berrange.com      -o-
> > https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org         -o-
> > https://fstop138.berrange.com :|
> > |: https://entangle-photo.org    -o-
> > https://www.instagram.com/dberrange :|
> >
> >

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|