On Wed, Jun 04, 2025 at 03:18:43PM -0400, Stefan Hajnoczi wrote:
> Since commit 7ff9ff039380 ("meson: mitigate against use of uninitialize
> stack for exploits") the -ftrivial-auto-var-init=zero compiler option is
> used to zero local variables. While this reduces security risks
> associated with uninitialized stack data, it introduced a measurable
> bottleneck in the virtqueue_split_pop() and virtqueue_packed_pop()
> functions.
>
> These virtqueue functions are in the hot path. They are called for each
> element (request) that is popped from a VIRTIO device's virtqueue. Using
> __attribute__((uninitialized)) on large stack variables in these
> functions improves fio randread bs=4k iodepth=64 performance from 304k
> to 332k IOPS (+9%).
IIUC, the 'hwaddr addr' variable is 8k in size, and the 'struct iovec iov'
array is 16k in size, so we have 24k on the stack that we're clearing and
then later writing the real value. Makes sense that this would have a
perf impact in a hotpath.
> This issue was found using perf-top(1). virtqueue_split_pop() was one of
> the top CPU consumers and the "annotate" feature showed that the memory
> zeroing instructions at the beginning of the functions were hot.
When you say you found it with 'perf-top' was that just discovered by
accident, or was this usage of perf-top in response to users reporting
a performance degradation vs earlier QEMU ?
>
> Fixes: 7ff9ff039380 ("meson: mitigate against use of uninitialize stack for
> exploits")
> Cc: Daniel P. Berrangé <berra...@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> include/qemu/compiler.h | 12 ++++++++++++
> hw/virtio/virtio.c | 8 ++++----
> 2 files changed, 16 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
>
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index 496dac5ac1..fabd540b02 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -207,6 +207,18 @@
> # define QEMU_USED
> #endif
>
> +/*
> + * Disable -ftrivial-auto-var-init on a local variable. Use this in rare
> cases
> + * when the compiler zeroes a large on-stack variable and this causes a
> + * performance bottleneck. Only use it when performance data indicates this
> is
> + * necessary since security risks increase with uninitialized stack
> variables.
> + */
> +#if __has_attribute(uninitialized)
> +# define QEMU_UNINITIALIZED __attribute__((uninitialized))
> +#else
> +# define QEMU_UNINITIALIZED
> +#endif
For the benefit of other reviewers, this attribute is specifically
intended for this very purpose:
[quote "info gcc"]
‘uninitialized’
This attribute, attached to a variable with automatic storage,
means that the variable should not be automatically initialized by
the compiler when the option ‘-ftrivial-auto-var-init’ presents.
With the option ‘-ftrivial-auto-var-init’, all the automatic
variables that do not have explicit initializers will be
initialized by the compiler. These additional compiler
initializations might incur run-time overhead, sometimes
dramatically. This attribute can be used to mark some variables to
be excluded from such automatic initialization in order to reduce
runtime overhead.
This attribute has no effect when the option
‘-ftrivial-auto-var-init’ is not present.
[/quote]
> +
> /*
> * http://clang.llvm.org/docs/ThreadSafetyAnalysis.html
> *
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5534251e01..82a285a31d 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1689,8 +1689,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t
> sz)
> VirtIODevice *vdev = vq->vdev;
> VirtQueueElement *elem = NULL;
> unsigned out_num, in_num, elem_entries;
> - hwaddr addr[VIRTQUEUE_MAX_SIZE];
> - struct iovec iov[VIRTQUEUE_MAX_SIZE];
> + hwaddr QEMU_UNINITIALIZED addr[VIRTQUEUE_MAX_SIZE];
> + struct iovec QEMU_UNINITIALIZED iov[VIRTQUEUE_MAX_SIZE];
> VRingDesc desc;
> int rc;
>
> @@ -1836,8 +1836,8 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t
> sz)
> VirtIODevice *vdev = vq->vdev;
> VirtQueueElement *elem = NULL;
> unsigned out_num, in_num, elem_entries;
> - hwaddr addr[VIRTQUEUE_MAX_SIZE];
> - struct iovec iov[VIRTQUEUE_MAX_SIZE];
> + hwaddr QEMU_UNINITIALIZED addr[VIRTQUEUE_MAX_SIZE];
> + struct iovec QEMU_UNINITIALIZED iov[VIRTQUEUE_MAX_SIZE];
> VRingPackedDesc desc;
> uint16_t id;
> int rc;
> --
> 2.49.0
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
