* [Qemu-devel] [PATCH v3] util/mmap-alloc: check parameter before using
@ 2016-10-28 8:56 Cao jin
2016-10-28 14:22 ` Michael Tokarev
0 siblings, 1 reply; 5+ messages in thread
From: Cao jin @ 2016-10-28 8:56 UTC (permalink / raw)
To: qemu-devel, qemu-trivial; +Cc: eblake, peter.maydell, armbru, mst, thuth
Also refactor a little bit for readability
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;
}
void qemu_ram_munmap(void *ptr, size_t size)
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] util/mmap-alloc: check parameter before using
2016-10-28 8:56 [Qemu-devel] [PATCH v3] util/mmap-alloc: check parameter before using Cao jin
@ 2016-10-28 14:22 ` Michael Tokarev
2016-10-31 3:57 ` Cao jin
0 siblings, 1 reply; 5+ messages in thread
From: Michael Tokarev @ 2016-10-28 14:22 UTC (permalink / raw)
To: Cao jin, qemu-devel, qemu-trivial
Cc: peter.maydell, thuth, eblake, armbru, mst
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?
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.. :)
At the very least, your commit comment is a bit misleading, as it says
about readability, but it also MAY change semantics.
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 :)
Thanks,
/mjt
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] util/mmap-alloc: check parameter before using
2016-10-28 14:22 ` Michael Tokarev
@ 2016-10-31 3:57 ` Cao jin
2016-10-31 7:32 ` Thomas Huth
0 siblings, 1 reply; 5+ messages in thread
From: Cao jin @ 2016-10-31 3:57 UTC (permalink / raw)
To: Michael Tokarev, qemu-devel, qemu-trivial
Cc: peter.maydell, thuth, eblake, armbru, mst
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] util/mmap-alloc: check parameter before using
2016-10-31 3:57 ` Cao jin
@ 2016-10-31 7:32 ` Thomas Huth
2016-10-31 11:17 ` Cao jin
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Huth @ 2016-10-31 7:32 UTC (permalink / raw)
To: Cao jin, Michael Tokarev, qemu-devel, qemu-trivial
Cc: peter.maydell, eblake, armbru, mst
On 31.10.2016 04:57, Cao jin wrote:
>
>
> 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.
I originally had similar problems as Michael understanding your changes
to ptr / ptr1 ... so you should likely at add the information with
MAP_FIXED etc. to the patch description at least, I think.
It's maybe also cleaner if you split your patch into two parts, first
patch to fix the parameter checking, and second patch to change the ptr1
stuff. That way the later patch could also be easier reverted in case
there are problems with that.
Thomas
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] util/mmap-alloc: check parameter before using
2016-10-31 7:32 ` Thomas Huth
@ 2016-10-31 11:17 ` Cao jin
0 siblings, 0 replies; 5+ messages in thread
From: Cao jin @ 2016-10-31 11:17 UTC (permalink / raw)
To: Thomas Huth, Michael Tokarev, qemu-devel, qemu-trivial
Cc: peter.maydell, eblake, armbru, mst
On 10/31/2016 03:32 PM, Thomas Huth wrote:
> On 31.10.2016 04:57, Cao jin wrote:
>>>
>>> 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.
>
> I originally had similar problems as Michael understanding your changes
> to ptr / ptr1 ... so you should likely at add the information with
> MAP_FIXED etc. to the patch description at least, I think.
>
> It's maybe also cleaner if you split your patch into two parts, first
> patch to fix the parameter checking, and second patch to change the ptr1
> stuff. That way the later patch could also be easier reverted in case
> there are problems with that.
>
> Thomas
>
Oh, I didn't realize it would confuse people so easy, seems it actually
did:(
Readability is kind of personal taste thing, generally, I woundn't send
a patch only care readability which maybe controversial, so I am not
sure this one worth the trouble to split the patch, but if everyone want
it, I am pleased to do it.
I was hoping someone(maybe author) could give a ack, or point out the
error I made.
Will try to document it in next version.
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-10-31 11:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-28 8:56 [Qemu-devel] [PATCH v3] util/mmap-alloc: check parameter before using Cao jin
2016-10-28 14:22 ` Michael Tokarev
2016-10-31 3:57 ` Cao jin
2016-10-31 7:32 ` Thomas Huth
2016-10-31 11:17 ` Cao jin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.