[PATCH v5 07/14] futex: Move the retry_private label.

Sebastian Andrzej Siewior posted 14 patches 1 year ago
There is a newer version of this series
[PATCH v5 07/14] futex: Move the retry_private label.
Posted by Sebastian Andrzej Siewior 1 year ago
The label futex_requeue in futex_requeue() and futex_wake_op() is jumped
after the lock is dropped in a retry operation. This assumes that the hb
does not need to be hashed again. If hb is resized then the hb can
change if the reference is dropped.

Move the retry_private label before the hashing operation.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/futex/requeue.c  | 2 +-
 kernel/futex/waitwake.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/futex/requeue.c b/kernel/futex/requeue.c
index 80e99a498de28..0395740ce5e71 100644
--- a/kernel/futex/requeue.c
+++ b/kernel/futex/requeue.c
@@ -443,10 +443,10 @@ int futex_requeue(u32 __user *uaddr1, unsigned int flags1,
 	if (requeue_pi && futex_match(&key1, &key2))
 		return -EINVAL;
 
+retry_private:
 	hb1 = futex_hash(&key1);
 	hb2 = futex_hash(&key2);
 
-retry_private:
 	futex_hb_waiters_inc(hb2);
 	double_lock_hb(hb1, hb2);
 
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
index fdb9fcaaf9fba..ec73a6ea7462a 100644
--- a/kernel/futex/waitwake.c
+++ b/kernel/futex/waitwake.c
@@ -267,10 +267,10 @@ int futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
 	if (unlikely(ret != 0))
 		return ret;
 
+retry_private:
 	hb1 = futex_hash(&key1);
 	hb2 = futex_hash(&key2);
 
-retry_private:
 	double_lock_hb(hb1, hb2);
 	op_ret = futex_atomic_op_inuser(op, uaddr2);
 	if (unlikely(op_ret < 0)) {
-- 
2.45.2
Re: [PATCH v5 07/14] futex: Move the retry_private label.
Posted by Thomas Gleixner 1 year ago
On Mon, Dec 16 2024 at 00:00, Sebastian Andrzej Siewior wrote:
> The label futex_requeue in futex_requeue() and futex_wake_op() is jumped
> after the lock is dropped in a retry operation.

The label is jumped? 

> This assumes that the hb does not need to be hashed again. If hb is
> resized then the hb can change if the reference is dropped.

Again 'hb' and the confusion of hash bucket (hb) resize.

> Move the retry_private label before the hashing operation.

The overall explanation is not really comprehensible.

    futex: Re-evaluate the hash bucket after dropping the lock

     Sebastian Andrzej Siewior wrote:

     In futex_requeue() and futex_wake_op() the hash bucket lock is
     dropped in the failure paths for handling page faults and other
     error scenarios. After that the code jumps back to retry_private
     which relocks the hash bucket[s] under the assumption that the hash
     bucket pointer which was retrieved via futex_hash() is still valid.
     
     With resizable private hash buckets, that assumption is not longer
     true as the waiters can be moved to a larger hash in the meantime.

     Move the retry_private label above the hashing function to handle
     this correctly.

Or so.

Thanks,

        tglx