[v3,03/13] char: chardevice hotswap

This patch adds a possibility to change a char device without a frontend
removal.

1. Ideally, it would have to happen transparently to a frontend, i.e.
frontend would continue its regular operation.
However, backends are not stateless and are set up by the frontends
via qemu_chr_fe_<> functions, and it's not (generally) possible to replay
that setup entirely in a backend code, as different chardevs respond
to the setup calls differently, so do frontends work differently basing
on those setup responses.
Moreover, some frontend can generally get and save the backend pointer
(qemu_chr_fe_get_driver()), and it will become invalid after backend change.

So, a frontend which would like to support chardev hotswap has to register
a "backend change" handler, and redo its backend setup there.

2. Write path can be used by multiple threads and thus protected with
chr_write_lock.
So hotswap also has to be protected so write functions won't access
a backend being replaced.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 chardev/char.c        | 126 ++++++++++++++++++++++++++++++++++++++++++++++----
 include/sysemu/char.h |   9 ++++
 qapi-schema.json      |  40 ++++++++++++++++
 3 files changed, 165 insertions(+), 10 deletions(-)

Hi

On Tue, May 30, 2017 at 6:12 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:

> This patch adds a possibility to change a char device without a frontend
> removal.
>
> 1. Ideally, it would have to happen transparently to a frontend, i.e.
> frontend would continue its regular operation.
> However, backends are not stateless and are set up by the frontends
> via qemu_chr_fe_<> functions, and it's not (generally) possible to replay
> that setup entirely in a backend code, as different chardevs respond
> to the setup calls differently, so do frontends work differently basing
> on those setup responses.
> Moreover, some frontend can generally get and save the backend pointer
> (qemu_chr_fe_get_driver()), and it will become invalid after backend
> change.
>
> So, a frontend which would like to support chardev hotswap has to register
> a "backend change" handler, and redo its backend setup there.
>
> 2. Write path can be used by multiple threads and thus protected with
> chr_write_lock.
> So hotswap also has to be protected so write functions won't access
> a backend being replaced.
>
>
Tbh, I don't understand the need for a different lock. Could you explain?
Even better would be to write a test that shows in which way the lock helps.



Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>

patch looks good otherwise


> ---
>  chardev/char.c        | 126
> ++++++++++++++++++++++++++++++++++++++++++++++----
>  include/sysemu/char.h |   9 ++++
>  qapi-schema.json      |  40 ++++++++++++++++
>  3 files changed, 165 insertions(+), 10 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 1b097b3..ba1a5f5 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -132,12 +132,16 @@ static bool qemu_chr_replay(Chardev *chr)
>
>  int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
>  {
> -    Chardev *s = be->chr;
> +    Chardev *s;
>      ChardevClass *cc;
>      int ret;
>
> +    qemu_mutex_lock(&be->chr_lock);
> +    s = be->chr;
> +
>      if (!s) {
> -        return 0;
> +        ret = 0;
> +        goto end;
>      }
>
>      if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_PLAY) {
> @@ -145,7 +149,7 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t
> *buf, int len)
>          replay_char_write_event_load(&ret, &offset);
>          assert(offset <= len);
>          qemu_chr_fe_write_buffer(s, buf, offset, &offset);
> -        return ret;
> +        goto end;
>      }
>
>      cc = CHARDEV_GET_CLASS(s);
> @@ -161,7 +165,9 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t
> *buf, int len)
>      if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_RECORD) {
>          replay_char_write_event_save(ret, ret < 0 ? 0 : ret);
>      }
> -
> +
> +end:
> +    qemu_mutex_unlock(&be->chr_lock);
>      return ret;
>  }
>
> @@ -191,13 +197,16 @@ int qemu_chr_write_all(Chardev *s, const uint8_t
> *buf, int len)
>
>  int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len)
>  {
> -    Chardev *s = be->chr;
> +    Chardev *s;
> +    int ret;
>
> -    if (!s) {
> -        return 0;
> -    }
> +    qemu_mutex_lock(&be->chr_lock);
> +
> +    s = be->chr;
> +    ret = s ? qemu_chr_write_all(s, buf, len) : 0;
>
> -    return qemu_chr_write_all(s, buf, len);
> +    qemu_mutex_unlock(&be->chr_lock);
> +    return ret;
>  }
>
>  int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
> @@ -478,7 +487,7 @@ Chardev *qemu_chr_fe_get_driver(CharBackend *be)
>      return be->chr;
>  }
>
> -bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
> +static bool qemu_chr_fe_connect(CharBackend *b, Chardev *s, Error **errp)
>  {
>      int tag = 0;
>
> @@ -507,6 +516,16 @@ unavailable:
>      return false;
>  }
>
> +bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
> +{
> +    if (!qemu_chr_fe_connect(b, s, errp)) {
> +        return false;
> +    }
> +
> +    qemu_mutex_init(&b->chr_lock);
> +    return true;
> +}
> +
>  static bool qemu_chr_is_busy(Chardev *s)
>  {
>      if (CHARDEV_IS_MUX(s)) {
> @@ -531,6 +550,7 @@ void qemu_chr_fe_deinit(CharBackend *b)
>              d->backends[b->tag] = NULL;
>          }
>          b->chr = NULL;
> +        qemu_mutex_destroy(&b->chr_lock);
>      }
>  }
>
> @@ -1306,6 +1326,92 @@ ChardevReturn *qmp_chardev_add(const char *id,
> ChardevBackend *backend,
>      return ret;
>  }
>
> +ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> +                                  Error **errp)
> +{
> +    CharBackend *be;
> +    const ChardevClass *cc;
> +    Chardev *chr, *chr_new;
> +    bool closed_sent = false;
> +    ChardevReturn *ret;
> +
> +    chr = qemu_chr_find(id);
> +    if (!chr) {
> +        error_setg(errp, "Chardev '%s' does not exist", id);
> +        return NULL;
> +    }
> +
> +    if (CHARDEV_IS_MUX(chr)) {
> +        error_setg(errp, "Mux device hotswap not supported yet");
> +        return NULL;
> +    }
> +
> +    if (qemu_chr_replay(chr)) {
> +        error_setg(errp,
> +            "Chardev '%s' cannot be changed in record/replay mode", id);
> +        return NULL;
> +    }
> +
> +    be = chr->be;
> +    if (!be) {
> +        /* easy case */
> +        object_unparent(OBJECT(chr));
> +        return qmp_chardev_add(id, backend, errp);
> +    }
> +
> +    if (!be->chr_be_change) {
> +        error_setg(errp, "Chardev user does not support chardev hotswap");
> +        return NULL;
> +    }
> +
> +    cc = char_get_class(ChardevBackendKind_lookup[backend->type], errp);
> +    if (!cc) {
> +        return NULL;
> +    }
> +
> +    chr_new = qemu_chardev_new(NULL,
> object_class_get_name(OBJECT_CLASS(cc)),
> +                               backend, errp);
> +    if (!chr_new) {
> +        return NULL;
> +    }
> +    chr_new->label = g_strdup(id);
> +
> +    if (chr->be_open && !chr_new->be_open) {
> +        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +        closed_sent = true;
> +    }
> +
> +    qemu_mutex_lock(&be->chr_lock);
> +    chr->be = NULL;
> +    qemu_chr_fe_connect(be, chr_new, &error_abort);
> +
> +    if (be->chr_be_change(be->opaque) < 0) {
> +        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);
> +        if (closed_sent) {
> +            qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> +        }
> +        object_unref(OBJECT(chr_new));
> +        return NULL;
> +    }
> +    qemu_mutex_unlock(&be->chr_lock);
> +
> +    object_unparent(OBJECT(chr));
> +    object_property_add_child(get_chardevs_root(), chr_new->label,
> +                              OBJECT(chr_new), &error_abort);
> +    object_unref(OBJECT(chr_new));
> +
> +    ret = g_new0(ChardevReturn, 1);
> +    if (CHARDEV_IS_PTY(chr_new)) {
> +        ret->pty = g_strdup(chr_new->filename + 4);
> +        ret->has_pty = true;
> +    }
> +
> +    return ret;
> +}
> +
>  void qmp_chardev_remove(const char *id, Error **errp)
>  {
>      Chardev *chr;
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 9f8df07..014ebbc 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -92,6 +92,7 @@ typedef struct CharBackend {
>      void *opaque;
>      int tag;
>      int fe_open;
> +    QemuMutex chr_lock;
>  } CharBackend;
>
>  struct Chardev {
> @@ -141,6 +142,14 @@ void qemu_chr_parse_common(QemuOpts *opts,
> ChardevCommon *backend);
>   */
>  Chardev *qemu_chr_new(const char *label, const char *filename);
>
> +/**
> + * @qemu_chr_change:
> + *
> + * Change an existing character backend
> + *
> + * @opts the new backend options
> + */
> +void qemu_chr_change(QemuOpts *opts, Error **errp);
>
>  /**
>   * @qemu_chr_fe_disconnect:
> diff --git a/qapi-schema.json b/qapi-schema.json
> index e38c5f0..0f0df36 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -5097,6 +5097,46 @@
>    'returns': 'ChardevReturn' }
>
>  ##
> +# @chardev-change:
> +#
> +# Change a character device backend
> +#
> +# @id: the chardev's ID, must exist
> +# @backend: new backend type and parameters
> +#
> +# Returns: ChardevReturn.
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "execute" : "chardev-change",
> +#      "arguments" : { "id" : "baz",
> +#                      "backend" : { "type" : "pty", "data" : {} } } }
> +# <- { "return": { "pty" : "/dev/pty/42" } }
> +#
> +# -> {"execute" : "chardev-change",
> +#     "arguments" : {
> +#         "id" : "charchannel2",
> +#         "backend" : {
> +#             "type" : "socket",
> +#             "data" : {
> +#                 "addr" : {
> +#                     "type" : "unix" ,
> +#                     "data" : {
> +#                         "path" : "/tmp/charchannel2.socket"
> +#                     }
> +#                  },
> +#                  "server" : true,
> +#                  "wait" : false }}}}
> +# <- {"return": {}}
> +#
> +##
> +{ 'command': 'chardev-change', 'data': {'id'      : 'str',
> +                                        'backend' : 'ChardevBackend' },
> +  'returns': 'ChardevReturn' }
> +
> +##
>  # @chardev-remove:
>  #
>  # Remove a character device backend
> --
> 2.7.4
>
>
> --
Marc-André Lureau

On 05/31/2017 10:20 PM, Marc-André Lureau wrote:
> Hi
> 
> On Tue, May 30, 2017 at 6:12 PM Anton Nefedov 
> <anton.nefedov@virtuozzo.com <mailto:anton.nefedov@virtuozzo.com>> wrote:
> 
>     This patch adds a possibility to change a char device without a frontend
>     removal.
> 
>     1. Ideally, it would have to happen transparently to a frontend, i.e.
>     frontend would continue its regular operation.
>     However, backends are not stateless and are set up by the frontends
>     via qemu_chr_fe_<> functions, and it's not (generally) possible to
>     replay
>     that setup entirely in a backend code, as different chardevs respond
>     to the setup calls differently, so do frontends work differently basing
>     on those setup responses.
>     Moreover, some frontend can generally get and save the backend pointer
>     (qemu_chr_fe_get_driver()), and it will become invalid after backend
>     change.
> 
>     So, a frontend which would like to support chardev hotswap has to
>     register
>     a "backend change" handler, and redo its backend setup there.
> 
>     2. Write path can be used by multiple threads and thus protected with
>     chr_write_lock.
>     So hotswap also has to be protected so write functions won't access
>     a backend being replaced.
> 
> 
> Tbh, I don't understand the need for a different lock. Could you 
> explain? Even better would be to write a test that shows in which way 
> the lock helps.
> 

hi Marc-André,

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?

I can try to add a test but can't quite see yet how to freeze the old
chardev somewhere in cc->chr_write() and hotswap it while it's there.


> 
> 
>     Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com
>     <mailto:anton.nefedov@virtuozzo.com>>
>     Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com
>     <mailto:vsementsov@virtuozzo.com>>
> 
> 
> patch looks good otherwise
> 
>     ---
>       chardev/char.c        | 126
>     ++++++++++++++++++++++++++++++++++++++++++++++----
>       include/sysemu/char.h |   9 ++++
>       qapi-schema.json      |  40 ++++++++++++++++
>       3 files changed, 165 insertions(+), 10 deletions(-)
> 
> -- 
> Marc-André Lureau

/Anton

On Thu, Jun 1, 2017 at 3:23 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:

>
>
> On 05/31/2017 10:20 PM, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, May 30, 2017 at 6:12 PM Anton Nefedov
> > <anton.nefedov@virtuozzo.com <mailto:anton.nefedov@virtuozzo.com>>
> wrote:
> >
> >     This patch adds a possibility to change a char device without a
> frontend
> >     removal.
> >
> >     1. Ideally, it would have to happen transparently to a frontend, i.e.
> >     frontend would continue its regular operation.
> >     However, backends are not stateless and are set up by the frontends
> >     via qemu_chr_fe_<> functions, and it's not (generally) possible to
> >     replay
> >     that setup entirely in a backend code, as different chardevs respond
> >     to the setup calls differently, so do frontends work differently
> basing
> >     on those setup responses.
> >     Moreover, some frontend can generally get and save the backend
> pointer
> >     (qemu_chr_fe_get_driver()), and it will become invalid after backend
> >     change.
> >
> >     So, a frontend which would like to support chardev hotswap has to
> >     register
> >     a "backend change" handler, and redo its backend setup there.
> >
> >     2. Write path can be used by multiple threads and thus protected with
> >     chr_write_lock.
> >     So hotswap also has to be protected so write functions won't access
> >     a backend being replaced.
> >
> >
> > Tbh, I don't understand the need for a different lock. Could you
> > explain? Even better would be to write a test that shows in which way
> > the lock helps.
> >
>
> hi Marc-André,
>
> 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' :)


> I can try to add a test but can't quite see yet how to freeze the old
> chardev somewhere in cc->chr_write() and hotswap it while it's there.
>
>
> >
> >
> >     Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com
> >     <mailto:anton.nefedov@virtuozzo.com>>
> >     Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com
> >     <mailto:vsementsov@virtuozzo.com>>
> >
> >
> > patch looks good otherwise
> >
> >     ---
> >       chardev/char.c        | 126
> >     ++++++++++++++++++++++++++++++++++++++++++++++----
> >       include/sysemu/char.h |   9 ++++
> >       qapi-schema.json      |  40 ++++++++++++++++
> >       3 files changed, 165 insertions(+), 10 deletions(-)
> >
> > --
> > Marc-André Lureau
>
> /Anton
>

On 06/09/2017 05:53 PM, Marc-André Lureau wrote:
> 
> 
> On Thu, Jun 1, 2017 at 3:23 PM Anton Nefedov 
> <anton.nefedov@virtuozzo.com <mailto:anton.nefedov@virtuozzo.com>> wrote:
> 
> 
> 
>     On 05/31/2017 10:20 PM, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Tue, May 30, 2017 at 6:12 PM Anton Nefedov
>      > <anton.nefedov@virtuozzo.com <mailto:anton.nefedov@virtuozzo.com>
>     <mailto:anton.nefedov@virtuozzo.com
>     <mailto:anton.nefedov@virtuozzo.com>>> wrote:
>      >
>      >     This patch adds a possibility to change a char device without
>     a frontend
>      >     removal.
>      >
>      >     1. Ideally, it would have to happen transparently to a
>     frontend, i.e.
>      >     frontend would continue its regular operation.
>      >     However, backends are not stateless and are set up by the
>     frontends
>      >     via qemu_chr_fe_<> functions, and it's not (generally)
>     possible to
>      >     replay
>      >     that setup entirely in a backend code, as different chardevs
>     respond
>      >     to the setup calls differently, so do frontends work
>     differently basing
>      >     on those setup responses.
>      >     Moreover, some frontend can generally get and save the
>     backend pointer
>      >     (qemu_chr_fe_get_driver()), and it will become invalid after
>     backend
>      >     change.
>      >
>      >     So, a frontend which would like to support chardev hotswap has to
>      >     register
>      >     a "backend change" handler, and redo its backend setup there.
>      >
>      >     2. Write path can be used by multiple threads and thus
>     protected with
>      >     chr_write_lock.
>      >     So hotswap also has to be protected so write functions won't
>     access
>      >     a backend being replaced.
>      >
>      >
>      > Tbh, I don't understand the need for a different lock. Could you
>      > explain? Even better would be to write a test that shows in which way
>      > the lock helps.
>      >
> 
>     hi Marc-André,
> 
>     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?


>     I can try to add a test but can't quite see yet how to freeze the old
>     chardev somewhere in cc->chr_write() and hotswap it while it's there.
> 
> 
>      >
>      >
>      >     Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com
>     <mailto:anton.nefedov@virtuozzo.com>
>      >     <mailto:anton.nefedov@virtuozzo.com
>     <mailto:anton.nefedov@virtuozzo.com>>>
>      >     Reviewed-by: Vladimir Sementsov-Ogievskiy
>     <vsementsov@virtuozzo.com <mailto:vsementsov@virtuozzo.com>
>      >     <mailto:vsementsov@virtuozzo.com
>     <mailto:vsementsov@virtuozzo.com>>>
>      >
>      >
>      > patch looks good otherwise
>      >
>      >     ---
>      >       chardev/char.c        | 126
>      >     ++++++++++++++++++++++++++++++++++++++++++++++----
>      >       include/sysemu/char.h |   9 ++++
>      >       qapi-schema.json      |  40 ++++++++++++++++
>      >       3 files changed, 165 insertions(+), 10 deletions(-)
>      >
>      > --
>      > Marc-André Lureau
> 
>     /Anton
> 
> -- 
> Marc-André Lureau

/Anton

Hi

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

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

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