Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap

[Marc-André Lureau]

Hi

On Tue, Jun 13, 2017 at 3:53 PM Anton Nefedov <address@hidden>
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