linux-kernel - Re: [PATCH v12 14/21] futex: Allow to resize the private local hash

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: <20250516104921.sy7Z-oy_@linutronix.de>
Date: Fri, 16 May 2025 12:49:21 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: André Almeida <andrealmeid@...lia.com>
Cc: linux-kernel@...r.kernel.org, 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>,
	Thomas Gleixner <tglx@...utronix.de>,
	Valentin Schneider <vschneid@...hat.com>,
	Waiman Long <longman@...hat.com>
Subject: Re: [PATCH v12 14/21] futex: Allow to resize the private local hash

On 2025-05-08 17:32:24 [-0300], André Almeida wrote:
> > @@ -1290,26 +1466,102 @@ static int futex_hash_allocate(unsigned int hash_slots, bool custom)
> >   	for (i = 0; i < hash_slots; i++)
> >   		futex_hash_bucket_init(&fph->queues[i], fph);
> > -	mm->futex_phash = fph;
> 
> If (hash_slots == 0), do we still need to do all of this work bellow? I
> thought that using the global hash would allow to skip this.

Not sure what you mean by below. We need to create a smaller struct
futex_private_hash and initialize it. We also need to move all current
futex waiters, which might be on the private hash that is going away,
over to the global hash. So yes, all this is needed.

> > +	if (custom) {
> > +		/*
> > +		 * Only let prctl() wait / retry; don't unduly delay clone().
> > +		 */
> > +again:
> > +		wait_var_event(mm, futex_pivot_pending(mm));
> > +	}
> > +
> > +	scoped_guard(mutex, &mm->futex_hash_lock) {
> > +		struct futex_private_hash *free __free(kvfree) = NULL;
> > +		struct futex_private_hash *cur, *new;
> > +
> > +		cur = rcu_dereference_protected(mm->futex_phash,
> > +						lockdep_is_held(&mm->futex_hash_lock));
> > +		new = mm->futex_phash_new;
> > +		mm->futex_phash_new = NULL;
> > +
> > +		if (fph) {
> > +			if (cur && !new) {
> > +				/*
> > +				 * If we have an existing hash, but do not yet have
> > +				 * allocated a replacement hash, drop the initial
> > +				 * reference on the existing hash.
> > +				 */
> > +				futex_private_hash_put(cur);
> > +			}
> > +
> > +			if (new) {
> > +				/*
> > +				 * Two updates raced; throw out the lesser one.
> > +				 */
> > +				if (futex_hash_less(new, fph)) {
> > +					free = new;
> > +					new = fph;
> > +				} else {
> > +					free = fph;
> > +				}
> > +			} else {
> > +				new = fph;
> > +			}
> > +			fph = NULL;
> > +		}
> > +
> > +		if (new) {
> > +			/*
> > +			 * Will set mm->futex_phash_new on failure;
> > +			 * futex_private_hash_get() will try again.
> > +			 */
> > +			if (!__futex_pivot_hash(mm, new) && custom)
> > +				goto again;
> 
> Is it safe to use a goto inside a scoped_guard(){}?

We jump outside of the scoped_guard() and while testing I've been
looking at the assembly and gcc did the right thing. So I would say why
not. The alternative would be to do manual lock/unlock and think about
the unlock just before the goto statement so this looks "easier".

Sebastian