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; >