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