Re: [PATCH V2] physmem: fix qemu_ram_alloc_from_fd size calculation

[Steven Sistare]

On 1/7/2025 5:01 AM, David Hildenbrand wrote:
> On 02.01.25 22:32, Steve Sistare wrote:
> > qemu_ram_alloc_from_fd allocates space if file_size == 0.  If non-zero,
> > it uses the existing space and verifies it is large enough, but the
> > verification was broken when the offset parameter was introduced.  As
> > a result, a file smaller than offset passes the verification and causes
> > errors later.  Fix that, and update the error message to include offset.
> >
> > Peter provides this concise reproducer:
> >
> >    $ touch ramfile
> >    $ truncate -s 64M ramfile
> >    $ ./qemu-system-x86_64 -object 
> > memory-backend-file,mem-path=./ramfile,offset=128M,size=128M,id=mem1,prealloc=on
> >    qemu-system-x86_64: qemu_prealloc_mem: preallocating memory failed: Bad 
> > address
> >
> > With the fix, the error message is:
> >    qemu-system-x86_64: mem1 backing store size 0x4000000 is too small for 
> > 'size' option 0x8000000 plus 'offset' option 0x8000000
> >
> > Cc: qemu-stable@nongnu.org
> > Fixes: 4b870dc4d0c0 ("hostmem-file: add offset option")
> > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > ---
> >   system/physmem.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/system/physmem.c b/system/physmem.c
> > index c76503a..f01325f 100644
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -1970,10 +1970,11 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
> > MemoryRegion *mr,
> >       size = REAL_HOST_PAGE_ALIGN(size);
> >       file_size = get_file_size(fd);
> > -    if (file_size > offset && file_size < (offset + size)) {
> > -        error_setg(errp, "backing store size 0x%" PRIx64
> > -                   " does not match 'size' option 0x" RAM_ADDR_FMT,
> > -                   file_size, size);
> > +    if (file_size && file_size < offset + size) {
> > +        error_setg(errp, "%s backing store size 0x%" PRIx64
> > +                   " is too small for 'size' option 0x" RAM_ADDR_FMT
> > +                   " plus 'offset' option 0x" RAM_ADDR_FMT,
>
>
> Note that offset is of type "off_t", not ram_addr_t.
>
> ram_addr_t is a uintptr_t, but off_t can be a different integer type.
>
> In meson.build we use "-D_FILE_OFFSET_BITS=64". So on 32bit ram_addr_t would be 
> 32bit but off_t will be 64bit.
>
>
> Printing off_t can be weird [1]. Maybe just cast it to an uint64_t and print it 
> using PRIx64?

I will fix as you suggest, thanks.  Good catch.
With that, should I add your ack or RB to V3?

- Steve

> [1] 
> https://stackoverflow.com/questions/586928/how-should-i-print-types-like-off-t-and-size-t