* Vladimir Sementsov-Ogievskiy (vsement...@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 <vsement...@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.
Dave
> >
> > > -Â Â Â Â Â Â Â 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 <ebl...@redhat.com>
> >
>
>
> --
> Best regards,
> Vladimir
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
