Re: [PATCH] Use "void *" as parameter for functions that are used for aio_set_event_notifier()
On Wed, May 29, 2024 at 07:49:48PM +0200, Thomas Huth wrote: > aio_set_event_notifier() and aio_set_event_notifier_poll() in > util/aio-posix.c and util/aio-win32.c are casting function pointers of > functions that take an "EventNotifier *" pointer as parameter to function > pointers that take a "void *" pointer as parameter (i.e. the IOHandler > type). When those function pointers are later used to call the referenced > function, this triggers undefined behavior errors with the latest version > of Clang in Fedora 40 when compiling with the option "-fsanitize=undefined". > And this also prevents enabling the strict mode of CFI which is currently > disabled with -fsanitize-cfi-icall-generalize-pointers. Thus let us avoid > the problem by using "void *" as parameter in all spots where it is needed. > > Signed-off-by: Thomas Huth Acked-by: Michael S. Tsirkin > --- > Yes, I know, the patch looks ugly ... but I don't see a better way to > tackle this. If someone has a better idea, suggestions are welcome! > > include/block/aio.h| 8 > include/hw/virtio/virtio.h | 2 +- > include/qemu/main-loop.h | 3 +-- > block/linux-aio.c | 6 +++--- > block/nvme.c | 8 > block/win32-aio.c | 4 ++-- > hw/hyperv/hyperv.c | 6 +++--- > hw/hyperv/hyperv_testdev.c | 5 +++-- > hw/hyperv/vmbus.c | 8 > hw/nvme/ctrl.c | 8 > hw/usb/ccid-card-emulated.c| 5 +++-- > hw/virtio/vhost-shadow-virtqueue.c | 11 ++- > hw/virtio/vhost.c | 5 +++-- > hw/virtio/virtio.c | 26 ++ > tests/unit/test-aio.c | 9 + > tests/unit/test-nested-aio-poll.c | 8 > util/aio-posix.c | 14 ++ > util/aio-win32.c | 10 +- > util/async.c | 6 +++--- > util/main-loop.c | 3 +-- > 20 files changed, 79 insertions(+), 76 deletions(-) > > diff --git a/include/block/aio.h b/include/block/aio.h > index 8378553eb9..01e7ea069d 100644 > --- a/include/block/aio.h > +++ b/include/block/aio.h > @@ -476,9 +476,9 @@ void aio_set_fd_handler(AioContext *ctx, > */ > void aio_set_event_notifier(AioContext *ctx, > EventNotifier *notifier, > -EventNotifierHandler *io_read, > +IOHandler *io_read, > AioPollFn *io_poll, > -EventNotifierHandler *io_poll_ready); > +IOHandler *io_poll_ready); > > /* > * Set polling begin/end callbacks for an event notifier that has already > been > @@ -491,8 +491,8 @@ void aio_set_event_notifier(AioContext *ctx, > */ > void aio_set_event_notifier_poll(AioContext *ctx, > EventNotifier *notifier, > - EventNotifierHandler *io_poll_begin, > - EventNotifierHandler *io_poll_end); > + IOHandler *io_poll_begin, > + IOHandler *io_poll_end); > > /* Return a GSource that lets the main loop poll the file descriptors > attached > * to this AioContext. > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 7d5ffdc145..e98cecfdd7 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -396,7 +396,7 @@ void virtio_device_release_ioeventfd(VirtIODevice *vdev); > bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev); > EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq); > void virtio_queue_set_host_notifier_enabled(VirtQueue *vq, bool enabled); > -void virtio_queue_host_notifier_read(EventNotifier *n); > +void virtio_queue_host_notifier_read(void *n); > void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx); > void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext > *ctx); > void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx); > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h > index 5764db157c..ba73a0c6da 100644 > --- a/include/qemu/main-loop.h > +++ b/include/qemu/main-loop.h > @@ -241,8 +241,7 @@ void qemu_set_fd_handler(int fd, > * @handler: A level-triggered callback that is fired when @e > * has been set. @e is passed to it as a parameter. > */ > -void event_notifier_set_handler(EventNotifier *e, > -EventNotifierHandler *handler); > +void event_notifier_set_handler(EventNotifier *e, IOHandler *handler); > > GSource *iohandler_get_g_source(void); > AioContext *iohandler_get_aio_context(void); > diff --git a/block/linux-aio.c b/block/linux-aio.c > index ec05d946f3..61f796f7e0 100644 > --- a/block/linux-aio.c > +++
Re: [PATCH] Use "void *" as parameter for functions that are used for aio_set_event_notifier()
On 29/05/2024 20.22, Stefan Hajnoczi wrote:
On Wed, May 29, 2024 at 07:49:48PM +0200, Thomas Huth wrote:
aio_set_event_notifier() and aio_set_event_notifier_poll() in
util/aio-posix.c and util/aio-win32.c are casting function pointers of
functions that take an "EventNotifier *" pointer as parameter to function
pointers that take a "void *" pointer as parameter (i.e. the IOHandler
type). When those function pointers are later used to call the referenced
function, this triggers undefined behavior errors with the latest version
of Clang in Fedora 40 when compiling with the option "-fsanitize=undefined".
And this also prevents enabling the strict mode of CFI which is currently
disabled with -fsanitize-cfi-icall-generalize-pointers. Thus let us avoid
the problem by using "void *" as parameter in all spots where it is needed.
Signed-off-by: Thomas Huth
---
Yes, I know, the patch looks ugly ... but I don't see a better way to
tackle this. If someone has a better idea, suggestions are welcome!
An alternative is adding EventNotifierHandler *io_read, *io_poll_ready,
*io_poll_begin, and *io_poll_end fields to EventNotifier so that
aio_set_event_notifier() and aio_set_event_notifier_poll() can pass
helper functions to the underlying aio_set_fd_handler() and
aio_set_fd_poll() APIs. These helper functions then invoke the
EventNotifier callbacks:
/* Helpers */
static void event_notifier_io_read(void *opaque)
{
EventNotifier *notifier = opaque;
notifier->io_read(notifier);
}
That's a nice idea, thanks, I'll give it a try!
Thomas
Re: [PATCH] Use "void *" as parameter for functions that are used for aio_set_event_notifier()
On Wed, May 29, 2024 at 07:49:48PM +0200, Thomas Huth wrote:
> aio_set_event_notifier() and aio_set_event_notifier_poll() in
> util/aio-posix.c and util/aio-win32.c are casting function pointers of
> functions that take an "EventNotifier *" pointer as parameter to function
> pointers that take a "void *" pointer as parameter (i.e. the IOHandler
> type). When those function pointers are later used to call the referenced
> function, this triggers undefined behavior errors with the latest version
> of Clang in Fedora 40 when compiling with the option "-fsanitize=undefined".
> And this also prevents enabling the strict mode of CFI which is currently
> disabled with -fsanitize-cfi-icall-generalize-pointers. Thus let us avoid
> the problem by using "void *" as parameter in all spots where it is needed.
>
> Signed-off-by: Thomas Huth
> ---
> Yes, I know, the patch looks ugly ... but I don't see a better way to
> tackle this. If someone has a better idea, suggestions are welcome!
An alternative is adding EventNotifierHandler *io_read, *io_poll_ready,
*io_poll_begin, and *io_poll_end fields to EventNotifier so that
aio_set_event_notifier() and aio_set_event_notifier_poll() can pass
helper functions to the underlying aio_set_fd_handler() and
aio_set_fd_poll() APIs. These helper functions then invoke the
EventNotifier callbacks:
/* Helpers */
static void event_notifier_io_read(void *opaque)
{
EventNotifier *notifier = opaque;
notifier->io_read(notifier);
}
static void event_notifier_io_poll_ready(void *opaque)
{
EventNotifier *notifier = opaque;
notifier->io_poll_ready(notifier);
}
void aio_set_event_notifier(AioContext *ctx,
EventNotifier *notifier,
EventNotifierHandler *io_read,
AioPollFn *io_poll,
EventNotifierHandler *io_poll_ready)
{
notifier->io_read = io_read;
notifier->io_poll_ready = io_poll_ready;
aio_set_fd_handler(ctx, event_notifier_get_fd(notifier),
io_read ? event_notifier_io_read : NULL,
NULL, io_poll,
io_poll_ready ? event_notifier_io_poll_ready : NULL,
notifier);
}
...same for aio_set_event_notifier_poll()...
This is not beautiful either but keeps the API type safe and simpler for
users.
>
> include/block/aio.h| 8
> include/hw/virtio/virtio.h | 2 +-
> include/qemu/main-loop.h | 3 +--
> block/linux-aio.c | 6 +++---
> block/nvme.c | 8
> block/win32-aio.c | 4 ++--
> hw/hyperv/hyperv.c | 6 +++---
> hw/hyperv/hyperv_testdev.c | 5 +++--
> hw/hyperv/vmbus.c | 8
> hw/nvme/ctrl.c | 8
> hw/usb/ccid-card-emulated.c| 5 +++--
> hw/virtio/vhost-shadow-virtqueue.c | 11 ++-
> hw/virtio/vhost.c | 5 +++--
> hw/virtio/virtio.c | 26 ++
> tests/unit/test-aio.c | 9 +
> tests/unit/test-nested-aio-poll.c | 8
> util/aio-posix.c | 14 ++
> util/aio-win32.c | 10 +-
> util/async.c | 6 +++---
> util/main-loop.c | 3 +--
> 20 files changed, 79 insertions(+), 76 deletions(-)
>
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 8378553eb9..01e7ea069d 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -476,9 +476,9 @@ void aio_set_fd_handler(AioContext *ctx,
> */
> void aio_set_event_notifier(AioContext *ctx,
> EventNotifier *notifier,
> -EventNotifierHandler *io_read,
> +IOHandler *io_read,
> AioPollFn *io_poll,
> -EventNotifierHandler *io_poll_ready);
> +IOHandler *io_poll_ready);
>
> /*
> * Set polling begin/end callbacks for an event notifier that has already
> been
> @@ -491,8 +491,8 @@ void aio_set_event_notifier(AioContext *ctx,
> */
> void aio_set_event_notifier_poll(AioContext *ctx,
> EventNotifier *notifier,
> - EventNotifierHandler *io_poll_begin,
> - EventNotifierHandler *io_poll_end);
> + IOHandler *io_poll_begin,
> + IOHandler *io_poll_end);
>
> /* Return a GSource that lets the main loop poll the file descriptors
> attached
> * to this AioContext.
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 7d5ffdc145..e98cecfdd7 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -396,7 +396,7 @@ void
[PATCH] Use "void *" as parameter for functions that are used for aio_set_event_notifier()
aio_set_event_notifier() and aio_set_event_notifier_poll() in
util/aio-posix.c and util/aio-win32.c are casting function pointers of
functions that take an "EventNotifier *" pointer as parameter to function
pointers that take a "void *" pointer as parameter (i.e. the IOHandler
type). When those function pointers are later used to call the referenced
function, this triggers undefined behavior errors with the latest version
of Clang in Fedora 40 when compiling with the option "-fsanitize=undefined".
And this also prevents enabling the strict mode of CFI which is currently
disabled with -fsanitize-cfi-icall-generalize-pointers. Thus let us avoid
the problem by using "void *" as parameter in all spots where it is needed.
Signed-off-by: Thomas Huth
---
Yes, I know, the patch looks ugly ... but I don't see a better way to
tackle this. If someone has a better idea, suggestions are welcome!
include/block/aio.h| 8
include/hw/virtio/virtio.h | 2 +-
include/qemu/main-loop.h | 3 +--
block/linux-aio.c | 6 +++---
block/nvme.c | 8
block/win32-aio.c | 4 ++--
hw/hyperv/hyperv.c | 6 +++---
hw/hyperv/hyperv_testdev.c | 5 +++--
hw/hyperv/vmbus.c | 8
hw/nvme/ctrl.c | 8
hw/usb/ccid-card-emulated.c| 5 +++--
hw/virtio/vhost-shadow-virtqueue.c | 11 ++-
hw/virtio/vhost.c | 5 +++--
hw/virtio/virtio.c | 26 ++
tests/unit/test-aio.c | 9 +
tests/unit/test-nested-aio-poll.c | 8
util/aio-posix.c | 14 ++
util/aio-win32.c | 10 +-
util/async.c | 6 +++---
util/main-loop.c | 3 +--
20 files changed, 79 insertions(+), 76 deletions(-)
diff --git a/include/block/aio.h b/include/block/aio.h
index 8378553eb9..01e7ea069d 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -476,9 +476,9 @@ void aio_set_fd_handler(AioContext *ctx,
*/
void aio_set_event_notifier(AioContext *ctx,
EventNotifier *notifier,
-EventNotifierHandler *io_read,
+IOHandler *io_read,
AioPollFn *io_poll,
-EventNotifierHandler *io_poll_ready);
+IOHandler *io_poll_ready);
/*
* Set polling begin/end callbacks for an event notifier that has already been
@@ -491,8 +491,8 @@ void aio_set_event_notifier(AioContext *ctx,
*/
void aio_set_event_notifier_poll(AioContext *ctx,
EventNotifier *notifier,
- EventNotifierHandler *io_poll_begin,
- EventNotifierHandler *io_poll_end);
+ IOHandler *io_poll_begin,
+ IOHandler *io_poll_end);
/* Return a GSource that lets the main loop poll the file descriptors attached
* to this AioContext.
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 7d5ffdc145..e98cecfdd7 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -396,7 +396,7 @@ void virtio_device_release_ioeventfd(VirtIODevice *vdev);
bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
void virtio_queue_set_host_notifier_enabled(VirtQueue *vq, bool enabled);
-void virtio_queue_host_notifier_read(EventNotifier *n);
+void virtio_queue_host_notifier_read(void *n);
void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx);
void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext
*ctx);
void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx);
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 5764db157c..ba73a0c6da 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -241,8 +241,7 @@ void qemu_set_fd_handler(int fd,
* @handler: A level-triggered callback that is fired when @e
* has been set. @e is passed to it as a parameter.
*/
-void event_notifier_set_handler(EventNotifier *e,
-EventNotifierHandler *handler);
+void event_notifier_set_handler(EventNotifier *e, IOHandler *handler);
GSource *iohandler_get_g_source(void);
AioContext *iohandler_get_aio_context(void);
diff --git a/block/linux-aio.c b/block/linux-aio.c
index ec05d946f3..61f796f7e0 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -253,9 +253,9 @@ static void qemu_laio_completion_bh(void *opaque)
qemu_laio_process_completions_and_submit(s);
}
-static void qemu_laio_completion_cb(EventNotifier *e)
+static void qemu_laio_completion_cb(void *e)
{
-LinuxAioState *s = container_of(e, LinuxAioState,
