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

[Fabiano Rosas <farosas@suse.de>]

Daniel P. Berrangé <berrange@redhat.com> writes:

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

Yeah... I can't tell, I'm new to this code. But I agree that this smells
of a bug somewhere else.

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

I wasn't able to, it triggers in the glib subprocess which is a pain to
debug. I'll give it another try now that there's fixes for the other
bugs.

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