Re: [Xen-devel] [Qemu-devel] [RFC/BUG] xen-mapcache: buggy invalidate map cache?

["Herongguang (Stephen)" <herongguang.he@xxxxxxxxxx>]

On 2017/4/12 14:17, Alexey G wrote:
> On Tue, 11 Apr 2017 15:32:09 -0700 (PDT)
> Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>
> > On Tue, 11 Apr 2017, hrg wrote:
> > > On Tue, Apr 11, 2017 at 3:50 AM, Stefano Stabellini
> > > <sstabellini@xxxxxxxxxx> wrote:
> > > > On Mon, 10 Apr 2017, Stefano Stabellini wrote:
> > > > > On Mon, 10 Apr 2017, hrg wrote:
> > > > > > On Sun, Apr 9, 2017 at 11:55 PM, hrg <hrgstephen@xxxxxxxxx> wrote:
> > > > > > > On Sun, Apr 9, 2017 at 11:52 PM, hrg <hrgstephen@xxxxxxxxx> wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > In xen_map_cache_unlocked(), map to guest memory maybe in
> > > > > > > > entry->next instead of first level entry (if map to rom other than
> > > > > > > > guest memory comes first), while in xen_invalidate_map_cache(),
> > > > > > > > when VM ballooned out memory, qemu did not invalidate cache entries
> > > > > > > > in linked list(entry->next), so when VM balloon back in memory,
> > > > > > > > gfns probably mapped to different mfns, thus if guest asks device
> > > > > > > > to DMA to these GPA, qemu may DMA to stale MFNs.
> > > > > > > >
> > > > > > > > So I think in xen_invalidate_map_cache() linked lists should also be
> > > > > > > > checked and invalidated.
> > > > > > > >
> > > > > > > > What’s your opinion? Is this a bug? Is my analyze correct?
> > > > > Yes, you are right. We need to go through the list for each element of
> > > > > the array in xen_invalidate_map_cache. Can you come up with a patch?
> > > > I spoke too soon. In the regular case there should be no locked mappings
> > > > when xen_invalidate_map_cache is called (see the DPRINTF warning at the
> > > > beginning of the functions). Without locked mappings, there should never
> > > > be more than one element in each list (see xen_map_cache_unlocked:
> > > > entry->lock == true is a necessary condition to append a new entry to
> > > > the list, otherwise it is just remapped).
> > > >
> > > > Can you confirm that what you are seeing are locked mappings
> > > > when xen_invalidate_map_cache is called? To find out, enable the DPRINTK
> > > > by turning it into a printf or by defininig MAPCACHE_DEBUG.
> > > In fact, I think the DPRINTF above is incorrect too. In
> > > pci_add_option_rom(), rtl8139 rom is locked mapped in
> > > pci_add_option_rom->memory_region_get_ram_ptr (after
> > > memory_region_init_ram). So actually I think we should remove the
> > > DPRINTF warning as it is normal.
> > Let me explain why the DPRINTF warning is there: emulated dma operations
> > can involve locked mappings. Once a dma operation completes, the related
> > mapping is unlocked and can be safely destroyed. But if we destroy a
> > locked mapping in xen_invalidate_map_cache, while a dma is still
> > ongoing, QEMU will crash. We cannot handle that case.
> >
> > However, the scenario you described is different. It has nothing to do
> > with DMA. It looks like pci_add_option_rom calls
> > memory_region_get_ram_ptr to map the rtl8139 rom. The mapping is a
> > locked mapping and it is never unlocked or destroyed.
> >
> > It looks like "ptr" is not used after pci_add_option_rom returns. Does
> > the append patch fix the problem you are seeing? For the proper fix, I
> > think we probably need some sort of memory_region_unmap wrapper or maybe
> > a call to address_space_unmap.
> Hmm, for some reason my message to the Xen-devel list got rejected but was sent
> to Qemu-devel instead, without any notice. Sorry if I'm missing something
> obvious as a list newbie.
>
> Stefano, hrg,
>
> There is an issue with inconsistency between the list of normal MapCacheEntry's
> and their 'reverse' counterparts - MapCacheRev's in locked_entries.
> When bad situation happens, there are multiple (locked) MapCacheEntry
> entries in the bucket's linked list along with a number of MapCacheRev's. And
> when it comes to a reverse lookup, xen-mapcache picks the wrong entry from the
> first list and calculates a wrong pointer from it which may then be caught with
> the "Bad RAM offset" check (or not). Mapcache invalidation might be related to
> this issue as well I think.
>
> I'll try to provide a test code which can reproduce the issue from the
> guest side using an emulated IDE controller, though it's much simpler to achieve
> this result with an AHCI controller using multiple NCQ I/O commands. So far I've
> seen this issue only with Windows 7 (and above) guest on AHCI, but any block I/O
> DMA should be enough I think.
Yes, I think there may be other bugs lurking, considering the complexity, 
though we need to reproduce it if we want to delve into it.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel