Richard W.M. Jones
2019-Jun-04 11:23 UTC
[Libguestfs] [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
Eric Blake
2019-Jun-04 11:58 UTC
Re: [Libguestfs] [PATCH libnbd] generator: Fix race condition when validating h->state.
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 ( > functionDoesn'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
Richard W.M. Jones
2019-Jun-04 21:48 UTC
Re: [Libguestfs] [PATCH libnbd] generator: Fix race condition when validating h->state.
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/
Reasonably Related Threads
- Re: [PATCH libnbd] generator: Fix race condition when validating h->state.
- [PATCH libnbd 0/2] Avoid lock and error overhead on some calls.
- [PATCH libnbd 0/4] lib: Atomically update h->state.
- [libnbd PATCH 0/2] Drop generated file from git
- Re: [PATCH libnbd 2/4] lib: Split nbd_aio_is_* functions into internal.