Hi
On Tue, Jun 13, 2017 at 3:53 PM Anton Nefedov <anton.nefe...@virtuozzo.com>
wrote:
> > The existing chr_write_lock belongs to Chardev.
> > For the hotswap case, we need to ensure that be->chr won't change and
> > the old Chardev (together with its mutex) won't be destroyed while
> it's
> > used in the write functions.
> >
> > Maybe we could move the lock to CharBackend, instead of creating a
> new
> > one. But I guess this
> > 1. won't work for mux, where multiple CharBackends share the same
> > Chardev
> > 2. won't work for some chardevs, like pty which uses the lock for
> the
> > timer handler
> >
> > Sorry if I'm not explaining clearly enough or maybe I'm missing some
> > easier solution?
> >
> >
> > It looks to me like you would get the same guarantees by using the
> > chr_write_lock directly:
> >
> > @@ -1381,7 +1374,7 @@ ChardevReturn *qmp_chardev_change(const char *id,
> > ChardevBackend *backend,
> > closed_sent = true;
> > }
> >
> > - qemu_mutex_lock(&be->chr_lock);
> > + qemu_mutex_lock(&chr->chr_write_lock);
> > chr->be = NULL;
> > qemu_chr_fe_connect(be, chr_new, &error_abort);
> >
> > @@ -1389,14 +1382,14 @@ ChardevReturn *qmp_chardev_change(const char
> > *id, ChardevBackend *backend,
> > error_setg(errp, "Chardev '%s' change failed", chr_new->label);
> > chr_new->be = NULL;
> > qemu_chr_fe_connect(be, chr, &error_abort);
> > - qemu_mutex_unlock(&be->chr_lock);
> > + qemu_mutex_unlock(&chr->chr_write_lock);
> > if (closed_sent) {
> > qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> > }
> > object_unref(OBJECT(chr_new));
> > return NULL;
> > }
> > - qemu_mutex_unlock(&be->chr_lock);
> > + qemu_mutex_unlock(&chr->chr_write_lock);
> >
> > I wonder if we should rename 'chr_write_lock' to just 'lock' :)
> >
>
> hi,
>
> but isn't there a potential race?
>
> Assume thread-1 is somewhere in qemu_chr_write() and thread-2 is in
> qmp_chardev_change().
>
> T2 can change the chardev, and destroy the old one while T1 still has
> the pointer ( = use after free).
> Or T1 unlocks chr_write_lock, T2 acquires that in
> qemu_chr_write_buffer(), then T1 destroys it together
> with the chardev ( = undefined behaviour).
>
> What I'm trying to say is that the critical section for the hotswap is
> bigger than what chr_write_lock currently covers - we also need to cover
> the be->chr pointer.
> Or am I missing something?
>
Thanks for the detail, I think you are correct, and probably your solution
is good. Another way would be to object_ref/unref the Chardev when using it
in seperate thread, this way we wouldn't need an extra lock, I think. Paolo
might be of good advice to get this right.
--
Marc-André Lureau
