linux-kernel - Re: [PATCH v4 05/11] futex: Track the futex hash bucket.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87a5d3cr6j.ffs@tglx>
Date: Tue, 10 Dec 2024 19:45:56 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 linux-kernel@...r.kernel.org
Cc: André Almeida <andrealmeid@...lia.com>, Darren Hart
 <dvhart@...radead.org>, Davidlohr Bueso <dave@...olabs.net>, Ingo Molnar
 <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>, Peter Zijlstra
 <peterz@...radead.org>, Valentin Schneider <vschneid@...hat.com>, Waiman
 Long <longman@...hat.com>, Sebastian Andrzej Siewior
 <bigeasy@...utronix.de>
Subject: Re: [PATCH v4 05/11] futex: Track the futex hash bucket.

On Tue, Dec 03 2024 at 17:42, Sebastian Andrzej Siewior wrote:
> Add futex_hash_get/put() to keep the assigned hash_bucket around while a
> futex operation is performed. Have RCU lifetime guarantee for
> futex_hash_bucket_private.
>
> This is should have the right amount of gets/ puts so that the private

This is should have? This either has or not :)

>  struct futex_hash_bucket *futex_hash(union futex_key *key)
>  {
> -	struct futex_hash_bucket *fhb;
> +	struct futex_hash_bucket_private *hb_p = NULL;
>  	u32 hash;
>  
> -	fhb = current->mm->futex_hash_bucket;
> -	if (fhb && futex_key_is_private(key)) {
> -		u32 hash_mask = current->mm->futex_hash_mask;
> +	if (futex_key_is_private(key)) {
> +		guard(rcu)();
> +
> +		do {
> +			hb_p = rcu_dereference(current->mm->futex_hash_bucket);
> +		} while (hb_p && !rcuref_get(&hb_p->users));

This loop really wants an explanation about the potential loop
duration.

> +void futex_hash_put(struct futex_hash_bucket *hb)
> +{
> +	struct futex_hash_bucket_private *hb_p;
> +
> +	if (hb->hb_slot == 0)
> +		return;
> +	hb_p = container_of(hb, struct futex_hash_bucket_private,
> +			    queues[hb->hb_slot - 1]);

Duh. This off by one abuse of hb_slot is really counter intuitive. It
took me a while to wrap my head around it.

The structure has a 4 byte hole, so adding a private flag or such is
feasible without going over a cache line, unless lockdep or rt is
enabled, but in that case it expands into a second cache line anyway.

> +	futex_hash_priv_put(hb_p);
> +}
> +
> +void futex_hash_get(struct futex_hash_bucket *hb)
> +{
> +	struct futex_hash_bucket_private *hb_p;
> +
> +	if (hb->hb_slot == 0)
> +		return;
> +
> +	hb_p = container_of(hb, struct futex_hash_bucket_private,
> +			    queues[hb->hb_slot - 1]);
> +	/* The ref needs to be owned by the caller so this can't fail */

reference please. This is not twatter. But see below.

> +	WARN_ON_ONCE(!rcuref_get(&hb_p->users));
> +}
>  
>  /**
>   * futex_setup_timer - set up the sleeping hrtimer.
> @@ -599,7 +642,10 @@ int futex_unqueue(struct futex_q *q)
>  	 */
>  	lock_ptr = READ_ONCE(q->lock_ptr);
>  	if (lock_ptr != NULL) {
> +		struct futex_hash_bucket *hb;
> +
>  		spin_lock(lock_ptr);
> +		hb = futex_hb_from_futex_q(q);
>  		/*
>  		 * q->lock_ptr can change between reading it and
>  		 * spin_lock(), causing us to take the wrong lock.  This
> @@ -622,6 +668,7 @@ int futex_unqueue(struct futex_q *q)
>  		BUG_ON(q->pi_state);
>  
>  		spin_unlock(lock_ptr);
> +		futex_hash_put(hb);

This is invoked from futex_wait_multiple() which means you are
are holding the reference count accross schedule(),

I'm not convinced that this is the right thing to do. Let me look at
your actual resize implementation...

>  		futex_q_unlock(hb);
> +		futex_hash_put(hb);

This pattern is there in a gazillion instances. Can't we have a single
function doing all of it?

Thanks,

        tglx