linux-kernel - Re: [PATCH v5 06/14] futex: Add helper which include the put of a hb after end of operation.

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] [day] [month] [year] [list]

Message-ID: <20241217170739.AbsgKxuZ@linutronix.de>
Date: Tue, 17 Dec 2024 18:07:39 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org,
	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>
Subject: Re: [PATCH v5 06/14] futex: Add helper which include the put of a hb
 after end of operation.

On 2024-12-16 19:48:07 [+0100], Thomas Gleixner wrote:
> So something like this might be more to the point:
> 
>    futex: Prepare for reference counting of the process private hash
> 
>    To support runtime resizing of the process private hash, it's
>    required to add a reference count to the hash structure. The
>    reference count ensures that the hash cannot be resized or freed
>    while a task is operating on it.
> 
>    The reference count will be obtained within futex_hash() and dropped
>    once the hash bucket is unlocked and not longer required for the
>    particular operation (queue, unqueue, wakeup etc.).
> 
>    This is achieved by:
> 
>     - appending _put() to existing functions so it's clear that they
>       also put the hash reference and fixing up the usage sites
> 
>     - providing new helpers, which combine common operations (unlock,
>       put), and using them at the appropriate places
> 
>     - providing new helper for standalone reference counting
>       functionality and using them at places, where the unlock operation
>       needs to be separate.
>    
> Hmm?

much better.

> > -void futex_q_unlock(struct futex_hash_bucket *hb)
> > +void futex_q_unlock_put(struct futex_hash_bucket *hb)
> >  	__releases(&hb->lock)
> >  {
> >  	spin_unlock(&hb->lock);
> >  	futex_hb_waiters_dec(hb);
> > +	futex_hash_put(hb);
> 
> You missed a place to move the waiter_dec() before the unlock. Actually

This is fine because futex_hb_waiters_dec() happens before the reference
drop. However it is better to keep it symmetrical so I going to move it.

> this move should be in a separate preparatory patch, which does only
> that. It also needs a proper change log which explains why this done,
> why it is equivalent and not introducing a change in terms of ordering
> rules. This:
>
> > Move futex_hb_waiters_dec() before the reference drop, if needed
> > before the unlock.
> 
> does not really give any clue at all.
> 
> >  		if (unlikely(ret)) {
> > -			double_unlock_hb(hb1, hb2);
> >  			futex_hb_waiters_dec(hb2);
> > +			double_unlock_hb_put(hb1, hb2);
> 
> And having this move before the _put() change makes the latter a purely
> mechanical change which let's the reader/reviewer focus on the reference
> count rules and not be distracted by the waiter count changes.

Okay, moving this into its own patch.

> > -	/* futex_queue and wait for wakeup, timeout, or a signal. */
> > +	/* futex_queue_put and wait for wakeup, timeout, or a signal. */
> 
> When you fix up these comments, can you please use the fn() notation?

sure

> Thanks,
> 
>         tglx

Sebastian