Re: [PATCH v4 2/3] async: Add an optional reentrancy guard to the BH API

[Stefan Hajnoczi]

On Thu, Jan 19, 2023 at 02:03:07AM -0500, Alexander Bulekov wrote:
> 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.
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  docs/devel/multiple-iothreads.txt |  2 ++
>  include/block/aio.h               | 18 ++++++++++++++++--
>  include/qemu/main-loop.h          |  7 +++++--
>  tests/unit/ptimer-test-stubs.c    |  3 ++-
>  util/async.c                      | 12 +++++++++++-
>  util/main-loop.c                  |  5 +++--
>  6 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/devel/multiple-iothreads.txt 
> b/docs/devel/multiple-iothreads.txt
> index 343120f2ef..e4fafed9d9 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,6 +73,7 @@ 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 AioContext can be obtained from the IOThread using
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 0f65a3cc9e..94d661ff7e 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);
> @@ -332,9 +334,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
> @@ -343,7 +347,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 f5e75a96b6..24d5413f9d 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 14d63b3091..08924c3212 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 */
> @@ -133,7 +134,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);
> @@ -142,13 +143,22 @@ 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)
>  {
> +    if (bh->reentrancy_guard) {
> +        bh->reentrancy_guard->engaged_in_io = true;
> +    }
> +
>      bh->cb(bh->opaque);
> +
> +    if (bh->reentrancy_guard) {
> +        bh->reentrancy_guard->engaged_in_io = false;
> +    }
>  }

QEMU supports nested event loops. I think aio_bh_call() -> cb() ->
aio_poll() -> aio_bh_call() -> ... is possible although it should be
rare.

->engaged_in_io will set to false after the innermost aio_bh_call()
returns. Therefore the protection doesn't cover the remainder of the
parent cb() functions.

I think aio_bh_call() should be:

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

That way nested aio_poll() calls work as expected.

This also raises the question whether aio_bh_call() should call abort(3)
if ->engaged_in_io is already true when the function is entered? I think
that may be too strict, but I'm not sure. A scenario where this can
happen:

The memory region read/write function calls aio_poll() -> aio_bh_call()
and a BH with our device's re-entrancy guard is executed.

>  
>  /* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 58f776a8c9..07d2e2040a 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -617,9 +617,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);
>  }
>  
>  /*
> -- 
> 2.39.0
>

signature.asc
Description: PGP signature