Re: [Qemu-devel] [PATCH v3] util/mmap-alloc: check parameter before using

[Cao jin <caoj.fnst@cn.fujitsu.com>]

On 10/28/2016 10:22 PM, Michael Tokarev wrote:
> 28.10.2016 11:56, Cao jin wrote:
>> Also refactor a little bit for readability
>
> See comments below...
>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   util/mmap-alloc.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
>> index 5a85aa3..2f55f5e 100644
>> --- a/util/mmap-alloc.c
>> +++ b/util/mmap-alloc.c
>> @@ -12,6 +12,7 @@
>>
>>   #include "qemu/osdep.h"
>>   #include "qemu/mmap-alloc.h"
>> +#include "qemu/host-utils.h"
>>
>>   #define HUGETLBFS_MAGIC       0x958458f6
>>
>> @@ -61,18 +62,18 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
>>   #else
>>       void *ptr = mmap(0, total, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>>   #endif
>> -    size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
>> +    size_t offset;
>>       void *ptr1;
>>
>>       if (ptr == MAP_FAILED) {
>>           return MAP_FAILED;
>>       }
>>
>> -    /* Make sure align is a power of 2 */
>> -    assert(!(align & (align - 1)));
>> +    assert(is_power_of_2(align));
>>       /* Always align to host page size */
>>       assert(align >= getpagesize());
>>
>> +    offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
>>       ptr1 = mmap(ptr + offset, size, PROT_READ | PROT_WRITE,
>>                   MAP_FIXED |
>>                   (fd == -1 ? MAP_ANONYMOUS : 0) |
>> @@ -83,22 +84,20 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
>>           return MAP_FAILED;
>>       }
>>
>> -    ptr += offset;
>> -    total -= offset;
>> -
>>       if (offset > 0) {
>> -        munmap(ptr - offset, offset);
>> +        munmap(ptr, offset);
>>       }
>>
>>       /*
>>        * Leave a single PROT_NONE page allocated after the RAM block, to serve as
>>        * a guard page guarding against potential buffer overflows.
>>        */
>> +    total -= offset;
>>       if (total > size + getpagesize()) {
>> -        munmap(ptr + size + getpagesize(), total - size - getpagesize());
>> +        munmap(ptr1 + size + getpagesize(), total - size - getpagesize());
>>       }
>>
>> -    return ptr;
>> +    return ptr1;
>
> Why did you change ptr to ptr1 here and above?

Because, I think there always is: ptr + offset == ptr1

>
> On linux, mmap(2) manpage says that address serves as hint, and the
> system create the mapping at a nearby page boundary.  Generally, this
> address is just a hint.  So I'm not really sure if this code is actually
> right.. :)
>

Yes, but the 2nd mmap used MAP_FIXED, which the manpage says:

/Don't interpret addr as a hint: place the mapping at exactly that/ 
/address. addr must be a multiple of the page size/
/If the specified address cannot be used, mmap() will fail/

> At the very least, your commit comment is a bit misleading, as it says
> about readability, but it also MAY change semantics.
>

I don't think so, one just need dig a little deeper:)

> Maybe just move BOTH "ptr+=, total-=" parts down the line and keep
> using ptr instead of ptr1?
>
> It'd be very good, in my opinion, to document how this whole thing
> is supposed to work :)
>

the change is just some simple arithmetic operation, I think it is 
little difficult for me to find a decent description.

Hope this could also got other maintainer's help, review, or ack

> Thanks,
>
> /mjt
>
>
> .
>

-- 
Yours Sincerely,

Cao jin