All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] memory: prevent dma-reentracy issues
@ 2023-01-19  7:00 Alexander Bulekov
  2023-01-19  7:00 ` [PATCH v4 1/3] " Alexander Bulekov
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Bulekov @ 2023-01-19  7:00 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
	Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
	Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
	Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
	Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
	Eduardo Habkost, Jon Maloy, Siqi Chen

These patches aim to solve two types of DMA-reentrancy issues:

1.) mmio -> dma -> mmio case
To solve this, we track whether the device is engaged in io by
checking/setting a reentrancy-guard within APIs used for MMIO access.

2.) bh -> dma write -> mmio case
This case is trickier, since we dont have a generic way to associate a
bh with the underlying Device/DeviceState. Thus, this version allows a
device to associate a reentrancy-guard with a bh, when creating it.
(Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)

I replaced most of the qemu_bh_new invocations with the guarded analog,
except for the ones where the DeviceState was not trivially accessible

Unlike v3, these changes should address issues in devices that bypass
DMA apis and directly call into address_space.
e.g. https://gitlab.com/qemu-project/qemu/-/issues/827

v3 -> v4: Instead of changing all of the DMA APIs, instead add an
    optional reentrancy guard to the BH API.

v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
    attempting to guess DeviceStates in BHs.


Alexander Bulekov (3):
  memory: prevent dma-reentracy issues
  async: Add an optional reentrancy guard to the BH API
  hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

Alexander Bulekov (3):
  memory: prevent dma-reentracy issues
  async: Add an optional reentrancy guard to the BH API
  hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

 docs/devel/multiple-iothreads.txt |  2 ++
 hw/9pfs/xen-9p-backend.c          |  4 +++-
 hw/block/dataplane/virtio-blk.c   |  3 ++-
 hw/block/dataplane/xen-block.c    |  5 +++--
 hw/block/virtio-blk.c             |  5 +++--
 hw/char/virtio-serial-bus.c       |  3 ++-
 hw/display/qxl.c                  |  9 ++++++---
 hw/display/virtio-gpu.c           |  6 ++++--
 hw/ide/ahci.c                     |  3 ++-
 hw/ide/core.c                     |  3 ++-
 hw/misc/imx_rngc.c                |  6 ++++--
 hw/misc/macio/mac_dbdma.c         |  2 +-
 hw/net/virtio-net.c               |  3 ++-
 hw/nvme/ctrl.c                    |  6 ++++--
 hw/scsi/mptsas.c                  |  3 ++-
 hw/scsi/scsi-bus.c                |  3 ++-
 hw/scsi/vmw_pvscsi.c              |  3 ++-
 hw/usb/dev-uas.c                  |  3 ++-
 hw/usb/hcd-dwc2.c                 |  3 ++-
 hw/usb/hcd-ehci.c                 |  3 ++-
 hw/usb/hcd-uhci.c                 |  2 +-
 hw/usb/host-libusb.c              |  6 ++++--
 hw/usb/redirect.c                 |  6 ++++--
 hw/usb/xen-usb.c                  |  3 ++-
 hw/virtio/virtio-balloon.c        |  5 +++--
 hw/virtio/virtio-crypto.c         |  3 ++-
 include/block/aio.h               | 18 ++++++++++++++++--
 include/hw/qdev-core.h            |  7 +++++++
 include/qemu/main-loop.h          |  7 +++++--
 softmmu/memory.c                  | 15 +++++++++++++++
 softmmu/trace-events              |  1 +
 tests/unit/ptimer-test-stubs.c    |  3 ++-
 util/async.c                      | 12 +++++++++++-
 util/main-loop.c                  |  5 +++--
 34 files changed, 128 insertions(+), 43 deletions(-)

-- 
2.39.0



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 1/3] memory: prevent dma-reentracy issues
  2023-01-19  7:00 [PATCH v4 0/3] memory: prevent dma-reentracy issues Alexander Bulekov
@ 2023-01-19  7:00 ` Alexander Bulekov
  2023-01-20 14:41   ` Darren Kenny
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Bulekov @ 2023-01-19  7:00 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
	Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
	Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
	Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
	Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
	Eduardo Habkost, Jon Maloy, Siqi Chen

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-JVA6W38LK4hJoWae5KcknhPRD5fPBZA@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 <alxndr@bu.edu>
---
 include/hw/qdev-core.h |  7 +++++++
 softmmu/memory.c       | 15 +++++++++++++++
 softmmu/trace-events   |  1 +
 3 files changed, 23 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 35fddb19a6..8858195262 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
     QLIST_ENTRY(NamedClockList) node;
 };
 
+typedef struct {
+    bool engaged_in_io;
+} MemReentrancyGuard;
+
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
     int alias_required_for_version;
     ResettableState reset;
     GSList *unplug_blockers;
+
+    /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+    MemReentrancyGuard mem_reentrancy_guard;
 };
 
 struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index e05332d07f..90ffaaa4f5 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -533,6 +533,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) {
@@ -542,6 +543,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) {
+        dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
+        if (dev->mem_reentrancy_guard.engaged_in_io) {
+            trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+            return MEMTX_ERROR;
+        }
+        dev->mem_reentrancy_guard.engaged_in_io = true;
+    }
+
     /* FIXME: support unaligned access? */
     access_size = MAX(MIN(size, access_size_max), access_size_min);
     access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -556,6 +568,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
                         access_mask, attrs);
         }
     }
+    if (dev) {
+        dev->mem_reentrancy_guard.engaged_in_io = false;
+    }
     return r;
 }
 
diff --git a/softmmu/trace-events b/softmmu/trace-events
index 22606dc27b..62d04ea9a7 100644
--- a/softmmu/trace-events
+++ b/softmmu/trace-events
@@ -13,6 +13,7 @@ memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, u
 memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'"
 memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
+memory_region_reentrant_io(int cpu_index, void *mr, uint64_t offset, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" size %u"
 memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_sync_dirty(const char *mr, const char *listener, int global) "mr '%s' listener '%s' synced (global=%d)"
-- 
2.39.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/3] memory: prevent dma-reentracy issues
  2023-01-19  7:00 ` [PATCH v4 1/3] " Alexander Bulekov
@ 2023-01-20 14:41   ` Darren Kenny
  2023-01-20 14:47     ` Peter Maydell
  0 siblings, 1 reply; 7+ messages in thread
From: Darren Kenny @ 2023-01-20 14:41 UTC (permalink / raw)
  To: Alexander Bulekov, qemu-devel
  Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
	Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
	Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
	Edgar E . Iglesias, Bin Meng, Paolo Bonzini, Michael S . Tsirkin,
	Marcel Apfelbaum, Daniel P . Berrangé,
	Eduardo Habkost, Jon Maloy, Siqi Chen

Hi Alex,

Generally, this looks good, but I do have a comment below...

On Thursday, 2023-01-19 at 02:00:02 -05, 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-JVA6W38LK4hJoWae5KcknhPRD5fPBZA@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 <alxndr@bu.edu>
> ---
>  include/hw/qdev-core.h |  7 +++++++
>  softmmu/memory.c       | 15 +++++++++++++++
>  softmmu/trace-events   |  1 +
>  3 files changed, 23 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 35fddb19a6..8858195262 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -162,6 +162,10 @@ struct NamedClockList {
>      QLIST_ENTRY(NamedClockList) node;
>  };
>  
> +typedef struct {
> +    bool engaged_in_io;
> +} MemReentrancyGuard;
> +
>  /**
>   * DeviceState:
>   * @realized: Indicates whether the device has been fully constructed.
> @@ -194,6 +198,9 @@ struct DeviceState {
>      int alias_required_for_version;
>      ResettableState reset;
>      GSList *unplug_blockers;
> +
> +    /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
> +    MemReentrancyGuard mem_reentrancy_guard;
>  };
>  
>  struct DeviceListener {
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index e05332d07f..90ffaaa4f5 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -533,6 +533,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) {
> @@ -542,6 +543,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) {
> +        dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);

I don't know how likely this is to happen, but according to:

- https://qemu-project.gitlab.io/qemu/devel/qom.html#c.object_dynamic_cast

it is possible for the object_dynamic_cast() function to return NULL,
so it might make sense to wrap the subsequent calls in a test of dev !=
NULL.

Thanks,

Darren.

> +        if (dev->mem_reentrancy_guard.engaged_in_io) {
> +            trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
> +            return MEMTX_ERROR;
> +        }
> +        dev->mem_reentrancy_guard.engaged_in_io = true;
> +    }
> +
>      /* FIXME: support unaligned access? */
>      access_size = MAX(MIN(size, access_size_max), access_size_min);
>      access_mask = MAKE_64BIT_MASK(0, access_size * 8);
> @@ -556,6 +568,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>                          access_mask, attrs);
>          }
>      }
> +    if (dev) {
> +        dev->mem_reentrancy_guard.engaged_in_io = false;
> +    }
>      return r;
>  }
>  
> diff --git a/softmmu/trace-events b/softmmu/trace-events
> index 22606dc27b..62d04ea9a7 100644
> --- a/softmmu/trace-events
> +++ b/softmmu/trace-events
> @@ -13,6 +13,7 @@ memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, u
>  memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'"
>  memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
>  memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
> +memory_region_reentrant_io(int cpu_index, void *mr, uint64_t offset, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" size %u"
>  memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
>  memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
>  memory_region_sync_dirty(const char *mr, const char *listener, int global) "mr '%s' listener '%s' synced (global=%d)"
> -- 
> 2.39.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/3] memory: prevent dma-reentracy issues
  2023-01-20 14:41   ` Darren Kenny
@ 2023-01-20 14:47     ` Peter Maydell
  2023-01-26  5:19       ` Alexander Bulekov
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2023-01-20 14:47 UTC (permalink / raw)
  To: Darren Kenny
  Cc: Alexander Bulekov, qemu-devel, Stefan Hajnoczi,
	Philippe Mathieu-Daudé,
	Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
	Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
	Edgar E . Iglesias, Bin Meng, Paolo Bonzini, Michael S . Tsirkin,
	Marcel Apfelbaum, Daniel P . Berrangé,
	Eduardo Habkost, Jon Maloy, Siqi Chen

On Fri, 20 Jan 2023 at 14:42, Darren Kenny <darren.kenny@oracle.com> wrote:
> Generally, this looks good, but I do have a comment below...
>
> On Thursday, 2023-01-19 at 02:00:02 -05, 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:

> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index e05332d07f..90ffaaa4f5 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -533,6 +533,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) {
> > @@ -542,6 +543,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) {
> > +        dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
>
> I don't know how likely this is to happen, but according to:
>
> - https://qemu-project.gitlab.io/qemu/devel/qom.html#c.object_dynamic_cast
>
> it is possible for the object_dynamic_cast() function to return NULL,
> so it might make sense to wrap the subsequent calls in a test of dev !=
> NULL.

Yes. This came up in a previous version of this:
https://lore.kernel.org/qemu-devel/CAFEAcA8E4nDoAWcj-v-dED-0hDtXGjJNSp3A=kdGF8UOCw0DrQ@mail.gmail.com/

It's generally a bug to call object_dynamic_cast() and then not check
the return value.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/3] memory: prevent dma-reentracy issues
  2023-01-20 14:47     ` Peter Maydell
@ 2023-01-26  5:19       ` Alexander Bulekov
  0 siblings, 0 replies; 7+ messages in thread
From: Alexander Bulekov @ 2023-01-26  5:19 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Darren Kenny, qemu-devel, Stefan Hajnoczi,
	Philippe Mathieu-Daudé,
	Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
	Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
	Edgar E . Iglesias, Bin Meng, Paolo Bonzini, Michael S . Tsirkin,
	Marcel Apfelbaum, Daniel P . Berrangé,
	Eduardo Habkost, Jon Maloy, Siqi Chen

On 230120 1447, Peter Maydell wrote:
> On Fri, 20 Jan 2023 at 14:42, Darren Kenny <darren.kenny@oracle.com> wrote:
> > Generally, this looks good, but I do have a comment below...
> >
> > On Thursday, 2023-01-19 at 02:00:02 -05, 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:
> 
> > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > index e05332d07f..90ffaaa4f5 100644
> > > --- a/softmmu/memory.c
> > > +++ b/softmmu/memory.c
> > > @@ -533,6 +533,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) {
> > > @@ -542,6 +543,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) {
> > > +        dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
> >
> > I don't know how likely this is to happen, but according to:
> >
> > - https://qemu-project.gitlab.io/qemu/devel/qom.html#c.object_dynamic_cast
> >
> > it is possible for the object_dynamic_cast() function to return NULL,
> > so it might make sense to wrap the subsequent calls in a test of dev !=
> > NULL.
> 
> Yes. This came up in a previous version of this:
> https://lore.kernel.org/qemu-devel/CAFEAcA8E4nDoAWcj-v-dED-0hDtXGjJNSp3A=kdGF8UOCw0DrQ@mail.gmail.com/
> 
> It's generally a bug to call object_dynamic_cast() and then not check
> the return value.
> 

Sorry I missed that - Will be fixed in V5.
-Alex

> thanks
> -- PMM


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 0/3] memory: prevent dma-reentracy issues
@ 2023-01-19  7:03 Alexander Bulekov
  0 siblings, 0 replies; 7+ messages in thread
From: Alexander Bulekov @ 2023-01-19  7:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
	Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
	Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
	Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
	Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
	Eduardo Habkost, Jon Maloy, Siqi Chen

These patches aim to solve two types of DMA-reentrancy issues:

1.) mmio -> dma -> mmio case
To solve this, we track whether the device is engaged in io by
checking/setting a reentrancy-guard within APIs used for MMIO access.

2.) bh -> dma write -> mmio case
This case is trickier, since we dont have a generic way to associate a
bh with the underlying Device/DeviceState. Thus, this version allows a
device to associate a reentrancy-guard with a bh, when creating it.
(Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)

I replaced most of the qemu_bh_new invocations with the guarded analog,
except for the ones where the DeviceState was not trivially accessible

Unlike v3, these changes should address issues in devices that bypass
DMA apis and directly call into address_space.
e.g. https://gitlab.com/qemu-project/qemu/-/issues/827

v3 -> v4: Instead of changing all of the DMA APIs, instead add an
    optional reentrancy guard to the BH API.

v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
    attempting to guess DeviceStates in BHs.


Alexander Bulekov (3):
  memory: prevent dma-reentracy issues
  async: Add an optional reentrancy guard to the BH API
  hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

Alexander Bulekov (3):
  memory: prevent dma-reentracy issues
  async: Add an optional reentrancy guard to the BH API
  hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

 docs/devel/multiple-iothreads.txt |  2 ++
 hw/9pfs/xen-9p-backend.c          |  4 +++-
 hw/block/dataplane/virtio-blk.c   |  3 ++-
 hw/block/dataplane/xen-block.c    |  5 +++--
 hw/block/virtio-blk.c             |  5 +++--
 hw/char/virtio-serial-bus.c       |  3 ++-
 hw/display/qxl.c                  |  9 ++++++---
 hw/display/virtio-gpu.c           |  6 ++++--
 hw/ide/ahci.c                     |  3 ++-
 hw/ide/core.c                     |  3 ++-
 hw/misc/imx_rngc.c                |  6 ++++--
 hw/misc/macio/mac_dbdma.c         |  2 +-
 hw/net/virtio-net.c               |  3 ++-
 hw/nvme/ctrl.c                    |  6 ++++--
 hw/scsi/mptsas.c                  |  3 ++-
 hw/scsi/scsi-bus.c                |  3 ++-
 hw/scsi/vmw_pvscsi.c              |  3 ++-
 hw/usb/dev-uas.c                  |  3 ++-
 hw/usb/hcd-dwc2.c                 |  3 ++-
 hw/usb/hcd-ehci.c                 |  3 ++-
 hw/usb/hcd-uhci.c                 |  2 +-
 hw/usb/host-libusb.c              |  6 ++++--
 hw/usb/redirect.c                 |  6 ++++--
 hw/usb/xen-usb.c                  |  3 ++-
 hw/virtio/virtio-balloon.c        |  5 +++--
 hw/virtio/virtio-crypto.c         |  3 ++-
 include/block/aio.h               | 18 ++++++++++++++++--
 include/hw/qdev-core.h            |  7 +++++++
 include/qemu/main-loop.h          |  7 +++++--
 softmmu/memory.c                  | 15 +++++++++++++++
 softmmu/trace-events              |  1 +
 tests/unit/ptimer-test-stubs.c    |  3 ++-
 util/async.c                      | 12 +++++++++++-
 util/main-loop.c                  |  5 +++--
 34 files changed, 128 insertions(+), 43 deletions(-)

-- 
2.39.0



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 0/3] memory: prevent dma-reentracy issues
@ 2023-01-19  7:01 Alexander Bulekov
  0 siblings, 0 replies; 7+ messages in thread
From: Alexander Bulekov @ 2023-01-19  7:01 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
	Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
	Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
	Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
	Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
	Eduardo Habkost, Jon Maloy, Siqi Chen

These patches aim to solve two types of DMA-reentrancy issues:

1.) mmio -> dma -> mmio case
To solve this, we track whether the device is engaged in io by
checking/setting a reentrancy-guard within APIs used for MMIO access.

2.) bh -> dma write -> mmio case
This case is trickier, since we dont have a generic way to associate a
bh with the underlying Device/DeviceState. Thus, this version allows a
device to associate a reentrancy-guard with a bh, when creating it.
(Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)

I replaced most of the qemu_bh_new invocations with the guarded analog,
except for the ones where the DeviceState was not trivially accessible

Unlike v3, these changes should address issues in devices that bypass
DMA apis and directly call into address_space.
e.g. https://gitlab.com/qemu-project/qemu/-/issues/827

v3 -> v4: Instead of changing all of the DMA APIs, instead add an
    optional reentrancy guard to the BH API.

v2 -> v3: Bite the bullet and modify the DMA APIs, rather than
    attempting to guess DeviceStates in BHs.


Alexander Bulekov (3):
  memory: prevent dma-reentracy issues
  async: Add an optional reentrancy guard to the BH API
  hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

Alexander Bulekov (3):
  memory: prevent dma-reentracy issues
  async: Add an optional reentrancy guard to the BH API
  hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

 docs/devel/multiple-iothreads.txt |  2 ++
 hw/9pfs/xen-9p-backend.c          |  4 +++-
 hw/block/dataplane/virtio-blk.c   |  3 ++-
 hw/block/dataplane/xen-block.c    |  5 +++--
 hw/block/virtio-blk.c             |  5 +++--
 hw/char/virtio-serial-bus.c       |  3 ++-
 hw/display/qxl.c                  |  9 ++++++---
 hw/display/virtio-gpu.c           |  6 ++++--
 hw/ide/ahci.c                     |  3 ++-
 hw/ide/core.c                     |  3 ++-
 hw/misc/imx_rngc.c                |  6 ++++--
 hw/misc/macio/mac_dbdma.c         |  2 +-
 hw/net/virtio-net.c               |  3 ++-
 hw/nvme/ctrl.c                    |  6 ++++--
 hw/scsi/mptsas.c                  |  3 ++-
 hw/scsi/scsi-bus.c                |  3 ++-
 hw/scsi/vmw_pvscsi.c              |  3 ++-
 hw/usb/dev-uas.c                  |  3 ++-
 hw/usb/hcd-dwc2.c                 |  3 ++-
 hw/usb/hcd-ehci.c                 |  3 ++-
 hw/usb/hcd-uhci.c                 |  2 +-
 hw/usb/host-libusb.c              |  6 ++++--
 hw/usb/redirect.c                 |  6 ++++--
 hw/usb/xen-usb.c                  |  3 ++-
 hw/virtio/virtio-balloon.c        |  5 +++--
 hw/virtio/virtio-crypto.c         |  3 ++-
 include/block/aio.h               | 18 ++++++++++++++++--
 include/hw/qdev-core.h            |  7 +++++++
 include/qemu/main-loop.h          |  7 +++++--
 softmmu/memory.c                  | 15 +++++++++++++++
 softmmu/trace-events              |  1 +
 tests/unit/ptimer-test-stubs.c    |  3 ++-
 util/async.c                      | 12 +++++++++++-
 util/main-loop.c                  |  5 +++--
 34 files changed, 128 insertions(+), 43 deletions(-)

-- 
2.39.0



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-01-26  5:20 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-19  7:00 [PATCH v4 0/3] memory: prevent dma-reentracy issues Alexander Bulekov
2023-01-19  7:00 ` [PATCH v4 1/3] " Alexander Bulekov
2023-01-20 14:41   ` Darren Kenny
2023-01-20 14:47     ` Peter Maydell
2023-01-26  5:19       ` Alexander Bulekov
2023-01-19  7:01 [PATCH v4 0/3] " Alexander Bulekov
2023-01-19  7:03 Alexander Bulekov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.