From 7c69c9b51f61a28dbb44599a176eaa0e54467013 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 3 Oct 2023 08:23:33 +0200 Subject: [PATCH] BUG/MAJOR: plock: fix major bug in pl_take_w() introduced with EBO When EBO was brought to pl_take_w() by plock commit 60d750d ("plock: use EBO when waiting for readers to leave in take_w() and stow()"), a mistake was made: the mask against which the current value of the lock is tested excludes the first reader like in stow(), but it must not because it was just obtained via an ldadd() which means that it doesn't count itself. The problem this causes is that if there is exactly one reader when a writer grabs the lock, the writer will not wait for it to leave before starting its operations. The solution consists in checking for any reader in the IF. However the mask passed to pl_wait_unlock_*() must still exclude the lowest bit as it's verified after a subsequent load. Kudos to Remi Tricot-Le Breton for reporting and bisecting this issue with a reproducer. No backport is needed since this was brought in 2.9-dev3 with commit 8178a5211 ("MAJOR: threads/plock: update the embedded library again"). The code is now on par with plock commit ada70fe. --- include/import/plock.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/import/plock.h b/include/import/plock.h index 0ac7020b1..fc001e2d1 100644 --- a/include/import/plock.h +++ b/include/import/plock.h @@ -553,7 +553,7 @@ static unsigned int pl_wait_new_int(const unsigned int *lock, const unsigned int __pl_r = pl_wait_unlock_long(__lk_r, __msk_r); \ } \ /* wait for all other readers to leave */ \ - if (__builtin_expect(__pl_r & (PLOCK64_RL_ANY & ~PLOCK64_RL_1), 0)) \ + if (__builtin_expect(__pl_r & PLOCK64_RL_ANY, 0)) \ __pl_r = pl_wait_unlock_long(__lk_r, (PLOCK64_RL_ANY & ~PLOCK64_RL_1)) - __set_r; \ pl_barrier(); \ 0; \ @@ -570,7 +570,7 @@ static unsigned int pl_wait_new_int(const unsigned int *lock, const unsigned int __pl_r = pl_wait_unlock_int(__lk_r, __msk_r); \ } \ /* wait for all other readers to leave */ \ - if (__builtin_expect(__pl_r & (PLOCK32_RL_ANY & ~PLOCK32_RL_1), 0)) \ + if (__builtin_expect(__pl_r & PLOCK32_RL_ANY, 0)) \ __pl_r = pl_wait_unlock_int(__lk_r, (PLOCK32_RL_ANY & ~PLOCK32_RL_1)) - __set_r; \ pl_barrier(); \ 0; \