| Message ID | 1735853532-330037-1-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] physmem: fix qemu_ram_alloc_from_fd size calculation | expand |
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? [1] https://stackoverflow.com/questions/586928/how-should-i-print-types-like-off-t-and-size-t
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 >
On 07.01.25 17:11, Steven Sistare wrote: > 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? Feel free to add Acked-by: David Hildenbrand <david@redhat.com> Thanks!
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, + memory_region_name(mr), file_size, size, offset); return NULL; }
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(-)