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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190605/bc09f141/attachment.sig>