Re: [PATCH 4/4] chardev: Introduce a lock for hup_source

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, May 15, 2025 at 07:20:14PM -0300, Fabiano Rosas wrote:
> It's possible for the hup_source to have its reference decremented by
> remove_hup_source() while it's still being added to the context,
> leading to asserts in glib:

IIUC this must mean that

  tcp_chr_free_connection

is being called concurrently with

  update_ioc_handlers

I'm wondering if that is really intended, or a sign of a deeper
bug that we'll just paper over if we add the mutex proposed here.

Are you able to provide stack traces showing the 2 concurrent
operations that are triggering this problem ?

> 
> g_source_set_callback_indirect: assertion 'g_atomic_int_get
> (&source->ref_count) > 0'
> 
> g_source_attach: assertion 'g_atomic_int_get (&source->ref_count) > 0'
> failed
> 
> Add a lock to serialize removal and creation.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  chardev/char-socket.c  | 4 ++++
>  chardev/char.c         | 2 ++
>  include/chardev/char.h | 1 +
>  3 files changed, 7 insertions(+)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index d16608f1ed..88db9acd0d 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -374,7 +374,9 @@ static void tcp_chr_free_connection(Chardev *chr)
>          s->read_msgfds_num = 0;
>      }
>  
> +    qemu_mutex_lock(&chr->hup_source_lock);
>      remove_hup_source(s);
> +    qemu_mutex_unlock(&chr->hup_source_lock);
>  
>      tcp_set_msgfds(chr, NULL, 0);
>      remove_fd_in_watch(chr);
> @@ -613,6 +615,7 @@ static void update_ioc_handlers(SocketChardev *s)
>                                       tcp_chr_read, chr,
>                                       chr->gcontext);
>  
> +    qemu_mutex_lock(&chr->hup_source_lock);
>      remove_hup_source(s);
>      s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
>      /*
> @@ -634,6 +637,7 @@ static void update_ioc_handlers(SocketChardev *s)
>      g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
>                            chr, NULL);
>      g_source_attach(s->hup_source, chr->gcontext);
> +    qemu_mutex_unlock(&chr->hup_source_lock);
>  }
>  
>  static void tcp_chr_connect(void *opaque)
> diff --git a/chardev/char.c b/chardev/char.c
> index bbebd246c3..d03f698b38 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -279,6 +279,7 @@ static void char_init(Object *obj)
>      chr->handover_yank_instance = false;
>      chr->logfd = -1;
>      qemu_mutex_init(&chr->chr_write_lock);
> +    qemu_mutex_init(&chr->hup_source_lock);
>  
>      /*
>       * Assume if chr_update_read_handler is implemented it will
> @@ -316,6 +317,7 @@ static void char_finalize(Object *obj)
>          close(chr->logfd);
>      }
>      qemu_mutex_destroy(&chr->chr_write_lock);
> +    qemu_mutex_destroy(&chr->hup_source_lock);
>  }
>  
>  static const TypeInfo char_type_info = {
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 429852f8d9..064184153d 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -60,6 +60,7 @@ struct Chardev {
>      Object parent_obj;
>  
>      QemuMutex chr_write_lock;
> +    QemuMutex hup_source_lock;
>      CharBackend *be;
>      char *label;
>      char *filename;
> -- 
> 2.35.3
> 

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 :|