Re: [PATCH v3 15/21] migration/block-dirty-bitmap: relax error handling in incoming part

[Vladimir Sementsov-Ogievskiy]

27.07.2020 14:16, Dr. David Alan Gilbert wrote:
> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> > 24.07.2020 20:35, Eric Blake wrote:
> > > On 7/24/20 3:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > > Bitmaps data is not critical, and we should not fail the migration (or
> > > > use postcopy recovering) because of dirty-bitmaps migration failure.
> > > > Instead we should just lose unfinished bitmaps.
> > > >
> > > > Still we have to report io stream violation errors, as they affect the
> > > > whole migration stream.
> > > >
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > > ---
> > > > Â  migration/block-dirty-bitmap.c | 152 +++++++++++++++++++++++++--------
> > > > Â  1 file changed, 117 insertions(+), 35 deletions(-)
> > >
> > > > @@ -650,15 +695,32 @@ static int dirty_bitmap_load_bits(QEMUFile *f, 
> > > > DBMLoadState *s)
> > > > Â Â Â Â Â  if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
> > > > Â Â Â Â Â Â Â Â Â  trace_dirty_bitmap_load_bits_zeroes();
> > > > -Â Â Â Â Â Â Â  bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte, 
> > > > nr_bytes,
> > > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 
> > > > Â Â Â Â Â  false);
> > > > +Â Â Â Â Â Â Â  if (!s->cancelled) {
> > > > +Â Â Â Â Â Â Â Â Â Â Â  bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, 
> > > > first_byte,
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 
> > > > Â Â Â Â Â Â Â Â Â  nr_bytes, false);
> > > > +Â Â Â Â Â Â Â  }
> > > > Â Â Â Â Â  } else {
> > > > Â Â Â Â Â Â Â Â Â  size_t ret;
> > > > Â Â Â Â Â Â Â Â Â  uint8_t *buf;
> > > > Â Â Â Â Â Â Â Â Â  uint64_t buf_size = qemu_get_be64(f);
> > >
> > > Pre-existing, but if I understand, we are reading a value from the migration 
> > > stream...
> >
> > Hmm, actually, this becomes worse after patch, as before patch we had the 
> > check, that the size at least corresponds to the bitmap.. But we want to relax 
> > things in cancelled mode (and we may not have any bitmap). Most correct thing 
> > is to use read in a loop to just skip the data from stream if we are in 
> > cancelled mode (something like nbd_drop()).
> >
> > I can fix this with a follow-up patch.
>
> If the size is bogus, it's probably not worth trying to skip anything
> because it could be just a broken/misaligned stream.

The problem is that, when we are already in "skipping" mode, we don't have 
actual bitmap to understand, is size look reasonable or not. We can probably just invent 
some heuristic constant (100M for example?), so that any size less will be silently 
skipped, and any size above will be considered as stream violation and cancel postcopy 
process.


> > > > -Â Â Â Â Â Â Â  uint64_t needed_size =
> > > > -Â Â Â Â Â Â Â Â Â Â Â  bdrv_dirty_bitmap_serialization_size(s->bitmap,
> > > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 
> > > > Â Â Â Â Â Â Â Â Â  first_byte, nr_bytes);
> > > > +Â Â Â Â Â Â Â  uint64_t needed_size;
> > > > +
> > > > +Â Â Â Â Â Â Â  buf = g_malloc(buf_size);
> > > > +Â Â Â Â Â Â Â  ret = qemu_get_buffer(f, buf, buf_size);
> > >
> > > ...and using it to malloc memory.  Is that a potential risk of a malicious 
> > > stream causing us to allocate too much memory in relation to the guest's normal 
> > > size?  If so, fixing that should be done separately.
> > >
> > > I'm not a migration expert, but the patch looks reasonable to me.
> > >
> > > Reviewed-by: Eric Blake <eblake@redhat.com>
> >
> >
> > --
> > Best regards,
> > Vladimir
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


--
Best regards,
Vladimir