[PATCH v7 0/6] memory: prevent dma-reentracy issues

[PATCH v7 0/6] memory: prevent dma-reentracy issues

[Alexander Bulekov]

v6 -> v7:
    - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4)
    - Add an MR-specific flag to disable reentrancy (Patch 5)
    - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6)
    
    Patches 5 and 6 need review. I left the review-tags for Patch 4,
    however a few of the qemu_bh_new_guarded calls have changed.

v5 -> v6:
    - Only apply checkpatch checks to code in paths containing "/hw/"
      (/hw/ and include/hw/)
    - Fix a bug in a _guarded call added to hw/block/virtio-blk.c
v4-> v5:
    - Add corresponding checkpatch checks
    - Save/restore reentrancy-flag when entering/exiting BHs
    - Improve documentation
    - Check object_dynamic_cast return value

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.

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.


Alexander Bulekov (6):
  memory: prevent dma-reentracy issues
  async: Add an optional reentrancy guard to the BH API
  checkpatch: add qemu_bh_new/aio_bh_new checks
  hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
  memory: Allow disabling re-entrancy checking per-MR
  lsi53c895a: disable reentrancy detection for script RAM

 docs/devel/multiple-iothreads.txt |  7 +++++++
 hw/9pfs/xen-9p-backend.c          |  5 ++++-
 hw/block/dataplane/virtio-blk.c   |  3 ++-
 hw/block/dataplane/xen-block.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/ahci_internal.h            |  1 +
 hw/ide/core.c                     |  4 +++-
 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/lsi53c895a.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/exec/memory.h             |  3 +++
 include/hw/qdev-core.h            |  7 +++++++
 include/qemu/main-loop.h          |  7 +++++--
 scripts/checkpatch.pl             |  8 ++++++++
 softmmu/memory.c                  | 17 +++++++++++++++++
 softmmu/trace-events              |  1 +
 tests/unit/ptimer-test-stubs.c    |  3 ++-
 util/async.c                      | 18 +++++++++++++++++-
 util/main-loop.c                  |  5 +++--
 util/trace-events                 |  1 +
 38 files changed, 159 insertions(+), 41 deletions(-)

-- 
2.39.0

[PATCH v7 1/6] memory: prevent dma-reentracy issues

[Alexander Bulekov]

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
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Acked-by: Peter Xu <peterx@redhat.com>
---
 include/hw/qdev-core.h |  7 +++++++
 softmmu/memory.c       | 17 +++++++++++++++++
 softmmu/trace-events   |  1 +
 3 files changed, 25 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..7623703943 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 4699ba55ec..57bf18a257 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,19 @@ 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) {
+            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 +570,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

Re: [PATCH v7 1/6] memory: prevent dma-reentracy issues

[Philippe Mathieu-Daudé]

Hi Alex,

Sorry for the late review, *sigh*.

On 13/3/23 09:24, 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
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> 
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> Acked-by: Peter Xu <peterx@redhat.com>
> ---
>   include/hw/qdev-core.h |  7 +++++++
>   softmmu/memory.c       | 17 +++++++++++++++++
>   softmmu/trace-events   |  1 +
>   3 files changed, 25 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bd50ad5ee1..7623703943 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;

Do you plan to add more fields?

> +} 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;

At this point I'm not sure anymore this is a device or MR property.

>   };
>   
>   struct DeviceListener {
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 4699ba55ec..57bf18a257 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,19 @@ 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 */

Typo "simultaneous".

1/ access_with_adjusted_size() is complex enough and we are having hard
    time getting it right. I'd prefer we don't intermix size adjustment
    and re-entrancy check in the same function. This check could belong
    to the callers.

2/ I'm not keen on calling QOM object_dynamic_cast() in this hot path;
    and mixing QDev API within MR one. At least, can we cache this value
    once in memory_region_do_init() since we have access to @owner?

> +    if (mr->owner &&
> +        !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
> +        dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
> +        if (dev) {
> +            if (dev->mem_reentrancy_guard.engaged_in_io) {
> +                trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
> +                return MEMTX_ERROR;

MEMTX_ERROR is device-specific, I'm not sure it is right to return it
from this generic path. Maybe you meant MEMTX_ACCESS_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 +570,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>                           access_mask, attrs);
>           }
>       }
> +    if (dev) {
> +        dev->mem_reentrancy_guard.engaged_in_io = false;
> +    }
>       return r;
>   }

Re: [PATCH v7 1/6] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230313 0945, Philippe Mathieu-Daudé wrote:
> Hi Alex,
> 
> Sorry for the late review, *sigh*.
> 
> On 13/3/23 09:24, 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
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> > 
> > Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > Acked-by: Peter Xu <peterx@redhat.com>
> > ---
> >   include/hw/qdev-core.h |  7 +++++++
> >   softmmu/memory.c       | 17 +++++++++++++++++
> >   softmmu/trace-events   |  1 +
> >   3 files changed, 25 insertions(+)
> > 
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index bd50ad5ee1..7623703943 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;
> 
> Do you plan to add more fields?

Not right now, but maybe some need will come up.

> > +} 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;
> 
> At this point I'm not sure anymore this is a device or MR property.

It's designed to be an MR property. If it were MR specific, it wouldn't
handle the BH -> DMA case, or this one, where there are two MRs (doorbell
and oper) involed.
https://gitlab.com/qemu-project/qemu/-/issues/540

> 
> >   };
> >   struct DeviceListener {
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 4699ba55ec..57bf18a257 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,19 @@ 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 */
> 
> Typo "simultaneous".
> 
> 1/ access_with_adjusted_size() is complex enough and we are having hard
>    time getting it right. I'd prefer we don't intermix size adjustment
>    and re-entrancy check in the same function. This check could belong
>    to the callers.
> 

Would moving the code within this function to keep it separate from the
size adjustment be good enough? Otherwise we would end up with duplicate
code in the read/write callers.

The size-adjustment seems to be orthogonal (the MR won't change)?

> 2/ I'm not keen on calling QOM object_dynamic_cast() in this hot path;
>    and mixing QDev API within MR one. At least, can we cache this value
>    once in memory_region_do_init() since we have access to @owner?
>

Sounds like a good idea. Is it ever possible for the owner/owner's
address to change? 

Thanks
-Alex

> > +    if (mr->owner &&
> > +        !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
> > +        dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
> > +        if (dev) {
> > +            if (dev->mem_reentrancy_guard.engaged_in_io) {
> > +                trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
> > +                return MEMTX_ERROR;
> 
> MEMTX_ERROR is device-specific, I'm not sure it is right to return it
> from this generic path. Maybe you meant MEMTX_ACCESS_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 +570,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
> >                           access_mask, attrs);
> >           }
> >       }
> > +    if (dev) {
> > +        dev->mem_reentrancy_guard.engaged_in_io = false;
> > +    }
> >       return r;
> >   }
> 

Re: [PATCH v7 1/6] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230313 0515, Alexander Bulekov wrote:
> > 
> > At this point I'm not sure anymore this is a device or MR property.
> 
> It's designed to be an MR property. If it were MR specific, it wouldn't

Should be "It's designed to be a Device property."

Re: [PATCH v7 1/6] memory: prevent dma-reentracy issues

[Philippe Mathieu-Daudé]

On 13/3/23 09:24, 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
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282

BTW we need to commit these reproducers as tests/qtest/fuzz-*.

> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> Acked-by: Peter Xu <peterx@redhat.com>
> ---
>   include/hw/qdev-core.h |  7 +++++++
>   softmmu/memory.c       | 17 +++++++++++++++++
>   softmmu/trace-events   |  1 +
>   3 files changed, 25 insertions(+)

[PATCH v7 2/6] async: Add an optional reentrancy guard to the BH API

[Alexander Bulekov]

Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 docs/devel/multiple-iothreads.txt |  7 +++++++
 include/block/aio.h               | 18 ++++++++++++++++--
 include/qemu/main-loop.h          |  7 +++++--
 tests/unit/ptimer-test-stubs.c    |  3 ++-
 util/async.c                      | 18 +++++++++++++++++-
 util/main-loop.c                  |  5 +++--
 util/trace-events                 |  1 +
 7 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/docs/devel/multiple-iothreads.txt b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
  * LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
  * LEGACY timer_new_ms() - create a timer
  * LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
  * LEGACY qemu_aio_wait() - run an event loop iteration
 
 Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see include/block/aio.h):
  * aio_set_event_notifier() - monitor an event notifier
  * aio_timer_new() - create a timer
  * aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
  * aio_poll() - run an event loop iteration
 
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
 The AioContext can be obtained from the IOThread using
 iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
 Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index 8fba6a3584..3e3bdb9352 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
 #include "qemu/thread.h"
 #include "qemu/timer.h"
 #include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
 
 typedef struct BlockAIOCB BlockAIOCB;
 typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -331,9 +333,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
  * is opaque and must be allocated prior to its use.
  *
  * @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
  */
 QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-                        const char *name);
+                        const char *name, MemReentrancyGuard *reentrancy_guard);
 
 /**
  * aio_bh_new: Allocate a new bottom half structure
@@ -342,7 +346,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
  * string.
  */
 #define aio_bh_new(ctx, cb, opaque) \
-    aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+    aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+    aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
 
 /**
  * aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index c25f390696..84d1ce57f0 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -389,9 +389,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
 
 void qemu_fd_register(int fd);
 
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+    qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
 #define qemu_bh_new(cb, opaque) \
-    qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+    qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+                         MemReentrancyGuard *reentrancy_guard);
 void qemu_bh_schedule_idle(QEMUBH *bh);
 
 enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int attr_mask)
     return deadline;
 }
 
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+                         MemReentrancyGuard *reentrancy_guard)
 {
     QEMUBH *bh = g_new(QEMUBH, 1);
 
diff --git a/util/async.c b/util/async.c
index 21016a1ac7..a9b528c370 100644
--- a/util/async.c
+++ b/util/async.c
@@ -65,6 +65,7 @@ struct QEMUBH {
     void *opaque;
     QSLIST_ENTRY(QEMUBH) next;
     unsigned flags;
+    MemReentrancyGuard *reentrancy_guard;
 };
 
 /* Called concurrently from any thread */
@@ -137,7 +138,7 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx, QEMUBHFunc *cb,
 }
 
 QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-                        const char *name)
+                        const char *name, MemReentrancyGuard *reentrancy_guard)
 {
     QEMUBH *bh;
     bh = g_new(QEMUBH, 1);
@@ -146,13 +147,28 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
         .cb = cb,
         .opaque = opaque,
         .name = name,
+        .reentrancy_guard = reentrancy_guard,
     };
     return bh;
 }
 
 void aio_bh_call(QEMUBH *bh)
 {
+    bool last_engaged_in_io = false;
+
+    if (bh->reentrancy_guard) {
+        last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
+        if (bh->reentrancy_guard->engaged_in_io) {
+            trace_reentrant_aio(bh->ctx, bh->name);
+        }
+        bh->reentrancy_guard->engaged_in_io = true;
+    }
+
     bh->cb(bh->opaque);
+
+    if (bh->reentrancy_guard) {
+        bh->reentrancy_guard->engaged_in_io = last_engaged_in_io;
+    }
 }
 
 /* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
diff --git a/util/main-loop.c b/util/main-loop.c
index 3c0f525192..c4df36c6a5 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -616,9 +616,10 @@ void main_loop_wait(int nonblocking)
 
 /* Functions to operate on the main QEMU AioContext.  */
 
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name, MemReentrancyGuard *reentrancy_guard)
 {
-    return aio_bh_new_full(qemu_aio_context, cb, opaque, name);
+    return aio_bh_new_full(qemu_aio_context, cb, opaque, name,
+                           reentrancy_guard);
 }
 
 /*
diff --git a/util/trace-events b/util/trace-events
index 16f78d8fe5..3f7e766683 100644
--- a/util/trace-events
+++ b/util/trace-events
@@ -11,6 +11,7 @@ poll_remove(void *ctx, void *node, int fd) "ctx %p node %p fd %d"
 # async.c
 aio_co_schedule(void *ctx, void *co) "ctx %p co %p"
 aio_co_schedule_bh_cb(void *ctx, void *co) "ctx %p co %p"
+reentrant_aio(void *ctx, const char *name) "ctx %p name %s"
 
 # thread-pool.c
 thread_pool_submit(void *pool, void *req, void *opaque) "pool %p req %p opaque %p"
-- 
2.39.0

[PATCH v7 3/6] checkpatch: add qemu_bh_new/aio_bh_new checks

[Alexander Bulekov]

Advise authors to use the _guarded versions of the APIs, instead.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 scripts/checkpatch.pl | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d768171dcf..eeaec436eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2865,6 +2865,14 @@ sub process {
 		if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
 			ERROR("use sigaction to establish signal handlers; signal is not portable\n" . $herecurr);
 		}
+# recommend qemu_bh_new_guarded instead of qemu_bh_new
+        if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\bqemu_bh_new\s*\(/) {
+			ERROR("use qemu_bh_new_guarded() instead of qemu_bh_new() to avoid reentrancy problems\n" . $herecurr);
+		}
+# recommend aio_bh_new_guarded instead of aio_bh_new
+        if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\baio_bh_new\s*\(/) {
+			ERROR("use aio_bh_new_guarded() instead of aio_bh_new() to avoid reentrancy problems\n" . $herecurr);
+		}
 # check for module_init(), use category-specific init macros explicitly please
 		if ($line =~ /^module_init\s*\(/) {
 			ERROR("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr);
-- 
2.39.0

[PATCH v7 4/6] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Alexander Bulekov]

This protects devices from bh->mmio reentrancy issues.

Thanks: Thomas Huth <thuth@redhat.com> for diagnosing OS X test failure.
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 hw/9pfs/xen-9p-backend.c        | 5 ++++-
 hw/block/dataplane/virtio-blk.c | 3 ++-
 hw/block/dataplane/xen-block.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/ahci_internal.h          | 1 +
 hw/ide/core.c                   | 4 +++-
 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 ++-
 25 files changed, 66 insertions(+), 33 deletions(-)

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
 
     int num_rings;
     Xen9pfsRing *rings;
+    MemReentrancyGuard mem_reentrancy_guard;
 } Xen9pfsDev;
 
 static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
         xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
                                        XEN_FLEX_RING_SIZE(ring_order);
 
-        xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh, &xen_9pdev->rings[i]);
+        xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+                                                     &xen_9pdev->rings[i],
+                                                     &xen_9pdev->mem_reentrancy_guard);
         xen_9pdev->rings[i].out_cons = 0;
         xen_9pdev->rings[i].out_size = 0;
         xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     } else {
         s->ctx = qemu_get_aio_context();
     }
-    s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+    s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+                               &DEVICE(vdev)->mem_reentrancy_guard);
     s->batch_notify_vqs = bitmap_new(conf->num_queues);
 
     *dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice *xendev,
     } else {
         dataplane->ctx = qemu_get_aio_context();
     }
-    dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
-                               dataplane);
+    dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+                                       dataplane,
+                                       &DEVICE(xendev)->mem_reentrancy_guard);
 
     return dataplane;
 }
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    port->bh = qemu_bh_new(flush_queued_data_bh, port);
+    port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+                                   &dev->mem_reentrancy_guard);
     port->elem = NULL;
 }
 
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ec712d3ca2..c0460c4ef1 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
 
     qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
 
-    qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+    qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+                                          &DEVICE(qxl)->mem_reentrancy_guard);
     qxl_reset_state(qxl);
 
-    qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
-    qxl->ssd.cursor_bh = qemu_bh_new(qemu_spice_cursor_refresh_bh, &qxl->ssd);
+    qxl->update_area_bh = qemu_bh_new_guarded(qxl_render_update_area_bh, qxl,
+                                              &DEVICE(qxl)->mem_reentrancy_guard);
+    qxl->ssd.cursor_bh = qemu_bh_new_guarded(qemu_spice_cursor_refresh_bh, &qxl->ssd,
+                                             &DEVICE(qxl)->mem_reentrancy_guard);
 }
 
 static void qxl_realize_primary(PCIDevice *dev, Error **errp)
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5e15c79b94..66ac9b6cc5 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1339,8 +1339,10 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
 
     g->ctrl_vq = virtio_get_queue(vdev, 0);
     g->cursor_vq = virtio_get_queue(vdev, 1);
-    g->ctrl_bh = qemu_bh_new(virtio_gpu_ctrl_bh, g);
-    g->cursor_bh = qemu_bh_new(virtio_gpu_cursor_bh, g);
+    g->ctrl_bh = qemu_bh_new_guarded(virtio_gpu_ctrl_bh, g,
+                                     &qdev->mem_reentrancy_guard);
+    g->cursor_bh = qemu_bh_new_guarded(virtio_gpu_cursor_bh, g,
+                                       &qdev->mem_reentrancy_guard);
     QTAILQ_INIT(&g->reslist);
     QTAILQ_INIT(&g->cmdq);
     QTAILQ_INIT(&g->fenceq);
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 55902e1df7..4e76d6b191 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1509,7 +1509,8 @@ static void ahci_cmd_done(const IDEDMA *dma)
     ahci_write_fis_d2h(ad);
 
     if (ad->port_regs.cmd_issue && !ad->check_bh) {
-        ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
+        ad->check_bh = qemu_bh_new_guarded(ahci_check_cmd_bh, ad,
+                                           &ad->mem_reentrancy_guard);
         qemu_bh_schedule(ad->check_bh);
     }
 }
diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index 303fcd7235..2480455372 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -321,6 +321,7 @@ struct AHCIDevice {
     bool init_d2h_sent;
     AHCICmdHdr *cur_cmd;
     NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
+    MemReentrancyGuard mem_reentrancy_guard;
 };
 
 struct AHCIPCIState {
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 2d034731cf..50c8935366 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -513,6 +513,7 @@ BlockAIOCB *ide_issue_trim(
         BlockCompletionFunc *cb, void *cb_opaque, void *opaque)
 {
     IDEState *s = opaque;
+    IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;
     TrimAIOCB *iocb;
 
     /* Paired with a decrement in ide_trim_bh_cb() */
@@ -520,7 +521,8 @@ BlockAIOCB *ide_issue_trim(
 
     iocb = blk_aio_get(&trim_aiocb_info, s->blk, cb, cb_opaque);
     iocb->s = s;
-    iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
+    iocb->bh = qemu_bh_new_guarded(ide_trim_bh_cb, iocb,
+                                   &DEVICE(dev)->mem_reentrancy_guard);
     iocb->ret = 0;
     iocb->qiov = qiov;
     iocb->i = -1;
diff --git a/hw/misc/imx_rngc.c b/hw/misc/imx_rngc.c
index 632c03779c..082c6980ad 100644
--- a/hw/misc/imx_rngc.c
+++ b/hw/misc/imx_rngc.c
@@ -228,8 +228,10 @@ static void imx_rngc_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->iomem);
 
     sysbus_init_irq(sbd, &s->irq);
-    s->self_test_bh = qemu_bh_new(imx_rngc_self_test, s);
-    s->seed_bh = qemu_bh_new(imx_rngc_seed, s);
+    s->self_test_bh = qemu_bh_new_guarded(imx_rngc_self_test, s,
+                                          &dev->mem_reentrancy_guard);
+    s->seed_bh = qemu_bh_new_guarded(imx_rngc_seed, s,
+                                     &dev->mem_reentrancy_guard);
 }
 
 static void imx_rngc_reset(DeviceState *dev)
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 43bb1f56ba..80a789f32b 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -914,7 +914,7 @@ static void mac_dbdma_realize(DeviceState *dev, Error **errp)
 {
     DBDMAState *s = MAC_DBDMA(dev);
 
-    s->bh = qemu_bh_new(DBDMA_run_bh, s);
+    s->bh = qemu_bh_new_guarded(DBDMA_run_bh, s, &dev->mem_reentrancy_guard);
 }
 
 static void mac_dbdma_class_init(ObjectClass *oc, void *data)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 53e1c32643..447f669921 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2917,7 +2917,8 @@ static void virtio_net_add_queue(VirtIONet *n, int index)
         n->vqs[index].tx_vq =
             virtio_add_queue(vdev, n->net_conf.tx_queue_size,
                              virtio_net_handle_tx_bh);
-        n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
+        n->vqs[index].tx_bh = qemu_bh_new_guarded(virtio_net_tx_bh, &n->vqs[index],
+                                                  &DEVICE(vdev)->mem_reentrancy_guard);
     }
 
     n->vqs[index].tx_waiting = 0;
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 49c1210fce..62e4a1d7d9 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4604,7 +4604,8 @@ static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
         QTAILQ_INSERT_TAIL(&(sq->req_list), &sq->io_req[i], entry);
     }
 
-    sq->bh = qemu_bh_new(nvme_process_sq, sq);
+    sq->bh = qemu_bh_new_guarded(nvme_process_sq, sq,
+                                 &DEVICE(sq->ctrl)->mem_reentrancy_guard);
 
     if (n->dbbuf_enabled) {
         sq->db_addr = n->dbbuf_dbs + (sqid << 3);
@@ -5250,7 +5251,8 @@ static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, uint64_t dma_addr,
         }
     }
     n->cq[cqid] = cq;
-    cq->bh = qemu_bh_new(nvme_post_cqes, cq);
+    cq->bh = qemu_bh_new_guarded(nvme_post_cqes, cq,
+                                 &DEVICE(cq->ctrl)->mem_reentrancy_guard);
 }
 
 static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest *req)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index c485da792c..3de288b454 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1322,7 +1322,8 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
     }
     s->max_devices = MPTSAS_NUM_PORTS;
 
-    s->request_bh = qemu_bh_new(mptsas_fetch_requests, s);
+    s->request_bh = qemu_bh_new_guarded(mptsas_fetch_requests, s,
+                                        &DEVICE(dev)->mem_reentrancy_guard);
 
     scsi_bus_init(&s->bus, sizeof(s->bus), &dev->qdev, &mptsas_scsi_info);
 }
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index ceceafb2cd..e5c9f7a53d 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -193,7 +193,8 @@ static void scsi_dma_restart_cb(void *opaque, bool running, RunState state)
         AioContext *ctx = blk_get_aio_context(s->conf.blk);
         /* The reference is dropped in scsi_dma_restart_bh.*/
         object_ref(OBJECT(s));
-        s->bh = aio_bh_new(ctx, scsi_dma_restart_bh, s);
+        s->bh = aio_bh_new_guarded(ctx, scsi_dma_restart_bh, s,
+                                   &DEVICE(s)->mem_reentrancy_guard);
         qemu_bh_schedule(s->bh);
     }
 }
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index fa76696855..4de34536e9 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1184,7 +1184,8 @@ pvscsi_realizefn(PCIDevice *pci_dev, Error **errp)
         pcie_endpoint_cap_init(pci_dev, PVSCSI_EXP_EP_OFFSET);
     }
 
-    s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
+    s->completion_worker = qemu_bh_new_guarded(pvscsi_process_completion_queue, s,
+                                               &DEVICE(pci_dev)->mem_reentrancy_guard);
 
     scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(pci_dev), &pvscsi_scsi_info);
     /* override default SCSI bus hotplug-handler, with pvscsi's one */
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 88f99c05d5..f013ded91e 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -937,7 +937,8 @@ static void usb_uas_realize(USBDevice *dev, Error **errp)
 
     QTAILQ_INIT(&uas->results);
     QTAILQ_INIT(&uas->requests);
-    uas->status_bh = qemu_bh_new(usb_uas_send_status_bh, uas);
+    uas->status_bh = qemu_bh_new_guarded(usb_uas_send_status_bh, uas,
+                                         &d->mem_reentrancy_guard);
 
     dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
     scsi_bus_init(&uas->bus, sizeof(uas->bus), DEVICE(dev), &usb_uas_scsi_info);
diff --git a/hw/usb/hcd-dwc2.c b/hw/usb/hcd-dwc2.c
index 8755e9cbb0..a0c4e782b2 100644
--- a/hw/usb/hcd-dwc2.c
+++ b/hw/usb/hcd-dwc2.c
@@ -1364,7 +1364,8 @@ static void dwc2_realize(DeviceState *dev, Error **errp)
     s->fi = USB_FRMINTVL - 1;
     s->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, dwc2_frame_boundary, s);
     s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, dwc2_work_timer, s);
-    s->async_bh = qemu_bh_new(dwc2_work_bh, s);
+    s->async_bh = qemu_bh_new_guarded(dwc2_work_bh, s,
+                                      &dev->mem_reentrancy_guard);
 
     sysbus_init_irq(sbd, &s->irq);
 }
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index d4da8dcb8d..c930c60921 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2533,7 +2533,8 @@ void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp)
     }
 
     s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ehci_work_timer, s);
-    s->async_bh = qemu_bh_new(ehci_work_bh, s);
+    s->async_bh = qemu_bh_new_guarded(ehci_work_bh, s,
+                                      &dev->mem_reentrancy_guard);
     s->device = dev;
 
     s->vmstate = qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s);
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 8ac1175ad2..77baaa7a6b 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1190,7 +1190,7 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
                               USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
         }
     }
-    s->bh = qemu_bh_new(uhci_bh, s);
+    s->bh = qemu_bh_new_guarded(uhci_bh, s, &DEVICE(dev)->mem_reentrancy_guard);
     s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, uhci_frame_timer, s);
     s->num_ports_vmstate = NB_PORTS;
     QTAILQ_INIT(&s->queues);
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 176868d345..f500db85ab 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1141,7 +1141,8 @@ static void usb_host_nodev_bh(void *opaque)
 static void usb_host_nodev(USBHostDevice *s)
 {
     if (!s->bh_nodev) {
-        s->bh_nodev = qemu_bh_new(usb_host_nodev_bh, s);
+        s->bh_nodev = qemu_bh_new_guarded(usb_host_nodev_bh, s,
+                                          &DEVICE(s)->mem_reentrancy_guard);
     }
     qemu_bh_schedule(s->bh_nodev);
 }
@@ -1739,7 +1740,8 @@ static int usb_host_post_load(void *opaque, int version_id)
     USBHostDevice *dev = opaque;
 
     if (!dev->bh_postld) {
-        dev->bh_postld = qemu_bh_new(usb_host_post_load_bh, dev);
+        dev->bh_postld = qemu_bh_new_guarded(usb_host_post_load_bh, dev,
+                                             &DEVICE(dev)->mem_reentrancy_guard);
     }
     qemu_bh_schedule(dev->bh_postld);
     dev->bh_postld_pending = true;
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index fd7df599bc..39fbaaab16 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1441,8 +1441,10 @@ static void usbredir_realize(USBDevice *udev, Error **errp)
         }
     }
 
-    dev->chardev_close_bh = qemu_bh_new(usbredir_chardev_close_bh, dev);
-    dev->device_reject_bh = qemu_bh_new(usbredir_device_reject_bh, dev);
+    dev->chardev_close_bh = qemu_bh_new_guarded(usbredir_chardev_close_bh, dev,
+                                                &DEVICE(dev)->mem_reentrancy_guard);
+    dev->device_reject_bh = qemu_bh_new_guarded(usbredir_device_reject_bh, dev,
+                                                &DEVICE(dev)->mem_reentrancy_guard);
     dev->attach_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, usbredir_do_attach, dev);
 
     packet_id_queue_init(&dev->cancelled, dev, "cancelled");
diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 66cb3f7c24..38ee660a30 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -1032,7 +1032,8 @@ static void usbback_alloc(struct XenLegacyDevice *xendev)
 
     QTAILQ_INIT(&usbif->req_free_q);
     QSIMPLEQ_INIT(&usbif->hotplug_q);
-    usbif->bh = qemu_bh_new(usbback_bh, usbif);
+    usbif->bh = qemu_bh_new_guarded(usbback_bh, usbif,
+                                    &DEVICE(xendev)->mem_reentrancy_guard);
 }
 
 static int usbback_free(struct XenLegacyDevice *xendev)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 746f07c4d2..d60dd1f61e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -908,8 +908,9 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
         precopy_add_notifier(&s->free_page_hint_notify);
 
         object_ref(OBJECT(s->iothread));
-        s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
-                                     virtio_ballloon_get_free_page_hints, s);
+        s->free_page_bh = aio_bh_new_guarded(iothread_get_aio_context(s->iothread),
+                                             virtio_ballloon_get_free_page_hints, s,
+                                             &dev->mem_reentrancy_guard);
     }
 
     if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 802e1b9659..2fe804510f 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -1074,7 +1074,8 @@ static void virtio_crypto_device_realize(DeviceState *dev, Error **errp)
         vcrypto->vqs[i].dataq =
                  virtio_add_queue(vdev, 1024, virtio_crypto_handle_dataq_bh);
         vcrypto->vqs[i].dataq_bh =
-                 qemu_bh_new(virtio_crypto_dataq_bh, &vcrypto->vqs[i]);
+                 qemu_bh_new_guarded(virtio_crypto_dataq_bh, &vcrypto->vqs[i],
+                                     &dev->mem_reentrancy_guard);
         vcrypto->vqs[i].vcrypto = vcrypto;
     }
 
-- 
2.39.0

Re: [PATCH v7 4/6] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Thomas Huth]

On 13/03/2023 09.24, Alexander Bulekov wrote:
> This protects devices from bh->mmio reentrancy issues.
> 
> Thanks: Thomas Huth <thuth@redhat.com> for diagnosing OS X test failure.
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>   hw/9pfs/xen-9p-backend.c        | 5 ++++-
>   hw/block/dataplane/virtio-blk.c | 3 ++-
>   hw/block/dataplane/xen-block.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/ahci_internal.h          | 1 +
>   hw/ide/core.c                   | 4 +++-
>   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 ++-
>   25 files changed, 66 insertions(+), 33 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v7 5/6] memory: Allow disabling re-entrancy checking per-MR

[Alexander Bulekov]

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 include/exec/memory.h | 3 +++
 softmmu/memory.c      | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6fa0b071f0..5154b123d8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -791,6 +791,9 @@ struct MemoryRegion {
     unsigned ioeventfd_nb;
     MemoryRegionIoeventfd *ioeventfds;
     RamDiscardManager *rdm; /* Only for RAM */
+
+    /* For devices designed to perform re-entrant IO into their own IO MRs */
+    bool disable_reentrancy_guard;
 };
 
 struct IOMMUMemoryRegion {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 57bf18a257..3018fa2edb 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -544,7 +544,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
     }
 
     /* Do not allow more than one simultanous access to a device's IO Regions */
-    if (mr->owner &&
+    if (mr->owner && !mr->disable_reentrancy_guard &&
         !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
         dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
         if (dev) {
-- 
2.39.0

Re: [PATCH v7 5/6] memory: Allow disabling re-entrancy checking per-MR

[Thomas Huth]

On 13/03/2023 09.24, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>   include/exec/memory.h | 3 +++
>   softmmu/memory.c      | 2 +-
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 6fa0b071f0..5154b123d8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -791,6 +791,9 @@ struct MemoryRegion {
>       unsigned ioeventfd_nb;
>       MemoryRegionIoeventfd *ioeventfds;
>       RamDiscardManager *rdm; /* Only for RAM */
> +
> +    /* For devices designed to perform re-entrant IO into their own IO MRs */
> +    bool disable_reentrancy_guard;
>   };
>   
>   struct IOMMUMemoryRegion {
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 57bf18a257..3018fa2edb 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -544,7 +544,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>       }
>   
>       /* Do not allow more than one simultanous access to a device's IO Regions */
> -    if (mr->owner &&
> +    if (mr->owner && !mr->disable_reentrancy_guard &&
>           !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
>           dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
>           if (dev) {

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v7 5/6] memory: Allow disabling re-entrancy checking per-MR

[Darren Kenny]

On Monday, 2023-03-13 at 04:24:16 -04, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  include/exec/memory.h | 3 +++
>  softmmu/memory.c      | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 6fa0b071f0..5154b123d8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -791,6 +791,9 @@ struct MemoryRegion {
>      unsigned ioeventfd_nb;
>      MemoryRegionIoeventfd *ioeventfds;
>      RamDiscardManager *rdm; /* Only for RAM */
> +
> +    /* For devices designed to perform re-entrant IO into their own IO MRs */
> +    bool disable_reentrancy_guard;
>  };
>  
>  struct IOMMUMemoryRegion {
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 57bf18a257..3018fa2edb 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -544,7 +544,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>      }
>  
>      /* Do not allow more than one simultanous access to a device's IO Regions */
> -    if (mr->owner &&
> +    if (mr->owner && !mr->disable_reentrancy_guard &&
>          !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
>          dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
>          if (dev) {
> -- 
> 2.39.0

[PATCH v7 6/6] lsi53c895a: disable reentrancy detection for script RAM

[Alexander Bulekov]

As the code is designed to use the memory APIs to access the script ram,
disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion.

In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion.

Reported-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 hw/scsi/lsi53c895a.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index af93557a9a..db27872963 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -2302,6 +2302,12 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
     memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s,
                           "lsi-io", 256);
 
+    /*
+     * Since we use the address-space API to interact with ram_io, disable the
+     * re-entrancy guard.
+     */
+    s->ram_io.disable_reentrancy_guard = true;
+
     address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io");
     qdev_init_gpio_out(d, &s->ext_irq, 1);
 
-- 
2.39.0

Re: [PATCH v7 6/6] lsi53c895a: disable reentrancy detection for script RAM

[Thomas Huth]

On 13/03/2023 09.24, Alexander Bulekov wrote:
> As the code is designed to use the memory APIs to access the script ram,
> disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion.
> 
> In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion.
> 
> Reported-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>   hw/scsi/lsi53c895a.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index af93557a9a..db27872963 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -2302,6 +2302,12 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
>       memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s,
>                             "lsi-io", 256);
>   
> +    /*
> +     * Since we use the address-space API to interact with ram_io, disable the
> +     * re-entrancy guard.
> +     */
> +    s->ram_io.disable_reentrancy_guard = true;
> +
>       address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io");
>       qdev_init_gpio_out(d, &s->ext_irq, 1);
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v7 6/6] lsi53c895a: disable reentrancy detection for script RAM

[Darren Kenny]

On Monday, 2023-03-13 at 04:24:17 -04, Alexander Bulekov wrote:
> As the code is designed to use the memory APIs to access the script ram,
> disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion.
>
> In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion.
>
> Reported-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  hw/scsi/lsi53c895a.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index af93557a9a..db27872963 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -2302,6 +2302,12 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
>      memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s,
>                            "lsi-io", 256);
>  
> +    /*
> +     * Since we use the address-space API to interact with ram_io, disable the
> +     * re-entrancy guard.
> +     */
> +    s->ram_io.disable_reentrancy_guard = true;
> +
>      address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io");
>      qdev_init_gpio_out(d, &s->ext_irq, 1);
>  
> -- 
> 2.39.0

Re: [PATCH v7 6/6] lsi53c895a: disable reentrancy detection for script RAM

[Michael Tokarev]

13.03.2023 11:24, Alexander Bulekov пишет:
> As the code is designed to use the memory APIs to access the script ram,
> disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion.
> 
> In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion.

Ping?  Maybe it's worth to re-send this one, after it got 2 R-by's.

Does it close CVE-2023-0330 ?

Thanks,

/mjt

Re: [PATCH v7 0/6] memory: prevent dma-reentracy issues

[Thomas Huth]

On 13/03/2023 09.24, Alexander Bulekov wrote:
> v6 -> v7:
>      - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4)
>      - Add an MR-specific flag to disable reentrancy (Patch 5)
>      - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6)
>      
>      Patches 5 and 6 need review. I left the review-tags for Patch 4,
>      however a few of the qemu_bh_new_guarded calls have changed.

  Hi Alexander,

there seems to be another issue with one of the avocado tests:

  make -j8 qemu-system-aarch64
  make check-venv
  ./tests/venv/bin/avocado run \
    tests/avocado/boot_linux_console.py:BootLinuxConsole.test_aarch64_raspi3_atf

... works fine for me with the master branch, but it fails
for me after applying your patch series.
Can you reproduce that failure?

  Thomas

Re: [PATCH v7 0/6] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230313 1502, Thomas Huth wrote:
> On 13/03/2023 09.24, Alexander Bulekov wrote:
> > v6 -> v7:
> >      - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4)
> >      - Add an MR-specific flag to disable reentrancy (Patch 5)
> >      - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6)
> >      Patches 5 and 6 need review. I left the review-tags for Patch 4,
> >      however a few of the qemu_bh_new_guarded calls have changed.
> 
>  Hi Alexander,
> 
> there seems to be another issue with one of the avocado tests:
> 
>  make -j8 qemu-system-aarch64
>  make check-venv
>  ./tests/venv/bin/avocado run \
>    tests/avocado/boot_linux_console.py:BootLinuxConsole.test_aarch64_raspi3_atf
> 
> ... works fine for me with the master branch, but it fails
> for me after applying your patch series.
> Can you reproduce that failure?

#0  __GI_exit (status=0x1) at ./stdlib/exit.c:143
#1  0x0000555555f05819 in access_with_adjusted_size (addr=0x0, addr@entry=0x7ffff3b609d0, value=0x7ffff3b609d0, size=size@entry=0x4, access_size_min=0x1, access_size_max=0x4, access_fn=0x555555f0b4b0 <memory_region_read_accessor>, mr=0x7
#2  0x0000555555f05380 in memory_region_dispatch_read1 (mr=0x7ffff3e34990, addr=0x1, pval=<optimized out>, size=0x4, attrs=...) at ../softmmu/memory.c:1442
#3  memory_region_dispatch_read (mr=<optimized out>, mr@entry=0x7ffff3e34990, addr=0x1, pval=<optimized out>, pval@entry=0x7ffff3b609d0, op=<optimized out>, attrs=..., attrs@entry=...) at ../softmmu/memory.c:1476
#4  0x0000555555f1278f in address_space_ldl_internal (as=<optimized out>, addr=<optimized out>, attrs=..., result=0x0, endian=DEVICE_LITTLE_ENDIAN) at ../memory_ldst.c.inc:41
#5  0x00005555559ebb5d in ldl_le_phys (as=0x7ffff3e35258, addr=0x80) at /home/alxndr/Development/qemu-demo/qemu/include/exec/memory_ldst_phys.h.inc:79
#6  bcm2835_mbox_update (s=0x7ffff3e34f20) at ../hw/misc/bcm2835_mbox.c:109
#7  0x00005555559ecd5d in bcm2835_property_write (opaque=0x7ffff3e34600, offset=<optimized out>, value=<optimized out>, size=<optimized out>) at ../hw/misc/bcm2835_property.c:349
#8  0x0000555555f05903 in memory_region_write_accessor (mr=0x7ffff3e34990, addr=0x0, value=<optimized out>, size=0x4, shift=<optimized out>, mask=<optimized out>, attrs=...) at ../softmmu/memory.c:493
#9  0x0000555555f0576b in access_with_adjusted_size (addr=addr@entry=0x0, value=0x7ffff3b60c38, value@entry=0x7ffff3b60c28, size=size@entry=0x4, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=0x555555f05820 <
attrs=...) at ../softmmu/memory.c:570
#10 0x0000555555f055c6 in memory_region_dispatch_write (mr=<optimized out>, mr@entry=0x7ffff3e34990, addr=0x0, data=<optimized out>, data@entry=0x2f2228, op=<optimized out>, attrs=..., attrs@entry=...) at ../softmmu/memory.c:1532
#11 0x0000555555f132ec in address_space_stl_internal (as=<optimized out>, addr=<optimized out>, val=0x2f2228, attrs=..., result=0x0, endian=DEVICE_LITTLE_ENDIAN) at ../memory_ldst.c.inc:319
#12 0x00005555559eb9a4 in stl_le_phys (as=<optimized out>, addr=0x80, val=0x2f2228) at /home/alxndr/Development/qemu-demo/qemu/include/exec/memory_ldst_phys.h.inc:121
#13 bcm2835_mbox_write (opaque=0x7ffff3e34f20, offset=<optimized out>, value=0x2f2228, size=<optimized out>) at ../hw/misc/bcm2835_mbox.c:227
#14 0x0000555555f05903 in memory_region_write_accessor (mr=0x7ffff3e352b0, addr=0xa0, value=<optimized out>, size=0x4, shift=<optimized out>, mask=<optimized out>, attrs=...) at ../softmmu/memory.c:493
#15 0x0000555555f0576b in access_with_adjusted_size (addr=addr@entry=0xa0, value=0x7ffff3b60e48, value@entry=0x7ffff3b60e38, size=size@entry=0x4, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=0x555555f05820 
 attrs=...) at ../softmmu/memory.c:570
#16 0x0000555555f055c6 in memory_region_dispatch_write (mr=<optimized out>, mr@entry=0x2, addr=addr@entry=0xa0, data=<optimized out>, data@entry=0x2f2228, op=<optimized out>, op@entry=MO_32, attrs=...) at ../softmmu/memory.c:1532
#17 0x0000555555f9b3ae in io_writex (env=0x7ffff3dd60e0, full=0x55555790c710, mmu_idx=0x7, val=0x4, val@entry=0x2f2228, addr=0x3f00b8a0, retaddr=retaddr@entry=0x7fffac01f9dd, op=MO_32) at ../accel/tcg/cputlb.c:1430
#18 0x0000555555f90062 in store_helper (env=<optimized out>, addr=<optimized out>, val=0x2f2228, oi=<optimized out>, retaddr=0x7ffff3b609d0, op=MO_32) at ../accel/tcg/cputlb.c:2454
#19 full_le_stl_mmu (env=<optimized out>, addr=<optimized out>, val=0x2f2228, oi=<optimized out>, retaddr=0x7ffff3b609d0) at ../accel/tcg/cputlb.c:2542
#20 0x00007fffac01f9dd in code_gen_buffer ()
#21 0x0000555555f7367e in cpu_tb_exec (cpu=cpu@entry=0x7ffff3dd4210, itb=itb@entry=0x7fffac01f8c0 <code_gen_buffer+129171>, tb_exit=tb_exit@entry=0x7ffff3b6148c) at ../accel/tcg/cpu-exec.c:460
#22 0x0000555555f744f9 in cpu_loop_exec_tb (cpu=0x7ffff3dd4210, tb=<optimized out>, pc=<optimized out>, tb_exit=0x7ffff3b6148c, last_tb=<optimized out>) at ../accel/tcg/cpu-exec.c:894
#23 cpu_exec_loop (cpu=cpu@entry=0x7ffff3dd4210, sc=sc@entry=0x7ffff3b61510) at ../accel/tcg/cpu-exec.c:1005
#24 0x0000555555f73c27 in cpu_exec_setjmp (cpu=cpu@entry=0x7ffff3dd4210, sc=sc@entry=0x7ffff3b61510) at ../accel/tcg/cpu-exec.c:1037
#25 0x0000555555f73aee in cpu_exec (cpu=cpu@entry=0x7ffff3dd4210) at ../accel/tcg/cpu-exec.c:1063
#26 0x0000555555f9da4f in tcg_cpus_exec (cpu=cpu@entry=0x7ffff3dd4210) at ../accel/tcg/tcg-accel-ops.c:81
#27 0x0000555555f9e019 in mttcg_cpu_thread_fn (arg=arg@entry=0x7ffff3dd4210) at ../accel/tcg/tcg-accel-ops-mttcg.c:95
#28 0x000055555611d0c5 in qemu_thread_start (args=0x555557923bf0) at ../util/qemu-thread-posix.c:541
#29 0x00007ffff6960fd4 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
#30 0x00007ffff69e166c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Some sort of relationship between bcm2835_property and
bcm2835_mbox 

bcm2835_property calls directly into bcm2835_mbox which reads from
bcm2835_property..

Guess bcm2835_property s->iomem needs to be marked as reentrancy-safe
as-well.

Do the avocado tests exit on failure, or do you know if there are any
other test failures?
Thank you
-Alex

Re: [PATCH v7 0/6] memory: prevent dma-reentracy issues

[Philippe Mathieu-Daudé]

On 13/3/23 15:52, Alexander Bulekov wrote:
> On 230313 1502, Thomas Huth wrote:
>> On 13/03/2023 09.24, Alexander Bulekov wrote:
>>> v6 -> v7:
>>>       - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4)
>>>       - Add an MR-specific flag to disable reentrancy (Patch 5)
>>>       - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6)
>>>       Patches 5 and 6 need review. I left the review-tags for Patch 4,
>>>       however a few of the qemu_bh_new_guarded calls have changed.
>>
>>   Hi Alexander,
>>
>> there seems to be another issue with one of the avocado tests:
>>
>>   make -j8 qemu-system-aarch64
>>   make check-venv
>>   ./tests/venv/bin/avocado run \
>>     tests/avocado/boot_linux_console.py:BootLinuxConsole.test_aarch64_raspi3_atf
>>
>> ... works fine for me with the master branch, but it fails
>> for me after applying your patch series.
>> Can you reproduce that failure?
> 
> #0  __GI_exit (status=0x1) at ./stdlib/exit.c:143
> #1  0x0000555555f05819 in access_with_adjusted_size (addr=0x0, addr@entry=0x7ffff3b609d0, value=0x7ffff3b609d0, size=size@entry=0x4, access_size_min=0x1, access_size_max=0x4, access_fn=0x555555f0b4b0 <memory_region_read_accessor>, mr=0x7
> #2  0x0000555555f05380 in memory_region_dispatch_read1 (mr=0x7ffff3e34990, addr=0x1, pval=<optimized out>, size=0x4, attrs=...) at ../softmmu/memory.c:1442
> #3  memory_region_dispatch_read (mr=<optimized out>, mr@entry=0x7ffff3e34990, addr=0x1, pval=<optimized out>, pval@entry=0x7ffff3b609d0, op=<optimized out>, attrs=..., attrs@entry=...) at ../softmmu/memory.c:1476
> #4  0x0000555555f1278f in address_space_ldl_internal (as=<optimized out>, addr=<optimized out>, attrs=..., result=0x0, endian=DEVICE_LITTLE_ENDIAN) at ../memory_ldst.c.inc:41
> #5  0x00005555559ebb5d in ldl_le_phys (as=0x7ffff3e35258, addr=0x80) at /home/alxndr/Development/qemu-demo/qemu/include/exec/memory_ldst_phys.h.inc:79
> #6  bcm2835_mbox_update (s=0x7ffff3e34f20) at ../hw/misc/bcm2835_mbox.c:109
> #7  0x00005555559ecd5d in bcm2835_property_write (opaque=0x7ffff3e34600, offset=<optimized out>, value=<optimized out>, size=<optimized out>) at ../hw/misc/bcm2835_property.c:349
> #8  0x0000555555f05903 in memory_region_write_accessor (mr=0x7ffff3e34990, addr=0x0, value=<optimized out>, size=0x4, shift=<optimized out>, mask=<optimized out>, attrs=...) at ../softmmu/memory.c:493
> #9  0x0000555555f0576b in access_with_adjusted_size (addr=addr@entry=0x0, value=0x7ffff3b60c38, value@entry=0x7ffff3b60c28, size=size@entry=0x4, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=0x555555f05820 <
> attrs=...) at ../softmmu/memory.c:570
> #10 0x0000555555f055c6 in memory_region_dispatch_write (mr=<optimized out>, mr@entry=0x7ffff3e34990, addr=0x0, data=<optimized out>, data@entry=0x2f2228, op=<optimized out>, attrs=..., attrs@entry=...) at ../softmmu/memory.c:1532
> #11 0x0000555555f132ec in address_space_stl_internal (as=<optimized out>, addr=<optimized out>, val=0x2f2228, attrs=..., result=0x0, endian=DEVICE_LITTLE_ENDIAN) at ../memory_ldst.c.inc:319
> #12 0x00005555559eb9a4 in stl_le_phys (as=<optimized out>, addr=0x80, val=0x2f2228) at /home/alxndr/Development/qemu-demo/qemu/include/exec/memory_ldst_phys.h.inc:121
> #13 bcm2835_mbox_write (opaque=0x7ffff3e34f20, offset=<optimized out>, value=0x2f2228, size=<optimized out>) at ../hw/misc/bcm2835_mbox.c:227
> #14 0x0000555555f05903 in memory_region_write_accessor (mr=0x7ffff3e352b0, addr=0xa0, value=<optimized out>, size=0x4, shift=<optimized out>, mask=<optimized out>, attrs=...) at ../softmmu/memory.c:493
> #15 0x0000555555f0576b in access_with_adjusted_size (addr=addr@entry=0xa0, value=0x7ffff3b60e48, value@entry=0x7ffff3b60e38, size=size@entry=0x4, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=0x555555f05820
>   attrs=...) at ../softmmu/memory.c:570
> #16 0x0000555555f055c6 in memory_region_dispatch_write (mr=<optimized out>, mr@entry=0x2, addr=addr@entry=0xa0, data=<optimized out>, data@entry=0x2f2228, op=<optimized out>, op@entry=MO_32, attrs=...) at ../softmmu/memory.c:1532
> #17 0x0000555555f9b3ae in io_writex (env=0x7ffff3dd60e0, full=0x55555790c710, mmu_idx=0x7, val=0x4, val@entry=0x2f2228, addr=0x3f00b8a0, retaddr=retaddr@entry=0x7fffac01f9dd, op=MO_32) at ../accel/tcg/cputlb.c:1430
> #18 0x0000555555f90062 in store_helper (env=<optimized out>, addr=<optimized out>, val=0x2f2228, oi=<optimized out>, retaddr=0x7ffff3b609d0, op=MO_32) at ../accel/tcg/cputlb.c:2454
> #19 full_le_stl_mmu (env=<optimized out>, addr=<optimized out>, val=0x2f2228, oi=<optimized out>, retaddr=0x7ffff3b609d0) at ../accel/tcg/cputlb.c:2542
> #20 0x00007fffac01f9dd in code_gen_buffer ()
> #21 0x0000555555f7367e in cpu_tb_exec (cpu=cpu@entry=0x7ffff3dd4210, itb=itb@entry=0x7fffac01f8c0 <code_gen_buffer+129171>, tb_exit=tb_exit@entry=0x7ffff3b6148c) at ../accel/tcg/cpu-exec.c:460
> #22 0x0000555555f744f9 in cpu_loop_exec_tb (cpu=0x7ffff3dd4210, tb=<optimized out>, pc=<optimized out>, tb_exit=0x7ffff3b6148c, last_tb=<optimized out>) at ../accel/tcg/cpu-exec.c:894
> #23 cpu_exec_loop (cpu=cpu@entry=0x7ffff3dd4210, sc=sc@entry=0x7ffff3b61510) at ../accel/tcg/cpu-exec.c:1005
> #24 0x0000555555f73c27 in cpu_exec_setjmp (cpu=cpu@entry=0x7ffff3dd4210, sc=sc@entry=0x7ffff3b61510) at ../accel/tcg/cpu-exec.c:1037
> #25 0x0000555555f73aee in cpu_exec (cpu=cpu@entry=0x7ffff3dd4210) at ../accel/tcg/cpu-exec.c:1063
> #26 0x0000555555f9da4f in tcg_cpus_exec (cpu=cpu@entry=0x7ffff3dd4210) at ../accel/tcg/tcg-accel-ops.c:81
> #27 0x0000555555f9e019 in mttcg_cpu_thread_fn (arg=arg@entry=0x7ffff3dd4210) at ../accel/tcg/tcg-accel-ops-mttcg.c:95
> #28 0x000055555611d0c5 in qemu_thread_start (args=0x555557923bf0) at ../util/qemu-thread-posix.c:541
> #29 0x00007ffff6960fd4 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
> #30 0x00007ffff69e166c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> 
> Some sort of relationship between bcm2835_property and
> bcm2835_mbox
> 
> bcm2835_property calls directly into bcm2835_mbox which reads from
> bcm2835_property..

These models emulate (synchronously within vCPU) a vDSP firmware:
another core accessing the address space asynchronously.

> Guess bcm2835_property s->iomem needs to be marked as reentrancy-safe
> as-well.

Right.

Now I wonder again if this is a good time to merge this change set.
Hopefully users will test the RC before the final release.

Re: [PATCH v7 0/6] memory: prevent dma-reentracy issues

[Peter Maydell]

On Mon, 13 Mar 2023 at 15:41, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> Now I wonder again if this is a good time to merge this change set.

No, I don't think it is at this point in the release
cycle. I would vote for merging it when we reopen for 8.1,
so that we'll have a full cycle to find all the weird corner
cases that it breaks.

thanks
-- PMM

Re: [PATCH v7 0/6] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230313 1608, Peter Maydell wrote:
> On Mon, 13 Mar 2023 at 15:41, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > Now I wonder again if this is a good time to merge this change set.
> 
> No, I don't think it is at this point in the release
> cycle. I would vote for merging it when we reopen for 8.1,
> so that we'll have a full cycle to find all the weird corner
> cases that it breaks.

Ok. I'll fix the rasbpi issue and look into adding some compile-time
option to make the re-entrancy check fatal. That way we might catch
additional edge-cases with fuzzing (though non-x86 coverage is poor) and
unit-tests.
-Alex

Re: [PATCH v7 0/6] memory: prevent dma-reentracy issues

[Thomas Huth]

On 13/03/2023 15.52, Alexander Bulekov wrote:
> On 230313 1502, Thomas Huth wrote:
>> On 13/03/2023 09.24, Alexander Bulekov wrote:
>>> v6 -> v7:
>>>       - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4)
>>>       - Add an MR-specific flag to disable reentrancy (Patch 5)
>>>       - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6)
>>>       Patches 5 and 6 need review. I left the review-tags for Patch 4,
>>>       however a few of the qemu_bh_new_guarded calls have changed.
>>
>>   Hi Alexander,
>>
>> there seems to be another issue with one of the avocado tests:
>>
>>   make -j8 qemu-system-aarch64
>>   make check-venv
>>   ./tests/venv/bin/avocado run \
>>     tests/avocado/boot_linux_console.py:BootLinuxConsole.test_aarch64_raspi3_atf
>>
>> ... works fine for me with the master branch, but it fails
>> for me after applying your patch series.
...
> Do the avocado tests exit on failure, or do you know if there are any
> other test failures?

I noticed it in the gitlab-CI, the test was hanging and got marked as 
"INTERRUPTED":

  https://gitlab.com/thuth/qemu/-/jobs/3922243532#L214

As far as I could see, this was the only new failure there. There is another 
one in the avocado-system-fedora job here:

  https://gitlab.com/thuth/qemu/-/jobs/3920337136#L307

... but I think that was pre-existing and was caused by one of Philippe's 
reworks, hopefully to be fixed soon ... Phillipe?

  Thomas

Re: [PATCH v7 0/6] memory: prevent dma-reentracy issues

[Philippe Mathieu-Daudé]

On 13/3/23 17:18, Thomas Huth wrote:

>   https://gitlab.com/thuth/qemu/-/jobs/3920337136#L307
> 
> ... but I think that was pre-existing and was caused by one of 
> Philippe's reworks, hopefully to be fixed soon ... Phillipe?

Jiaxun fixed this on little-endian hosts, but this is still
failing on big-endian ones so I was a bit reluctant until figuring
out the full fix but since this helps CI...