| Message ID | 20190712032704.7826-1-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: check length directly to make sure the range is aligned | expand |
* 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
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
* 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
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
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
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
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. > Hi, Dave Haven't see you for a period of time :-) >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
* Wei Yang (richardw.yang@linux.intel.com) wrote: > 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. > > > > Hi, Dave > > Haven't see you for a period of time :-) I'm here (although been busy with virtiofs) - but this patch is exec.c so I think it needs to be picked by Paolo or rth. Dave > >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 > > -- > Wei Yang > Help you, Help me -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Oct 29, 2019 at 07:04:19AM +0000, Dr. David Alan Gilbert wrote: >* Wei Yang (richardw.yang@linux.intel.com) wrote: >> 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. >> > >> >> Hi, Dave >> >> Haven't see you for a period of time :-) > >I'm here (although been busy with virtiofs) - but this patch is exec.c >so I think it needs to be picked by Paolo or rth. > Thanks Dave Paolo Do you feel comfortable with this? >Dave > >> >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 >> >> -- >> Wei Yang >> Help you, Help me >-- >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 29/10/19 09:21, Wei Yang wrote: > Thanks Dave > > Paolo > > Do you feel comfortable with this? > Queued now, thanks. Paolo
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; }
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(-)