Libguestfs - Jul 2019 - Re: [PATCH libnbd v3 1/2] lib: Implement closure lifetimes.

On 7/25/19 8:07 AM, Richard W.M. Jones wrote:
> Previously closures had a crude flag which tells if they are
> persistent or transient.  Transient closures (flag = false) last for
> the lifetime of the currently called libnbd function.  Persistent
> closures had an indefinite lifetime which could last for as long as
> the handle.  In language bindings handling persistent closures was
> wasteful as we needed to register a "close callback" to free the
> closure when the handle is closed.  But if you had submitted thousands
> of asynchronous commands you would end up registering thousands of
> close callbacks.
> 
> +++ b/.gitignore
> @@ -108,6 +108,7 @@ Makefile.in
>  /tests/can-trim-flag
>  /tests/can-not-trim-flag
>  /tests/can-zero-flag
> +/tests/closure-lifetimes
Always good to see testsuite coverage of new features :)  I guess I'll
see below if I can offer more ideas for what the test can do.
> +
> +The valid flag is only present in the C API.  It is not needed when
> +using garbage-collected programming languages.
> +
> +=head2 Callbacks and locking
>  
>  The callbacks are invoked at a point where the libnbd lock is held; as
>  such, it is unsafe for the callback to call any C<nbd_*> APIs on the
Missing a change to the auto-retire text and error handling text (since
we have places where we now ignore the return value when calling with
only FREE); it may be worth squashing in:

diff --git i/docs/libnbd.pod w/docs/libnbd.pod
index 4d31c64..1fcbfd7 100644
--- i/docs/libnbd.pod
+++ w/docs/libnbd.pod
@@ -501,9 +501,10 @@ complete.  The completion callback will be invoked
with C<cookie> set
 to the same value returned by the original API such as
 C<nbd_aio_pread_callback> (in rare cases, it is possible that the
 completion callback may fire before the original API has returned).
-If the completion callback returns C<1>, the command is automatically
-retired (there is no need to call C<nbd_aio_command_completed>); for
-any other return value, the command still needs to be retired.
+When C<valid_flag> includes C<LIBNBD_CALLBACK_VALID>, and the
+completion callback returns C<1>, the command is automatically retired
+(there is no need to call C<nbd_aio_command_completed>); for any other
+return value, the command still needs to be retired.

 =head2 Callbacks with C<int *error> parameter

@@ -515,8 +516,9 @@ all of the completion callbacks, include a parameter
C<error>
 containing the value of any error detected so far; if the callback
 function fails, it should assign back into C<error> and return
C<-1>
 to change the resulting error of the overall command.  Assignments
-into C<error> are ignored for any other return value; similarly,
+into C<error> are ignored for any other return value or when
+C<valid_flag> did not contain C<LIBNBD_CALLBACK_VALID>; similarly,
 assigning C<0> into C<error> does not have an effect.

 =head1 SEE ALSO

> +++ b/examples/strict-structured-reads.c
>  static int
> -read_verify (void *opaque, int64_t cookie, int *error)
> +read_verify (unsigned valid_flag, void *opaque, int64_t cookie, int
*error)
>  {
> +  int ret = 0;
> +
> +  if (valid_flag & LIBNBD_CALLBACK_VALID) {
>    struct data *data = opaque;
> -  int ret = -1;
>  
> +    ret = -1;
>    total_reads++;
Was this patch posted with whitespace changes ignored?  Otherwise, it
looks like you missed reindenting things.
>    total_chunks += data->chunks;
>    if (*error)
> @@ -160,7 +167,11 @@ read_verify (void *opaque, int64_t cookie, int *error)
>      data->remaining = r->next;
>      free (r);
>    }
> -  free (data);
> +  }
> +
> +  if (valid_flag & LIBNBD_CALLBACK_FREE)
> +    free (opaque);
> +
>    return ret;
>  }
>  
> diff --git a/generator/generator b/generator/generator
> @@ -1350,11 +1346,10 @@ protocol extensions).";
>    "pread_structured", {
>      default_call with
>      args = [ BytesOut ("buf", "count"); UInt64
"offset";
> -             Closure (false,
> -                      { cbname="chunk";
> +             Closure { cbname="chunk";
>                          cbargs=[BytesIn ("subbuf",
"count");
Indentation.
>                                  UInt64 "offset"; UInt
"status";
> -                                Mutable (Int "error")] });
> +                                Mutable (Int "error")] };
>               Flags "flags" ];
>      ret = RErr;
>      permitted_states = [ Connected ];
> @@ -1541,13 +1536,12 @@ punching a hole.";
>    "block_status", {
>      default_call with
>      args = [ UInt64 "count"; UInt64 "offset";
> -             Closure (false,
> -                      { cbname="extent";
> +             Closure { cbname="extent";
>                          cbargs=[String "metacontext";
and again; I'll quit pointing it out for args=[].
> @@ -3714,28 +3707,16 @@ let print_python_binding name { args; ret } 
> +    | Closure { cbname; cbargs } ->
>         pr "/* Wrapper for %s callback of %s. */\n" cbname name;
>         pr "static int\n";
>         pr "%s_%s_wrapper " name cbname;
> -       C.print_arg_list ~user_data:true cbargs;
> +       C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
>         pr "\n";
>         pr "{\n";
> -       pr "  int ret;\n";
> +       pr "  int ret = 0;\n";
> +       pr "\n";
> +       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
>         pr "  PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
Another place where indentation looks odd, unless this email was
generated with whitespace changes ignored.  (Cleaning up whitespace in a
separate patch to let THIS patch focus on the semantic changes is also
acceptable)

> @@ -4684,16 +4638,24 @@ let print_ocaml_binding (name, { args; ret }) > 
pr "\n";
>         pr "static int\n";
>         pr "%s_%s_wrapper " name cbname;
> -       C.print_arg_list ~user_data:true cbargs;
> +       C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
>         pr "\n";
>         pr "{\n";
> -       pr "  int ret;\n";
> +       pr "  int ret = 0;\n";
>         pr "\n";
> +       pr "  if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
>         pr "  caml_leave_blocking_section ();\n";
Same here.

> +++ b/generator/states-reply-structured.c
> @@ -298,7 +298,7 @@
>           * current error rather than any earlier one. If the callback
fails
>           * without setting errno, then use the server's error below.
>           */
> -        if (cmd->cb.fn.read (cmd->cb.fn_user_data,
> +        if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
>                               cmd->data + (offset - cmd->offset),
>                               0, offset, LIBNBD_READ_ERROR, &scratch)
== -1)
>            if (cmd->error == 0)
We could still optimize this file based on NBD_REPLY_FLAG_DONE, but that
can be a followup.
> @@ -499,7 +499,7 @@
>        /* Call the caller's extent function. */
>        int error = cmd->error;
>  
> -      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
> +      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
>                               meta_context->name, cmd->offset,
>                               &h->bs_entries[1], (length-4) / 4,
&error) == -1)
>          if (cmd->error == 0)
Hmm - no change to the FINISH state, which means you are relying on
command retirement to free chunk/extent instead.  As long as that
happens, we should be okay, though.
> diff --git a/generator/states-reply.c b/generator/states-reply.c
> index 6ea43d5..8f62923 100644
> --- a/generator/states-reply.c
> +++ b/generator/states-reply.c
> @@ -170,9 +170,13 @@ save_reply_state (struct nbd_handle *h)
>    /* Notify the user */
>    if (cmd->cb.callback) {
>      int error = cmd->error;
> +    int r;
>  
>      assert (cmd->type != NBD_CMD_DISC);
> -    switch (cmd->cb.callback (cmd->cb.user_data, cookie,
&error)) {
> +    r = cmd->cb.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
> +                          cmd->cb.user_data, cookie, &error);
> +    cmd->cb.callback = NULL; /* because we've freed it */
> +    switch (r) {
Moving the side effect out of the switch() condition is a reasonable
move; my fault for putting it there in the first place.
>      case -1:
>        if (error)
>          cmd->error = error;
> @@ -190,7 +194,7 @@ save_reply_state (struct nbd_handle *h)
>      h->cmds_in_flight = cmd->next;
>    cmd->next = NULL;
>    if (retire)
> -    free (cmd);
> +    nbd_internal_retire_and_free_command (cmd);
Looks like a nice helper function.  (Side note: using 'git config
diff.orderfile some/file' can be a way to rearrange patches to be easier
to review logically: I think placing lib/internal.h and
generator/generator first, then generator/* and lib/* prior to tests/*
interop/* examples/* would have have made this review easier to read -
maybe I should propose a scripts/git.orderfile similar to nbdkit).

> +++ b/lib/aio.c
> @@ -27,6 +27,24 @@
>  
>  #include "internal.h"
>  
> +/* Internal function which retires and frees a command. */
> +void
> +nbd_internal_retire_and_free_command (struct command *cmd)
> +{
> +  /* Free the callbacks. */
> +  if (cmd->type != NBD_CMD_READ && cmd->cb.fn.extent)
Looks odd that this was not spelled 'cmd->type ==
NBD_CMD_BLOCK_STATUS'.
> +    cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> +                       NULL, 0, NULL, 0, NULL);
> +  if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read)
> +    cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> +                     NULL, 0, 0, 0, NULL);
Perhaps we could even have:
switch (cmd->type) {
case NBD_CMD_READ:
  if (cmd->cb.fn.read) ...
  break;
case NBD_CMD_BLOCK_STATUS:
  if (cmd->cb.fn.extent) ...
  break;
default:
  assert (!cmd->cb.fn.read);
}
> +  if (cmd->cb.callback)
> +    cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
> +                      0, NULL);
> +
> +  free (cmd);
> +}
But as written, the function operates correctly. So up to you if you
want to tweak it.

> @@ -96,6 +96,10 @@ nbd_close (struct nbd_handle *h)
>    if (h == NULL)
>      return;
>  
> +  /* Free user callbacks first. */
> +  if (h->debug_fn)
> +    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
> +
>    for (cc = h->close_callbacks; cc != NULL; cc = cc_next) {
I recommend either setting h->debug_fn = NULL here, or deferring the
FREE callback to after the h->sock->ops->close (h->sock) below.
Otherwise, a future edit to lib/sockets.c to add in a debug statement
there will cause a use-after-free at a distance.
> +++ b/tests/closure-lifetimes.c
> +static int
> +debug_fn (unsigned valid_flag, void *opaque,
> +          const char *context, const char *msg)
> +{
Is it worth assert(!debug_fn_free), to prove that we never have
use-after-free?
> +  if (valid_flag & LIBNBD_CALLBACK_VALID)
> +    debug_fn_valid++;
> +  if (valid_flag & LIBNBD_CALLBACK_FREE)
> +    debug_fn_free++;
> +  return 0;
> +}
> +
> +static int
> +read_cb (unsigned valid_flag, void *opaque,
> +         const void *subbuf, size_t count,
> +         uint64_t offset, unsigned status, int *error)
> +{
Same here.
> +  if (valid_flag & LIBNBD_CALLBACK_VALID)
> +    read_cb_valid++;
> +  if (valid_flag & LIBNBD_CALLBACK_FREE)
> +    read_cb_free++;
> +  return 0;
> +}
> +
> +static int
> +completion_cb (unsigned valid_flag, void *opaque,
> +               int64_t cookie, int *error)
> +{
and again
> +  if (valid_flag & LIBNBD_CALLBACK_VALID)
> +    completion_cb_valid++;
> +  if (valid_flag & LIBNBD_CALLBACK_FREE)
> +    completion_cb_free++;
> +  return 0;
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  struct nbd_handle *nbd;
> +  int64_t cookie;
> +  char buf[512];
> +
> +  /* Check debug functions are freed when a new debug function is
> +   * registered, and when the handle is closed.
> +   */
> +  nbd = nbd_create ();
> +  assert (nbd);
> +
> +  nbd_set_debug_callback (nbd, debug_fn, NULL);
> +  assert (debug_fn_free == 0);
> +
> +  nbd_set_debug_callback (nbd, debug_fn, NULL);
> +  assert (debug_fn_free == 1);
If you add asserts against use-after-free above, you'd also need to
reset debug_fn_free back to 0 here.
> +
> +  nbd_close (nbd);
> +  assert (debug_fn_free == 2);
with knock-on affects to what you assert here.

Is there any way to reliably test whether debug_fn_valid was
incremented? (To some extent, it depends on whether nbd_set_debug was
used, and if we know for sure that we triggered an action that results
in a debug statement).  Testing for >0 is sufficient, testing for a
specific value would be fragile as we may add or remove debug calls in
future refactorings.
> +
> +  /* Test command callbacks are freed when the command is retired. */
> +  nbd = nbd_create ();
> +  assert (nbd);
> +  assert (nbd_connect_command (nbd, nbdkit) == 0);
Side effects in an assert. Nasty. (Maybe you can get away with it if we
explicitly #undef NDEBUG at the top of this file, so the test still
works if someone does ./configure CFLAGS=-DNDEBUG, but splitting the
side effects from the test validation seems wise)
> +
> +  cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
> +                                              read_cb, NULL,
> +                                              completion_cb, NULL, 0);
> +  assert (read_cb_free == 0);
> +  assert (completion_cb_free == 0);
> +  while (!nbd_aio_command_completed (nbd, cookie))
> +    assert (nbd_poll (nbd, -1) >= 0);
More side effects in an assert :(
> +
> +  assert (read_cb_valid == 1);
> +  assert (completion_cb_valid == 1);
> +  assert (read_cb_free == 1);
> +  assert (completion_cb_free == 1);
> +
> +  nbd_close (nbd);
> +
> +  /* Test command callbacks are freed if the handle is closed without
> +   * running the commands.
> +   *
> +   * Note it's possible that nbd_aio_pread_structured_callback might
> +   * actually complete the command if the server is very fast.
We can use --filter=delay delay-read=15 to ensure the server is not very
fast, to reduce the risk of the race (but only for this part of the
test, not the earlier part where we do wait for completion - which means
two separate parameter lists for nbd_connect_command() calls...)
> +   */
> +  read_cb_valid = read_cb_free > +    completion_cb_valid =
completion_cb_free = 0;
> +  nbd = nbd_create ();
> +  assert (nbd);
> +  assert (nbd_connect_command (nbd, nbdkit) == 0);
side effects :(
> +
> +  cookie = nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, 0,
> +                                              read_cb, NULL,
> +                                              completion_cb, NULL, 0);
> +  nbd_close (nbd);
> +
> +  assert (read_cb_free == 1);
> +  assert (completion_cb_free == 1);
Worth asserting that read_cb_valid and completion_cb_valid are 0 (if we
are confident in our ability to strand the command unretired due to use
of a filter delay)?
> +
> +  exit (EXIT_SUCCESS);
> +}
> diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
> index 95e029b..9e88d6b 100644
> --- a/tests/meta-base-allocation.c
Most of my comments can be addressed by followups or pertain to the new
test; I'm okay if you want to push this without posting v4.

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

On 7/25/19 10:43 AM, Eric Blake wrote:
>> +++ b/generator/states-reply-structured.c
>> @@ -298,7 +298,7 @@
>>           * current error rather than any earlier one. If the callback
fails
>>           * without setting errno, then use the server's error
below.
>>           */
>> -        if (cmd->cb.fn.read (cmd->cb.fn_user_data,
>> +        if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
>>                               cmd->data + (offset - cmd->offset),
>>                               0, offset, LIBNBD_READ_ERROR,
&scratch) == -1)
>>            if (cmd->error == 0)
> 
> We could still optimize this file based on NBD_REPLY_FLAG_DONE, but that
> can be a followup.
> 
>> @@ -499,7 +499,7 @@
>>        /* Call the caller's extent function. */
>>        int error = cmd->error;
>>  
>> -      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
>> +      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
>>                               meta_context->name, cmd->offset,
>>                               &h->bs_entries[1], (length-4) / 4,
&error) == -1)
>>          if (cmd->error == 0)
> 
> Hmm - no change to the FINISH state, which means you are relying on
> command retirement to free chunk/extent instead.  As long as that
> happens, we should be okay, though.
Another thought: if a user calls nbd_aio_pread_structured_callback (...,
chunk, &data, callback, &data, ...), where chunk ignores FREE but
callback(FREE) calls free(data), is there any problem with the fact that
we may end up with a call order of callback(VALID|FREE) before
chunk(FREE)?  I think we're okay - as long as chunk doesn't dereference
data except when VALID is set, then the fact that chunk(FREE) is called
with a stale pointer should not be a problem.

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

On Thu, Jul 25, 2019 at 10:43:05AM -0500, Eric Blake
wrote:
> On 7/25/19 8:07 AM, Richard W.M. Jones wrote:
> > @@ -499,7 +499,7 @@
> >        /* Call the caller's extent function. */
> >        int error = cmd->error;
> >  
> > -      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
> > +      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
> >                               meta_context->name, cmd->offset,
> >                               &h->bs_entries[1], (length-4) /
4, &error) == -1)
> >          if (cmd->error == 0)
> 
> Hmm - no change to the FINISH state, which means you are relying on
> command retirement to free chunk/extent instead.  As long as that
> happens, we should be okay, though.
I didn't understand this one.  Is it wrong to add the VALID
flag here?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Thu, Jul 25, 2019 at 11:01:18AM -0500, Eric Blake
wrote:
> On 7/25/19 10:43 AM, Eric Blake wrote:
> 
> >> +++ b/generator/states-reply-structured.c
> >> @@ -298,7 +298,7 @@
> >>           * current error rather than any earlier one. If the
callback fails
> >>           * without setting errno, then use the server's error
below.
> >>           */
> >> -        if (cmd->cb.fn.read (cmd->cb.fn_user_data,
> >> +        if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
> >>                               cmd->data + (offset -
cmd->offset),
> >>                               0, offset, LIBNBD_READ_ERROR,
&scratch) == -1)
> >>            if (cmd->error == 0)
> > 
> > We could still optimize this file based on NBD_REPLY_FLAG_DONE, but
that
> > can be a followup.
> > 
> >> @@ -499,7 +499,7 @@
> >>        /* Call the caller's extent function. */
> >>        int error = cmd->error;
> >>  
> >> -      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
> >> +      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
> >>                               meta_context->name,
cmd->offset,
> >>                               &h->bs_entries[1], (length-4)
/ 4, &error) == -1)
> >>          if (cmd->error == 0)
> > 
> > Hmm - no change to the FINISH state, which means you are relying on
> > command retirement to free chunk/extent instead.  As long as that
> > happens, we should be okay, though.
> 
> Another thought: if a user calls nbd_aio_pread_structured_callback (...,
> chunk, &data, callback, &data, ...), where chunk ignores FREE but
> callback(FREE) calls free(data), is there any problem with the fact that
> we may end up with a call order of callback(VALID|FREE) before
> chunk(FREE)?  I think we're okay - as long as chunk doesn't
dereference
> data except when VALID is set, then the fact that chunk(FREE) is called
> with a stale pointer should not be a problem.
There's no claim about what order we call the free functions for
different callbacks, so I guess callers will simply need to deal with
it.  It doesn't affect our use in the language bindings.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 7/25/19 12:01 PM, Richard W.M. Jones wrote:
> On Thu, Jul 25, 2019 at 10:43:05AM -0500, Eric Blake wrote:
>> On 7/25/19 8:07 AM, Richard W.M. Jones wrote:
>>> @@ -499,7 +499,7 @@
>>>        /* Call the caller's extent function. */
>>>        int error = cmd->error;
>>>  
>>> -      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
>>> +      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
>>>                               meta_context->name,
cmd->offset,
>>>                               &h->bs_entries[1], (length-4)
/ 4, &error) == -1)
>>>          if (cmd->error == 0)
>>
>> Hmm - no change to the FINISH state, which means you are relying on
>> command retirement to free chunk/extent instead.  As long as that
>> happens, we should be okay, though.
> 
> I didn't understand this one.  Is it wrong to add the VALID
> flag here?
No, the VALID flag here is correct; my comment (with insufficient
context) was tied to the fact that this is the last hunk of changes to
this file, which means we did not change the FINISH state to call
read(FREE) or extent(FREE) (and instead defer the freeing to when we
retire the command).  So nothing to change in what you have here.

In fact, thinking a bit more - my followup patch to call
read(VALID|FREE) in more places is worthwhile (because it reduces the
number of callbacks compared to read(VALID);read(FREE)); but teaching
FINISH to call read(FREE) does not optimize how many callbacks are made,
only the point in time in which they are made (freeing the read callback
as soon as we know the command is done, and before the completion
callback runs), nor does it remove the requirement to still have code
for read(FREE) in the retirement code (for when the command is
stranded).  An argument for calling read(FREE) in FINISH is to avoid the
surprise mentioned in the other email where callback(FREE, data) &&
read(FREE, data) (because the user passed the same data to both
callbacks) results in the read callback seeing a stale pointer, but then
again, it's the caller's burden to not dereference the pointer if they
share the data between callbacks, and not necessarily a case where we
MUST call read(FREE) as soon as possible.

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