[Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned

Wei Yang posted 1 patch 13 weeks ago
Test docker-clang@ubuntu passed
Test asan passed
Test docker-mingw@fedora passed
Test FreeBSD passed
Test checkpatch passed
Test s390x passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190712032704.7826-1-richardw.yang@linux.intel.com
Maintainers: Richard Henderson <rth@twiddle.net>, Paolo Bonzini <pbonzini@redhat.com>
exec.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

[Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned

Posted by Wei Yang 13 weeks ago
Since the start addr is already checked, to make sure the range is
aligned, checking the length is enough.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 exec.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/exec.c b/exec.c
index 50ea9c5aaa..8fa980baae 100644
--- a/exec.c
+++ b/exec.c
@@ -4067,10 +4067,9 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
 
     if ((start + length) <= rb->used_length) {
         bool need_madvise, need_fallocate;
-        uint8_t *host_endaddr = host_startaddr + length;
-        if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
-            error_report("ram_block_discard_range: Unaligned end address: %p",
-                         host_endaddr);
+        if (length & (rb->page_size - 1)) {
+            error_report("ram_block_discard_range: Unaligned length: %lx",
+                         length);
             goto err;
         }
 
-- 
2.17.1


Re: [Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned

Posted by Dr. David Alan Gilbert 12 weeks ago
* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Since the start addr is already checked, to make sure the range is
> aligned, checking the length is enough.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  exec.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 50ea9c5aaa..8fa980baae 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -4067,10 +4067,9 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
>  
>      if ((start + length) <= rb->used_length) {
>          bool need_madvise, need_fallocate;
> -        uint8_t *host_endaddr = host_startaddr + length;
> -        if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
> -            error_report("ram_block_discard_range: Unaligned end address: %p",
> -                         host_endaddr);
> +        if (length & (rb->page_size - 1)) {
> +            error_report("ram_block_discard_range: Unaligned length: %lx",
> +                         length);

Yes, I *think* this is safe, we'll need to watch out for any warnings;
David Gibson's balloon fix from February means that the balloon code
should now warn in it's case.

rth: Want to pick this up?

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>              goto err;
>          }
>  
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned

Posted by Wei Yang 4 weeks ago
On Fri, Jul 19, 2019 at 06:54:00PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> Since the start addr is already checked, to make sure the range is
>> aligned, checking the length is enough.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  exec.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/exec.c b/exec.c
>> index 50ea9c5aaa..8fa980baae 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -4067,10 +4067,9 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
>>  
>>      if ((start + length) <= rb->used_length) {
>>          bool need_madvise, need_fallocate;
>> -        uint8_t *host_endaddr = host_startaddr + length;
>> -        if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
>> -            error_report("ram_block_discard_range: Unaligned end address: %p",
>> -                         host_endaddr);
>> +        if (length & (rb->page_size - 1)) {
>> +            error_report("ram_block_discard_range: Unaligned length: %lx",
>> +                         length);
>
>Yes, I *think* this is safe, we'll need to watch out for any warnings;
>David Gibson's balloon fix from February means that the balloon code
>should now warn in it's case.
>
>rth: Want to pick this up?
>
>Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>

Any other task I should do for progress?

>>              goto err;
>>          }
>>  
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned

Posted by Wei Yang 8 weeks ago
On Fri, Jul 19, 2019 at 06:54:00PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> Since the start addr is already checked, to make sure the range is
>> aligned, checking the length is enough.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  exec.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/exec.c b/exec.c
>> index 50ea9c5aaa..8fa980baae 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -4067,10 +4067,9 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
>>  
>>      if ((start + length) <= rb->used_length) {
>>          bool need_madvise, need_fallocate;
>> -        uint8_t *host_endaddr = host_startaddr + length;
>> -        if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
>> -            error_report("ram_block_discard_range: Unaligned end address: %p",
>> -                         host_endaddr);
>> +        if (length & (rb->page_size - 1)) {
>> +            error_report("ram_block_discard_range: Unaligned length: %lx",
>> +                         length);
>
>Yes, I *think* this is safe, we'll need to watch out for any warnings;
>David Gibson's balloon fix from February means that the balloon code
>should now warn in it's case.
>
>rth: Want to pick this up?
>
>Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>

Hi, David

Do you like this one?

>>              goto err;
>>          }
>>  
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned

Posted by Paolo Bonzini 12 weeks ago
On 19/07/19 19:54, Dr. David Alan Gilbert wrote:
>> -        if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
>> -            error_report("ram_block_discard_range: Unaligned end address: %p",
>> -                         host_endaddr);
>> +        if (length & (rb->page_size - 1)) {
>> +            error_report("ram_block_discard_range: Unaligned length: %lx",
>> +                         length);
> Yes, I *think* this is safe, we'll need to watch out for any warnings;

Do you mean compiler or QEMU warning?  The patch is safe since there's an

    if ((uintptr_t)host_startaddr & (rb->page_size - 1)) {
        error_report("ram_block_discard_range: Unaligned start address: %p",
                     host_startaddr);
        goto err;
    }

just before this context.

Paolo

Re: [Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned

Posted by Dr. David Alan Gilbert 12 weeks ago
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 19/07/19 19:54, Dr. David Alan Gilbert wrote:
> >> -        if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
> >> -            error_report("ram_block_discard_range: Unaligned end address: %p",
> >> -                         host_endaddr);
> >> +        if (length & (rb->page_size - 1)) {
> >> +            error_report("ram_block_discard_range: Unaligned length: %lx",
> >> +                         length);
> > Yes, I *think* this is safe, we'll need to watch out for any warnings;
> 
> Do you mean compiler or QEMU warning?

No, I mean lots of these error reports being printed out in some common
case.

Dave

  The patch is safe since there's an
> 
>     if ((uintptr_t)host_startaddr & (rb->page_size - 1)) {
>         error_report("ram_block_discard_range: Unaligned start address: %p",
>                      host_startaddr);
>         goto err;
>     }
> 
> just before this context.
> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned

Posted by Wei Yang 1 week ago
On Fri, Jul 19, 2019 at 07:06:51PM +0100, Dr. David Alan Gilbert wrote:
>* Paolo Bonzini (pbonzini@redhat.com) wrote:
>> On 19/07/19 19:54, Dr. David Alan Gilbert wrote:
>> >> -        if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
>> >> -            error_report("ram_block_discard_range: Unaligned end address: %p",
>> >> -                         host_endaddr);
>> >> +        if (length & (rb->page_size - 1)) {
>> >> +            error_report("ram_block_discard_range: Unaligned length: %lx",
>> >> +                         length);
>> > Yes, I *think* this is safe, we'll need to watch out for any warnings;
>> 
>> Do you mean compiler or QEMU warning?
>
>No, I mean lots of these error reports being printed out in some common
>case.
>
>Dave
>
>  The patch is safe since there's an

Dave,

Any further comment for this patch? or What should I do next?

>> 
>>     if ((uintptr_t)host_startaddr & (rb->page_size - 1)) {
>>         error_report("ram_block_discard_range: Unaligned start address: %p",
>>                      host_startaddr);
>>         goto err;
>>     }
>> 
>> just before this context.
>> 
>> Paolo
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me