[Libguestfs] [PATCH libnbd] generator: Fix race condition when validating h->state.
Richard W.M. Jones
rjones at redhat.com
Tue Jun 4 21:48:30 UTC 2019
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/
More information about the Libguestfs
mailing list