Libguestfs - Aug 2020 - Re: [nbdkit PATCH 2/3] server: Expose final thread_model to filter's .get_ready

On Fri, Aug 07, 2020 at 05:00:52PM -0500, Eric Blake
wrote:
> The next patch wants to add a filter that will prevent DoS attacks
> from a plaintext client; to be successful, the filter must guarantee
> that nbdkit did not settle on SERIALIZE_CONNECTIONS.  The easiest way
> to solve this is to expose the final thread model to .get_ready, which
> is after the point where .config_complete may have altered it, and
> before any connections are permitted.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  docs/nbdkit-filter.pod          | 9 ++++++++-
>  include/nbdkit-filter.h         | 3 ++-
>  server/filters.c                | 4 ++--
>  filters/extentlist/extentlist.c | 3 ++-
>  filters/log/log.c               | 2 +-
>  filters/rate/rate.c             | 2 +-
>  filters/stats/stats.c           | 2 +-
>  tests/test-layers-filter.c      | 2 +-
>  8 files changed, 18 insertions(+), 9 deletions(-)
This is fine.  Is this something that we would also with to add to
plugin->get_ready in V3 API?  If so it would be a good idea to add
this to TODO.

ACK

Rich.
> diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
> index b6ed5504..32db0938 100644
> --- a/docs/nbdkit-filter.pod
> +++ b/docs/nbdkit-filter.pod
> @@ -298,11 +298,18 @@ with an error message and return C<-1>.
> 
>  =head2 C<.get_ready>
> 
> - int (*get_ready) (nbdkit_next_get_ready *next, void *nxdata);
> + int (*get_ready) (nbdkit_next_get_ready *next, void *nxdata,
> +                   int thread_model);
> 
>  This intercepts the plugin C<.get_ready> method and can be used by
the
>  filter to get ready to serve requests.
> 
> +The C<thread_model> parameter informs the filter about the final
> +thread model chosen by nbdkit after considering the results of
> +C<.thread_model> of all filters in the chain after C<.config>.
This
> +does not need to be passed on to C<next>, as the model can no longer
> +be altered at this point.
> +
>  If there is an error, C<.get_ready> should call
C<nbdkit_error> with
>  an error message and return C<-1>.
> 
> diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
> index 708a1b54..6aba1aec 100644
> --- a/include/nbdkit-filter.h
> +++ b/include/nbdkit-filter.h
> @@ -166,7 +166,8 @@ struct nbdkit_filter {
>                            nbdkit_backend *nxdata);
>    const char *config_help;
>    int (*thread_model) (void);
> -  int (*get_ready) (nbdkit_next_get_ready *next, nbdkit_backend *nxdata);
> +  int (*get_ready) (nbdkit_next_get_ready *next, nbdkit_backend *nxdata,
> +                    int thread_model);
>    int (*after_fork) (nbdkit_next_after_fork *next, nbdkit_backend
*nxdata);
>    int (*preconnect) (nbdkit_next_preconnect *next, nbdkit_backend *nxdata,
>                       int readonly);
> diff --git a/server/filters.c b/server/filters.c
> index 90a9a948..0cfae344 100644
> --- a/server/filters.c
> +++ b/server/filters.c
> @@ -183,10 +183,10 @@ filter_get_ready (struct backend *b)
>  {
>    struct backend_filter *f = container_of (b, struct backend_filter,
backend);
> 
> -  debug ("%s: get_ready", b->name);
> +  debug ("%s: get_ready thread_model=%d", b->name,
thread_model);
> 
>    if (f->filter.get_ready) {
> -    if (f->filter.get_ready (next_get_ready, b->next) == -1)
> +    if (f->filter.get_ready (next_get_ready, b->next, thread_model)
== -1)
>        exit (EXIT_FAILURE);
>    }
>    else
> diff --git a/filters/extentlist/extentlist.c
b/filters/extentlist/extentlist.c
> index 3005b790..dfb5e808 100644
> --- a/filters/extentlist/extentlist.c
> +++ b/filters/extentlist/extentlist.c
> @@ -260,7 +260,8 @@ parse_extentlist (void)
>  }
> 
>  static int
> -extentlist_get_ready (nbdkit_next_get_ready *next, void *nxdata)
> +extentlist_get_ready (nbdkit_next_get_ready *next, void *nxdata,
> +                      int thread_model)
>  {
>    parse_extentlist ();
> 
> diff --git a/filters/log/log.c b/filters/log/log.c
> index f8da9ad8..6a3a9b14 100644
> --- a/filters/log/log.c
> +++ b/filters/log/log.c
> @@ -100,7 +100,7 @@ log_config_complete (nbdkit_next_config_complete *next,
void *nxdata)
> 
>  /* Open the logfile. */
>  static int
> -log_get_ready (nbdkit_next_get_ready *next, void *nxdata)
> +log_get_ready (nbdkit_next_get_ready *next, void *nxdata, int
thread_model)
>  {
>    int fd;
> 
> diff --git a/filters/rate/rate.c b/filters/rate/rate.c
> index 32c47fdf..325f5657 100644
> --- a/filters/rate/rate.c
> +++ b/filters/rate/rate.c
> @@ -145,7 +145,7 @@ rate_config (nbdkit_next_config *next, void *nxdata,
>  }
> 
>  static int
> -rate_get_ready (nbdkit_next_get_ready *next, void *nxdata)
> +rate_get_ready (nbdkit_next_get_ready *next, void *nxdata, int
thread_model)
>  {
>    /* Initialize the global buckets. */
>    bucket_init (&read_bucket, rate, BUCKET_CAPACITY);
> diff --git a/filters/stats/stats.c b/filters/stats/stats.c
> index 688078ec..687dd05b 100644
> --- a/filters/stats/stats.c
> +++ b/filters/stats/stats.c
> @@ -210,7 +210,7 @@ stats_config_complete (nbdkit_next_config_complete
*next, void *nxdata)
>  }
> 
>  static int
> -stats_get_ready (nbdkit_next_get_ready *next, void *nxdata)
> +stats_get_ready (nbdkit_next_get_ready *next, void *nxdata, int
thread_model)
>  {
>    int fd;
> 
> diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c
> index 5c5b3f0f..3f295588 100644
> --- a/tests/test-layers-filter.c
> +++ b/tests/test-layers-filter.c
> @@ -84,7 +84,7 @@ test_layers_filter_config_complete
(nbdkit_next_config_complete *next,
> 
>  static int
>  test_layers_filter_get_ready (nbdkit_next_get_ready *next,
> -                              void *nxdata)
> +                              void *nxdata, int thread_model)
>  {
>    DEBUG_FUNCTION;
>    return next (nxdata);
> -- 
> 2.28.0
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
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 8/10/20 8:01 AM, Richard W.M. Jones wrote:
> On Fri, Aug 07, 2020 at 05:00:52PM -0500, Eric Blake wrote:
>> The next patch wants to add a filter that will prevent DoS attacks
>> from a plaintext client; to be successful, the filter must guarantee
>> that nbdkit did not settle on SERIALIZE_CONNECTIONS.  The easiest way
>> to solve this is to expose the final thread model to .get_ready, which
>> is after the point where .config_complete may have altered it, and
>> before any connections are permitted.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   docs/nbdkit-filter.pod          | 9 ++++++++-
>>   include/nbdkit-filter.h         | 3 ++-
>>   server/filters.c                | 4 ++--
>>   filters/extentlist/extentlist.c | 3 ++-
>>   filters/log/log.c               | 2 +-
>>   filters/rate/rate.c             | 2 +-
>>   filters/stats/stats.c           | 2 +-
>>   tests/test-layers-filter.c      | 2 +-
>>   8 files changed, 18 insertions(+), 9 deletions(-)
> 
> This is fine.  Is this something that we would also with to add to
> plugin->get_ready in V3 API?  If so it would be a good idea to add
> this to TODO.
We might as well, so I'll tweak TODO.  But at the moment, I'm having a 
hard time coming up with a justification for why any existing v2 plugin 
would need to know this information, so without a client, I'm reluctant 
to introduce a plugin-only nbdkit_final_thread_model() just to be 
deprecated when v3 comes out.

Oh, one other oddity: I've been thinking about adding a fifth threading 
model (SERIALIZE_RETIREMENT), where commands can be serviced in parallel 
on a single connection, but replies to the client are reordered to match 
the client's submission order; for filters, exposing a new threading 
model through .get_ready is not a problem (all filters are in tree), but 
for plugins, such an action would mean that the plugin may get a thread 
model selected at runtime that was unknown to the plugin at compile 
time.  If the plugin is trying to make decisions on how much internal 
locking it needs to create, those decisions become more awkward when 
having to deal with an unknown thread model (then again, robust code 
should always be written to assume as much parallelism as possible, and 
only optimize for less locking on known models that are more serialized).

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

On Mon, Aug 10, 2020 at 08:13:31AM -0500, Eric Blake
wrote:
> On 8/10/20 8:01 AM, Richard W.M. Jones wrote:
> >On Fri, Aug 07, 2020 at 05:00:52PM -0500, Eric Blake wrote:
> >>The next patch wants to add a filter that will prevent DoS attacks
> >>from a plaintext client; to be successful, the filter must
guarantee
> >>that nbdkit did not settle on SERIALIZE_CONNECTIONS.  The easiest
way
> >>to solve this is to expose the final thread model to .get_ready,
which
> >>is after the point where .config_complete may have altered it, and
> >>before any connections are permitted.
> >>
> >>Signed-off-by: Eric Blake <eblake@redhat.com>
> >>---
> >>  docs/nbdkit-filter.pod          | 9 ++++++++-
> >>  include/nbdkit-filter.h         | 3 ++-
> >>  server/filters.c                | 4 ++--
> >>  filters/extentlist/extentlist.c | 3 ++-
> >>  filters/log/log.c               | 2 +-
> >>  filters/rate/rate.c             | 2 +-
> >>  filters/stats/stats.c           | 2 +-
> >>  tests/test-layers-filter.c      | 2 +-
> >>  8 files changed, 18 insertions(+), 9 deletions(-)
> >
> >This is fine.  Is this something that we would also with to add to
> >plugin->get_ready in V3 API?  If so it would be a good idea to add
> >this to TODO.
> 
> We might as well, so I'll tweak TODO.  But at the moment, I'm
having
> a hard time coming up with a justification for why any existing v2
> plugin would need to know this information, so without a client, I'm
> reluctant to introduce a plugin-only nbdkit_final_thread_model()
> just to be deprecated when v3 comes out.
It's certainly hard to imagine why a plugin would worry about things
being more serialized than they requested, but OTOH it does make the
filter and plugin APIs consistent (w.r.t. the informational parameters
they both receive) which is nice.
> Oh, one other oddity: I've been thinking about adding a fifth
> threading model (SERIALIZE_RETIREMENT), where commands can be
> serviced in parallel on a single connection, but replies to the
> client are reordered to match the client's submission order; for
> filters, exposing a new threading model through .get_ready is not a
> problem (all filters are in tree), but for plugins, such an action
> would mean that the plugin may get a thread model selected at
> runtime that was unknown to the plugin at compile time.  If the
> plugin is trying to make decisions on how much internal locking it
> needs to create, those decisions become more awkward when having to
> deal with an unknown thread model (then again, robust code should
> always be written to assume as much parallelism as possible, and
> only optimize for less locking on known models that are more
> serialized).
Sure; are there really clients which have this restriction?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html