On 220621 1034, David Hildenbrand wrote: > On 09.06.22 15:58, Alexander Bulekov wrote: > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > > This flag is set/checked prior to calling a device's MemoryRegion > > handlers, and set when device code initiates DMA. The purpose of this > > flag is to prevent two types of DMA-based reentrancy issues: > > > > 1.) mmio -> dma -> mmio case > > 2.) bh -> dma write -> mmio case > > > > These issues have led to problems such as stack-exhaustion and > > use-after-frees. > > > > Summary of the problem from Peter Maydell: > > https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827 > > Signed-off-by: Alexander Bulekov <alx...@bu.edu> > > --- > > include/hw/pci/pci.h | 13 +++++++++++-- > > include/hw/qdev-core.h | 3 +++ > > softmmu/dma-helpers.c | 12 ++++++++++++ > > softmmu/memory.c | 15 +++++++++++++++ > > softmmu/trace-events | 1 + > > 5 files changed, 42 insertions(+), 2 deletions(-) > > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > > index 44dacfa224..ab1ad0f7a8 100644 > > --- a/include/hw/pci/pci.h > > +++ b/include/hw/pci/pci.h > > @@ -834,8 +834,17 @@ static inline MemTxResult pci_dma_rw(PCIDevice *dev, > > dma_addr_t addr, > > void *buf, dma_addr_t len, > > DMADirection dir, MemTxAttrs attrs) > > { > > - return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, > > - dir, attrs); > > + bool prior_engaged_state; > > + MemTxResult result; > > + > > + prior_engaged_state = dev->qdev.engaged_in_io; > > + > > + dev->qdev.engaged_in_io = true; > > + result = dma_memory_rw(pci_get_address_space(dev), addr, buf, len, > > + dir, attrs); > > + dev->qdev.engaged_in_io = prior_engaged_state; > > + > > + return result; > > } > > > > /** > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > index 92c3d65208..6474dc51fa 100644 > > --- a/include/hw/qdev-core.h > > +++ b/include/hw/qdev-core.h > > @@ -193,6 +193,9 @@ struct DeviceState { > > int instance_id_alias; > > int alias_required_for_version; > > ResettableState reset; > > + > > + /* Is the device currently in mmio/pio/dma? Used to prevent > > re-entrancy */ > > + int engaged_in_io; > > }; > > > > struct DeviceListener { > > diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c > > index 7820fec54c..7a4f1fb9b3 100644 > > --- a/softmmu/dma-helpers.c > > +++ b/softmmu/dma-helpers.c > > @@ -288,8 +288,16 @@ static MemTxResult dma_buf_rw(void *buf, dma_addr_t > > len, dma_addr_t *residual, > > uint8_t *ptr = buf; > > dma_addr_t xresidual; > > int sg_cur_index; > > + DeviceState *dev; > > + bool prior_engaged_state; > > MemTxResult res = MEMTX_OK; > > > > + dev = sg->dev; > > + if (dev) { > > + prior_engaged_state = dev->engaged_in_io; > > + dev->engaged_in_io = true; > > + } > > + > > xresidual = sg->size; > > sg_cur_index = 0; > > len = MIN(len, xresidual); > > @@ -302,6 +310,10 @@ static MemTxResult dma_buf_rw(void *buf, dma_addr_t > > len, dma_addr_t *residual, > > xresidual -= xfer; > > } > > > > + if (dev) { > > + dev->engaged_in_io = prior_engaged_state; > > + } > > + > > if (residual) { > > *residual = xresidual; > > } > > diff --git a/softmmu/memory.c b/softmmu/memory.c > > index 7ba2048836..44a14bb4f5 100644 > > --- a/softmmu/memory.c > > +++ b/softmmu/memory.c > > @@ -532,6 +532,7 @@ static MemTxResult access_with_adjusted_size(hwaddr > > addr, > > uint64_t access_mask; > > unsigned access_size; > > unsigned i; > > + DeviceState *dev = NULL; > > MemTxResult r = MEMTX_OK; > > > > if (!access_size_min) { > > @@ -541,6 +542,17 @@ static MemTxResult access_with_adjusted_size(hwaddr > > addr, > > access_size_max = 4; > > } > > > > + /* Do not allow more than one simultanous access to a device's IO > > Regions */ > > + if (mr->owner && > > + !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) { > > Would it make sense to define some helper function like > memory_region_is_XXX (I assume XXX -> DEVICE_IO), to make that code > easier to be consumed by humans?
Yes - There are a few other places that have similar checks. It probably makes sense to consolidate them. Thanks > > Unfortunately I cannot really comment on the sanity of the approach, > because the underlying problem isn't completely clear to me (I think > other people on CC were involved in the discussions around DMA reentry > and failed attempts in the past). Having that said, that approach > doesn't look wrong to me. > > -- > Thanks, > > David / dhildenb >
