[PATCH V2] physmem: fix qemu_ram_alloc_from_fd size calculation

[PATCH V2] physmem: fix qemu_ram_alloc_from_fd size calculation

[Steve Sistare]

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,
+                   memory_region_name(mr), file_size, size, offset);
         return NULL;
     }
 
-- 
1.8.3.1

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

[David Hildenbrand]

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

-- 
Cheers,

David / dhildenb

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
> 

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

[David Hildenbrand]

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!

-- 
Cheers,

David / dhildenb