RE: Race condition in overlayed qcow2?

[Pavel Dovgalyuk]

Kevin, what do you think about it?

What guest is intended to receive, when it requests multiple reads to the same 
buffer in a single DMA transaction?

Should it be the first SG part? The last one?
Or just a random set of bytes? (Then why it is reading this data in that case?)

Pavel Dovgalyuk

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy [mailto:address@hidden]
> Sent: Tuesday, February 25, 2020 12:19 PM
> To: dovgaluk
> Cc: address@hidden; address@hidden; address@hidden
> Subject: Re: Race condition in overlayed qcow2?
> 
> 25.02.2020 10:56, dovgaluk wrote:
> > Vladimir Sementsov-Ogievskiy писал 2020-02-25 10:27:
> >> 25.02.2020 8:58, dovgaluk wrote:
> >>> Vladimir Sementsov-Ogievskiy писал 2020-02-21 16:23:
> >>>> 21.02.2020 15:35, dovgaluk wrote:
> >>>>> Vladimir Sementsov-Ogievskiy писал 2020-02-21 13:09:
> >>>>>> 21.02.2020 12:49, dovgaluk wrote:
> >>>>>>> Vladimir Sementsov-Ogievskiy писал 2020-02-20 12:36:
> >>>>>>
> >>>>>> So, preadv in file-posix.c returns different results for the same
> >>>>>> offset, for file which is always opened in RO mode? Sounds impossible
> >>>>>> :)
> >>>>>
> >>>>> True.
> >>>>> Maybe my logging is wrong?
> >>>>>
> >>>>> static ssize_t
> >>>>> qemu_preadv(int fd, const struct iovec *iov, int nr_iov, off_t offset)
> >>>>> {
> >>>>>      ssize_t res = preadv(fd, iov, nr_iov, offset);
> >>>>>      qemu_log("preadv %x %"PRIx64"\n", fd, (uint64_t)offset);
> >>>>>      int i;
> >>>>>      uint32_t sum = 0;
> >>>>>      int cnt = 0;
> >>>>>      for (i = 0 ; i < nr_iov ; ++i) {
> >>>>>          int j;
> >>>>>          for (j = 0 ; j < (int)iov[i].iov_len ; ++j)
> >>>>>          {
> >>>>>              sum += ((uint8_t*)iov[i].iov_base)[j];
> >>>>>              ++cnt;
> >>>>>          }
> >>>>>      }
> >>>>>      qemu_log("size: %x sum: %x\n", cnt, sum);
> >>>>>      assert(cnt == res);
> >>>>>      return res;
> >>>>> }
> >>>>>
> >>>>
> >>>> Hmm, I don't see any issues here..
> >>>>
> >>>> Are you absolutely sure, that all these reads are from backing file,
> >>>> which is read-only and never changed (may be by other processes)?
> >>>
> >>> Yes, I made a copy and compared the files with binwalk.
> >>>
> >>>> 2. guest modifies buffers during operation (you can catch it if
> >>>> allocate personal buffer for preadv, than calculate checksum, then
> >>>> memcpy to guest buffer)
> >>>
> >>> I added the following to the qemu_preadv:
> >>>
> >>>      // do it again
> >>>      unsigned char *buf = g_malloc(cnt);
> >>>      struct iovec v = {buf, cnt};
> >>>      res = preadv(fd, &v, 1, offset);
> >>>      assert(cnt == res);
> >>>      uint32_t sum2 = 0;
> >>>      for (i = 0 ; i < cnt ; ++i)
> >>>          sum2 += buf[i];
> >>>      g_free(buf);
> >>>      qemu_log("--- sum2 = %x\n", sum2);
> >>>      assert(sum2 == sum);
> >>>
> >>> These two reads give different results.
> >>> But who can modify the buffer while qcow2 workers filling it with data 
> >>> from the disk?
> >>>
> >>
> >> As far as I know, it's guest's buffer, and guest may modify it during
> >> the operation. So, it may be winxp :)
> >
> > True, but normally the guest won't do it.
> >
> > But I noticed that DMA operation which causes the problems has the 
> > following set of the
> buffers:
> > dma read sg size 20000 offset: c000fe00
> > --- sg: base: 2eb1000 len: 1000
> > --- sg: base: 3000000 len: 1000
> > --- sg: base: 2eb2000 len: 3000
> > --- sg: base: 3000000 len: 1000
> > --- sg: base: 2eb5000 len: b000
> > --- sg: base: 3040000 len: 1000
> > --- sg: base: 2f41000 len: 3000
> > --- sg: base: 3000000 len: 1000
> > --- sg: base: 2f44000 len: 4000
> > --- sg: base: 3000000 len: 1000
> > --- sg: base: 2f48000 len: 2000
> > --- sg: base: 3000000 len: 1000
> > --- sg: base: 3000000 len: 1000
> > --- sg: base: 3000000 len: 1000
> >
> >
> > It means that one DMA transaction performs multiple reads into the same 
> > address.
> > And no races is possible, when there is only one qcow2 worker.
> > When there are many of them - they can fill this buffer simultaneously.
> >
> 
> Hmm, actually if guest start parallel reads into same buffer from different 
> offsets, races are
> possible anyway, as different requests run in parallel even with one worker, 
> because
> MAX_WORKERS is per-request value, not total... But several workers may 
> increase probability of
> races or introduce new ones.
> 
> So, actually, several workers of one request can write to the same buffer 
> only if guest
> provides broken iovec, which references the same buffer several times (if it 
> is possible at
> all).
> 
> 
> 
> --
> Best regards,
> Vladimir