Re: [PATCH] futex: Avoid reusing outdated pi_state.

[Jiri Slaby]

On 16. 01. 24, 14:08, Sebastian Andrzej Siewior wrote:
> Jiri Slaby reported a futex state inconsistency resulting in -EINVAL
> during a lock operation for a PI futex. A requirement is that the lock
> process is interrupted by a timeout or signal:
>
> T1		T2
> *owns* futex
> 		futex_lock_pi()
>              	*create PI state, attach to it, queue RT waiter*
> 		rt_mutex_wait_proxy_lock() /* -ETIMEDOUT */
> 		rt_mutex_cleanup_proxy_lock()
> 		  remove_waiter()
>
> futex_unlock_pi()
> spin_lock(&hb->lock);
> top_waiter = futex_top_waiter(hb, &key);
> /* top_waiter is NULL, do_uncontended */
> spin_unlock(&hb->lock);
>
> To spice things up, player T3 and T4 enter the game:
>
> 	T3 			T4
> 	*acquires futex in userland*
> 				futex_lock_pi()
> 				futex_q_lock(&q);
> 				futex_lock_pi_atomic()
> 				top_waiter = futex_top_waiter(hb, key);
> 				/* top_waiter is from T1, still */
> 				attach_to_pi_state()
> 				/* Here -EINVAL is returned because uval
> 				 *  points to T3 but pi_state says T1.
> 				 */
>
> We must not unlock the futex for userland as long as there is still a
> state pending in kernel. It can be used by further futex_lock_pi()
> caller (as it has been observed by futex_unlock_pi()). The caller will
> observe an outdated state of the futex because it was not removed during
> unlock operation in kernel.
>
> The lock can not be handed over to T1 because it already gave up and
> stared to clean up.
> All futex_q entries point to the same pi_state and the pi_mutex has no
> waiters. A waiter can not be enqueued because hb->lock +
> pi_mutex.wait_lock is acquired (by the unlock operation) and the same
> ordering is used by futex_lock_pi() during locking.
>
> Remove all futex_q entries from the hb list which point to the futex if
> no waiter has been observed. This closes the race window by removing all
> pointer to the previous in-kernel state.
> The leaving futex_lock_pi() caller can clean up the pi-state once it
> acquires hb->lock. The following futex_lock_pi() caller will create a
> new in-kernel state.
> The optional removal from hb->chain is only needed if the futex was not
> acquired because it might have been done by the unlock path with
> hb->lock acquired.
>
> Fixes: fbeb558b0dd0d ("futex/pi: Fix recursive rt_mutex waiter state")
> Reported-by: Jiri Slaby <jirislaby@xxxxxxxxxx>

Tested-by: Jiri Slaby <jirislaby@xxxxxxxxxx>

> Closes: c737a604-d441-49c6-a5cd-ef01e9f2a454@xxxxxxxxxx
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>

thanks,
--
js
suse labs