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

[Alexey G <x1917x@xxxxxxxxx>]

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.

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