Libguestfs - Jun 2019 - [PATCH libnbd v2] lib: Atomically update h->state when leaving the locked region.

Split h->state into:

 - h->public_state = the state on entry to the locked region
   This is also the atomicly, publicly visible state.

 - h->state = the real current state of the handle

When we leave the locked region we update h->public_state with
h->state, so that from outside the lock the handle appears to move
atomically from its previous state to the final state without going
through any intermediate states.

Some calls to ‘get_state’ become calls to ‘get_next_state’ if the need
the real state.  Others which need to see the publicly visible state
are changed to ‘get_public_state’.

All calls to ‘set_state’ become ‘set_next_state’ because that is the
real state that gets updated.

The purpose of this patch is to make it easier to reason about the
state in lockless code.
---
 generator/generator | 29 ++++++++++++++++-------------
 lib/connect.c       | 10 +++++-----
 lib/disconnect.c    |  8 ++++----
 lib/handle.c        |  2 +-
 lib/internal.h      | 17 ++++++++++++++---
 lib/is-state.c      | 28 ++++++++++++++++------------
 lib/rw.c            |  4 ++--
 7 files changed, 58 insertions(+), 40 deletions(-)

diff --git a/generator/generator b/generator/generator
index 5faa897..246eaed 100755
--- a/generator/generator
+++ b/generator/generator
@@ -812,8 +812,8 @@ type call = {
   (* List of permitted states for making this call.  [[]] = Any state. *)
   permitted_states : permitted_state list;
   (* Most functions must take a lock.  The only known exception is
-   * for a function which {b only} reads from the atomic [h->state]
-   * field and does nothing else with the handle.
+   * for a function which {b only} reads from the atomic
+   * [get_public_state] field and does nothing else with the handle.
    *)
   is_locked : bool;
   (* Most functions can call set_error.  For functions which are
@@ -2418,11 +2418,11 @@ let generate_lib_states_c ()        pr "  enum
state next_state = %s;\n" state_enum;
       pr "\n";
       pr "  r = _enter_%s (h, &next_state, blocked);\n"
state_enum;
-      pr "  if (get_state (h) != next_state) {\n";
+      pr "  if (get_next_state (h) != next_state) {\n";
       pr "    debug (h, \"transition: %%s -> %%s\",\n";
       pr "           \"%s\",\n" display_name;
       pr "           nbd_internal_state_short_string
(next_state));\n";
-      pr "    set_state (h, next_state);\n";
+      pr "    set_next_state (h, next_state);\n";
       pr "  }\n";
       pr "  return r;\n";
       pr "}\n";
@@ -2437,7 +2437,7 @@ let generate_lib_states_c ()    pr "  bool
blocked;\n";
   pr "\n";
   pr "  /* Validate and handle the external event. */\n";
-  pr "  switch (get_state (h))\n";
+  pr "  switch (get_next_state (h))\n";
   pr "  {\n";
   List.iter (
     fun ({ parsed = { display_name; state_enum; events } } as state) ->
@@ -2449,7 +2449,7 @@ let generate_lib_states_c ()            fun (e,
next_state) ->
             pr "    case %s:\n" (c_string_of_external_event e);
             if state != next_state then (
-              pr "      set_state (h, %s);\n"
next_state.parsed.state_enum;
+              pr "      set_next_state (h, %s);\n"
next_state.parsed.state_enum;
               pr "      debug (h, \"event %%s: %%s ->
%%s\",\n";
               pr "             \"%s\", \"%s\",
\"%s\");\n"
                  (string_of_external_event e)
@@ -2465,7 +2465,7 @@ let generate_lib_states_c ()    pr "  }\n";
   pr "\n";
   pr "  set_error (EINVAL, \"external event %%d is invalid in state
%%s\",\n";
-  pr "             ev, nbd_internal_state_short_string (get_state
(h)));\n";
+  pr "             ev, nbd_internal_state_short_string (get_next_state
(h)));\n";
   pr "  return -1;\n";
   pr "\n";
   pr " ok:\n";
@@ -2473,7 +2473,7 @@ let generate_lib_states_c ()    pr "    blocked =
true;\n";
   pr "\n";
   pr "    /* Run a single step. */\n";
-  pr "    switch (get_state (h))\n";
+  pr "    switch (get_next_state (h))\n";
   pr "    {\n";
   List.iter (
     fun { parsed = { state_enum } } ->
@@ -2499,7 +2499,7 @@ let generate_lib_states_c ()    pr "{\n";
   pr "  int r = 0;\n";
   pr "\n";
-  pr "  switch (get_state (h))\n";
+  pr "  switch (get_next_state (h))\n";
   pr "  {\n";
   List.iter (
     fun ({ parsed = { state_enum; events } }) ->
@@ -2545,7 +2545,7 @@ let generate_lib_states_c ()    pr "const char
*\n";
   pr "nbd_unlocked_connection_state (struct nbd_handle *h)\n";
   pr "{\n";
-  pr "  switch (get_state (h))\n";
+  pr "  switch (get_next_state (h))\n";
   pr "  {\n";
   List.iter (
     fun ({ comment; parsed = { display_name; state_enum } }) ->
@@ -2844,7 +2844,7 @@ let generate_lib_api_c ()        pr "  /* We can
check the state outside the handle lock because the\n";
       pr "   * the state is atomic.\n";
       pr "   */\n";
-      pr "  enum state state = get_state (h);\n";
+      pr "  enum state state = get_public_state (h);\n";
       let tests          List.map (
           function
@@ -2869,8 +2869,11 @@ let generate_lib_api_c ()      let argnames =
List.flatten (List.map name_of_arg args) in
     List.iter (pr ", %s") argnames;
     pr ");\n";
-    if is_locked then
-      pr "  pthread_mutex_unlock (&h->lock);\n";
+    if is_locked then (
+      pr "  if (h->public_state != get_next_state (h))\n";
+      pr "    h->public_state = get_next_state (h);\n";
+      pr "  pthread_mutex_unlock (&h->lock);\n"
+    );
     pr "  return ret;\n";
     pr "}\n";
     pr "\n";
diff --git a/lib/connect.c b/lib/connect.c
index b889f80..4e3141f 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -38,16 +38,16 @@
 static int
 error_unless_ready (struct nbd_handle *h)
 {
-  if (nbd_internal_is_state_ready (get_state (h)))
+  if (nbd_internal_is_state_ready (get_next_state (h)))
     return 0;
 
   /* Why did it fail? */
-  if (nbd_internal_is_state_closed (get_state (h))) {
+  if (nbd_internal_is_state_closed (get_next_state (h))) {
     set_error (0, "connection is closed");
     return -1;
   }
 
-  if (nbd_internal_is_state_dead (get_state (h)))
+  if (nbd_internal_is_state_dead (get_next_state (h)))
     /* Don't set the error here, keep the error set when
      * the connection died.
      */
@@ -55,14 +55,14 @@ error_unless_ready (struct nbd_handle *h)
 
   /* Should probably never happen. */
   set_error (0, "connection in an unexpected state (%s)",
-             nbd_internal_state_short_string (get_state (h)));
+             nbd_internal_state_short_string (get_next_state (h)));
   return -1;
 }
 
 static int
 wait_until_connected (struct nbd_handle *h)
 {
-  while (nbd_internal_is_state_connecting (get_state (h))) {
+  while (nbd_internal_is_state_connecting (get_next_state (h))) {
     if (nbd_unlocked_poll (h, -1) == -1)
       return -1;
   }
diff --git a/lib/disconnect.c b/lib/disconnect.c
index 423edaf..95e9a37 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -29,14 +29,14 @@
 int
 nbd_unlocked_shutdown (struct nbd_handle *h)
 {
-  if (nbd_internal_is_state_ready (get_state (h)) ||
-      nbd_internal_is_state_processing (get_state (h))) {
+  if (nbd_internal_is_state_ready (get_next_state (h)) ||
+      nbd_internal_is_state_processing (get_next_state (h))) {
     if (nbd_unlocked_aio_disconnect (h, 0) == -1)
       return -1;
   }
 
-  while (!nbd_internal_is_state_closed (get_state (h)) &&
-         !nbd_internal_is_state_dead (get_state (h))) {
+  while (!nbd_internal_is_state_closed (get_next_state (h)) &&
+         !nbd_internal_is_state_dead (get_next_state (h))) {
     if (nbd_unlocked_poll (h, -1) == -1)
       return -1;
   }
diff --git a/lib/handle.c b/lib/handle.c
index cc311ba..4167b3e 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -57,7 +57,7 @@ nbd_create (void)
   s = getenv ("LIBNBD_DEBUG");
   h->debug = s && strcmp (s, "1") == 0;
 
-  h->state = STATE_START;
+  h->state = h->public_state = STATE_START;
   h->pid = -1;
 
   h->export_name = strdup ("");
diff --git a/lib/internal.h b/lib/internal.h
index 5b6152b..ce4bf5b 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -80,7 +80,17 @@ struct nbd_handle {
   /* Linked list of close callbacks. */
   struct close_callback *close_callbacks;
 
-  _Atomic enum state state;     /* State machine. */
+  /* State machine.
+   *
+   * The actual current state is ‘state’.  ‘public_state’ is updated
+   * before we release the lock.
+   *
+   * Note don't access these fields directly, use the SET_NEXT_STATE
+   * macro in generator/states* code, or the set_next_state,
+   * get_next_state and get_public_state macros in regular code.
+   */
+  _Atomic enum state public_state;
+  enum state state;
 
   bool structured_replies;      /* If we negotiated NBD_OPT_STRUCTURED_REPLY */
 
@@ -291,8 +301,9 @@ extern const char *nbd_internal_state_short_string (enum
state state);
 extern enum state_group nbd_internal_state_group (enum state state);
 extern enum state_group nbd_internal_state_group_parent (enum state_group
group);
 
-#define set_state(h,next_state) ((h)->state) = (next_state)
-#define get_state(h) ((h)->state)
+#define set_next_state(h,next_state) ((h)->state) = (next_state)
+#define get_next_state(h) ((h)->state)
+#define get_public_state(h) ((h)->public_state)
 
 /* utils.c */
 extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp);
diff --git a/lib/is-state.c b/lib/is-state.c
index c941ab4..b2c20df 100644
--- a/lib/is-state.c
+++ b/lib/is-state.c
@@ -98,44 +98,48 @@ nbd_internal_is_state_closed (enum state state)
   return state == STATE_CLOSED;
 }
 
-/* NB: is_locked = false, may_set_error = false. */
+/* The nbd_unlocked_aio_is_* calls are the public APIs
+ * for reading the state of the handle.
+ *
+ * They all have: is_locked = false, may_set_error = false.
+ *
+ * They all read the public state, not the real state.  Therefore you
+ * SHOULD NOT call these functions from elsewhere in the library (use
+ * nbd_internal_is_* instead).
+ */
+
 int
 nbd_unlocked_aio_is_created (struct nbd_handle *h)
 {
-  return nbd_internal_is_state_created (get_state (h));
+  return nbd_internal_is_state_created (get_public_state (h));
 }
 
-/* NB: is_locked = false, may_set_error = false. */
 int
 nbd_unlocked_aio_is_connecting (struct nbd_handle *h)
 {
-  return nbd_internal_is_state_connecting (get_state (h));
+  return nbd_internal_is_state_connecting (get_public_state (h));
 }
 
-/* NB: is_locked = false, may_set_error = false. */
 int
 nbd_unlocked_aio_is_ready (struct nbd_handle *h)
 {
-  return nbd_internal_is_state_ready (get_state (h));
+  return nbd_internal_is_state_ready (get_public_state (h));
 }
 
-/* NB: is_locked = false, may_set_error = false. */
 int
 nbd_unlocked_aio_is_processing (struct nbd_handle *h)
 {
-  return nbd_internal_is_state_processing (get_state (h));
+  return nbd_internal_is_state_processing (get_public_state (h));
 }
 
-/* NB: is_locked = false, may_set_error = false. */
 int
 nbd_unlocked_aio_is_dead (struct nbd_handle *h)
 {
-  return nbd_internal_is_state_dead (get_state (h));
+  return nbd_internal_is_state_dead (get_public_state (h));
 }
 
-/* NB: is_locked = false, may_set_error = false. */
 int
 nbd_unlocked_aio_is_closed (struct nbd_handle *h)
 {
-  return nbd_internal_is_state_closed (get_state (h));
+  return nbd_internal_is_state_closed (get_public_state (h));
 }
diff --git a/lib/rw.c b/lib/rw.c
index b38d95b..ad9c8a0 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -201,7 +201,7 @@ nbd_internal_command_common (struct nbd_handle *h,
    * be handled automatically on a future cycle around to READY.
    */
   if (h->cmds_to_issue != NULL) {
-    assert (nbd_internal_is_state_processing (get_state (h)));
+    assert (nbd_internal_is_state_processing (get_next_state (h)));
     prev_cmd = h->cmds_to_issue;
     while (prev_cmd->next)
       prev_cmd = prev_cmd->next;
@@ -209,7 +209,7 @@ nbd_internal_command_common (struct nbd_handle *h,
   }
   else {
     h->cmds_to_issue = cmd;
-    if (nbd_internal_is_state_ready (get_state (h)) &&
+    if (nbd_internal_is_state_ready (get_next_state (h)) &&
         nbd_internal_run (h, cmd_issue) == -1)
       return -1;
   }
-- 
2.21.0

On 6/5/19 10:21 AM, Richard W.M. Jones wrote:
> Split h->state into:
> 
>  - h->public_state = the state on entry to the locked region
>    This is also the atomicly, publicly visible state.
atomically
> 
>  - h->state = the real current state of the handle
> 
> When we leave the locked region we update h->public_state with
> h->state, so that from outside the lock the handle appears to move
> atomically from its previous state to the final state without going
> through any intermediate states.
> 
> Some calls to ‘get_state’ become calls to ‘get_next_state’ if the need
> the real state.  Others which need to see the publicly visible state
> are changed to ‘get_public_state’.
> 
> All calls to ‘set_state’ become ‘set_next_state’ because that is the
> real state that gets updated.
> 
> The purpose of this patch is to make it easier to reason about the
> state in lockless code.
> ---
> +++ b/generator/generator
> @@ -812,8 +812,8 @@ type call = {
>    (* List of permitted states for making this call.  [[]] = Any state. *)
>    permitted_states : permitted_state list;
>    (* Most functions must take a lock.  The only known exception is
> -   * for a function which {b only} reads from the atomic [h->state]
> -   * field and does nothing else with the handle.
> +   * for a function which {b only} reads from the atomic
> +   * [get_public_state] field and does nothing else with the handle.
Technically, nbd_supports_uri does not take the lock but does not access
state either; this sentence could probably use a slight tweak to reflect
changes that have happened elsewhere since it was first written.
> @@ -2844,7 +2844,7 @@ let generate_lib_api_c () >        pr "  /*
We can check the state outside the handle lock because the\n";
>        pr "   * the state is atomic.\n";
>        pr "   */\n";
> -      pr "  enum state state = get_state (h);\n";
> +      pr "  enum state state = get_public_state (h);\n";
So this is the part I'm still worried about. The state read may be
atomic, but there is a TOCTTOU race where the state we checked before
lock may not match the state when we finally get the lock.  Still,
moving the state check inside the lock means we block until the other
API finishes (other aio_ APIs should finish quickly by their very
design, but sync APIs could take a while to get to that point).  So
maybe we need to do two state checks - one outside the lock (as a sanity
check for quick failures when the state is obviously wrong) and again
inside the lock (to ensure things didn't change behind our backs).

> +++ b/lib/is-state.c
> @@ -98,44 +98,48 @@ nbd_internal_is_state_closed (enum state state)
>    return state == STATE_CLOSED;
>  }
>  
> -/* NB: is_locked = false, may_set_error = false. */
> +/* The nbd_unlocked_aio_is_* calls are the public APIs
> + * for reading the state of the handle.
> + *
> + * They all have: is_locked = false, may_set_error = false.
> + *
> + * They all read the public state, not the real state.  Therefore you
> + * SHOULD NOT call these functions from elsewhere in the library (use
> + * nbd_internal_is_* instead).
> + */
> +
I like the comment consolidation.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org