Libguestfs - Jun 2019 - [PATCH libnbd] generator: Fix race condition when validating h->state.

The code for (eg) nbd_aio_pread starts by checking this outside
the lock:

  if (!(nbd_aio_is_ready (h) || nbd_aio_is_processing (h))) {

However there can still be a race condition even though h->state is
atomic:

  Thread A                         Thread B
  (in a call that holds h->lock)   (calling nbd_aio_pread)
  --------------------------------------------------------------------
  h->state is processing
                                   checks nbd_aio_is_ready
                                     (it's false)
  h->state is moved to READY
                                   checks nbd_aio_is_processing
                                     (it's false)
                                   validation check fails

However the state was valid so the validation check should have
succeeded.

Fixes commit e63a11736930c381a79a8cc2d03844cfff5db3ef.

Thanks: Eric Blake for identifying the race condition above.
---
 generator/generator | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/generator/generator b/generator/generator
index ea6f739..8068762 100755
--- a/generator/generator
+++ b/generator/generator
@@ -2820,10 +2820,9 @@ let generate_lib_api_c ()        pr " 
nbd_internal_reset_error (\"nbd_%s\");\n" name;
       pr "\n"
     );
+    if is_locked then
+      pr "  pthread_mutex_lock (&h->lock);\n";
     if permitted_states <> [] then (
-      pr "  /* We can check the state outside the handle lock because
the\n";
-      pr "   * the state is atomic.\n";
-      pr "   */\n";
       let tests          List.map (
           function
@@ -2842,8 +2841,6 @@ let generate_lib_api_c ()        pr "  }\n";
       pr "\n"
     );
-    if is_locked then
-      pr "  pthread_mutex_lock (&h->lock);\n";
     pr "  ret = nbd_unlocked_%s (h" name;
     let argnames = List.flatten (List.map name_of_arg args) in
     List.iter (pr ", %s") argnames;
-- 
2.21.0

On 6/4/19 6:23 AM, Richard W.M. Jones wrote:
> The code for (eg) nbd_aio_pread starts by checking this outside
> the lock:
> 
>   if (!(nbd_aio_is_ready (h) || nbd_aio_is_processing (h))) {
> 
> However there can still be a race condition even though h->state is
> atomic:
> 
>   Thread A                         Thread B
>   (in a call that holds h->lock)   (calling nbd_aio_pread)
>   --------------------------------------------------------------------
>   h->state is processing
>                                    checks nbd_aio_is_ready
>                                      (it's false)
>   h->state is moved to READY
>                                    checks nbd_aio_is_processing
>                                      (it's false)
>                                    validation check fails
> 
> However the state was valid so the validation check should have
> succeeded.
> 
> Fixes commit e63a11736930c381a79a8cc2d03844cfff5db3ef.
> 
> Thanks: Eric Blake for identifying the race condition above.
> ---
>  generator/generator | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/generator/generator b/generator/generator
> index ea6f739..8068762 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -2820,10 +2820,9 @@ let generate_lib_api_c () >        pr " 
nbd_internal_reset_error (\"nbd_%s\");\n" name;
>        pr "\n"
>      );
> +    if is_locked then
> +      pr "  pthread_mutex_lock (&h->lock);\n";
>      if permitted_states <> [] then (
> -      pr "  /* We can check the state outside the handle lock because
the\n";
> -      pr "   * the state is atomic.\n";
> -      pr "   */\n";
>        let tests >          List.map (
>            function
Doesn't this code emit 'return -1' on error,
> @@ -2842,8 +2841,6 @@ let generate_lib_api_c () >        pr " 
}\n";
>        pr "\n"
>      );
> -    if is_locked then
> -      pr "  pthread_mutex_lock (&h->lock);\n";
but returning -1 without releasing the lock is wrong. I would expect a
bit more change to keep the locking sane, but once that is in place, the
patch does solve one race.
>      pr "  ret = nbd_unlocked_%s (h" name;
>      let argnames = List.flatten (List.map name_of_arg args) in
>      List.iter (pr ", %s") argnames;
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Jun 04, 2019 at 06:58:56AM -0500, Eric Blake
wrote:
> On 6/4/19 6:23 AM, Richard W.M. Jones wrote:
> > The code for (eg) nbd_aio_pread starts by checking this outside
> > the lock:
> > 
> >   if (!(nbd_aio_is_ready (h) || nbd_aio_is_processing (h))) {
> > 
> > However there can still be a race condition even though h->state is
> > atomic:
> > 
> >   Thread A                         Thread B
> >   (in a call that holds h->lock)   (calling nbd_aio_pread)
> >   --------------------------------------------------------------------
> >   h->state is processing
> >                                    checks nbd_aio_is_ready
> >                                      (it's false)
> >   h->state is moved to READY
> >                                    checks nbd_aio_is_processing
> >                                      (it's false)
> >                                    validation check fails
> > 
> > However the state was valid so the validation check should have
> > succeeded.
> > 
> > Fixes commit e63a11736930c381a79a8cc2d03844cfff5db3ef.
> > 
> > Thanks: Eric Blake for identifying the race condition above.
> > ---
> >  generator/generator | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/generator/generator b/generator/generator
> > index ea6f739..8068762 100755
> > --- a/generator/generator
> > +++ b/generator/generator
> > @@ -2820,10 +2820,9 @@ let generate_lib_api_c () > >        pr
"  nbd_internal_reset_error (\"nbd_%s\");\n" name;
> >        pr "\n"
> >      );
> > +    if is_locked then
> > +      pr "  pthread_mutex_lock (&h->lock);\n";
> >      if permitted_states <> [] then (
> > -      pr "  /* We can check the state outside the handle lock
because the\n";
> > -      pr "   * the state is atomic.\n";
> > -      pr "   */\n";
> >        let tests > >          List.map (
> >            function
> 
> Doesn't this code emit 'return -1' on error,
> 
> > @@ -2842,8 +2841,6 @@ let generate_lib_api_c () > >        pr
"  }\n";
> >        pr "\n"
> >      );
> > -    if is_locked then
> > -      pr "  pthread_mutex_lock (&h->lock);\n";
> 
> but returning -1 without releasing the lock is wrong. I would expect a
> bit more change to keep the locking sane, but once that is in place, the
> patch does solve one race.
> 
> >      pr "  ret = nbd_unlocked_%s (h" name;
> >      let argnames = List.flatten (List.map name_of_arg args) in
> >      List.iter (pr ", %s") argnames;
> > 
Yes this was bogus.

Moreover when we get around to making h->state updates atomic when
viewed outside the lock, we don't even need this.  I have a
half-working patch for that but it's a bit more complicated than I
thought initially.  I'll post it after I have the time to fix it, but
meetings are intervening.

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/