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

[Anton Nefedov]

On 06/13/2017 03:32 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Jun 13, 2017 at 3:53 PM Anton Nefedov 
> <address@hidden <mailto: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.

We also need to protect be->chr pointer, not just the Chardev itself, so
probably ref/unref won't be thread-safe, strictly speaking?

e.g.

  fe_write(CharBackend *be) {
    ref(be->chr);
    ...
    unref(be->chr); // oops, be->chr might have changed
  }

or

  fe_write(CharBackend *be) {
    Chardev *chr = be->chr;
    ref(chr); // not good either, chr might be destroyed already
    ...
    unref(chr);
  }


> --
> Marc-André Lureau
/Anton