linux-kernel - Re: [PATCH v8 08/15] futex: Prepare for reference counting of the process private hash 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250205075405.7bX5uyf3@linutronix.de>
Date: Wed, 5 Feb 2025 08:54:05 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Peter Zijlstra <peterz@...radead.org>
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>,
	Thomas Gleixner <tglx@...utronix.de>,
	Valentin Schneider <vschneid@...hat.com>,
	Waiman Long <longman@...hat.com>
Subject: Re: [PATCH v8 08/15] futex: Prepare for reference counting of the
 process private hash end of operation.

On 2025-02-04 10:49:22 [+0100], Peter Zijlstra wrote:
> On Mon, Feb 03, 2025 at 02:59:28PM +0100, Sebastian Andrzej Siewior wrote:
> 
> > @@ -555,11 +558,12 @@ struct futex_hash_bucket *futex_q_lock(struct futex_q *q)
> >  	return hb;
> >  }
> >  
> > -void futex_q_unlock(struct futex_hash_bucket *hb)
> > +void futex_q_unlock_put(struct futex_hash_bucket *hb)
> >  	__releases(&hb->lock)
> >  {
> >  	futex_hb_waiters_dec(hb);
> >  	spin_unlock(&hb->lock);
> > +	futex_hash_put(hb);
> >  }
> 
> Here you don't

unlock + put.

> > @@ -288,23 +289,29 @@ extern void __futex_unqueue(struct futex_q *q);
> >  extern void __futex_queue(struct futex_q *q, struct futex_hash_bucket *hb);
> >  extern int futex_unqueue(struct futex_q *q);
> >  
> > +static inline void futex_hb_unlock_put(struct futex_hash_bucket *hb)
> > +{
> > +	spin_unlock(&hb->lock);
> > +	futex_hash_put(hb);
> > +}
> > +
> >  /**
> > - * futex_queue() - Enqueue the futex_q on the futex_hash_bucket
> > + * futex_queue_put() - Enqueue the futex_q on the futex_hash_bucket
> >   * @q:	The futex_q to enqueue
> >   * @hb:	The destination hash bucket
> >   *
> > - * The hb->lock must be held by the caller, and is released here. A call to
> > - * futex_queue() is typically paired with exactly one call to futex_unqueue().  The
> > - * exceptions involve the PI related operations, which may use futex_unqueue_pi()
> > - * or nothing if the unqueue is done as part of the wake process and the unqueue
> > - * state is implicit in the state of woken task (see futex_wait_requeue_pi() for
> > - * an example).
> > + * The hb->lock must be held by the caller, and is released here and the reference
> > + * on the hb is dropped. A call to futex_queue_put() is typically paired with
> > + * exactly one call to futex_unqueue(). The exceptions involve the PI related
> > + * operations, which may use futex_unqueue_pi() or nothing if the unqueue is
> > + * done as part of the wake process and the unqueue state is implicit in the
> > + * state of woken task (see futex_wait_requeue_pi() for an example).
> >   */
> > -static inline void futex_queue(struct futex_q *q, struct futex_hash_bucket *hb)
> > +static inline void futex_queue_put(struct futex_q *q, struct futex_hash_bucket *hb)
> >  	__releases(&hb->lock)
> >  {
> >  	__futex_queue(q, hb);
> > -	spin_unlock(&hb->lock);
> > +	futex_hb_unlock_put(hb);
> >  }
> 
> And here you do.

unlock + put. What do I don't do?

> > @@ -380,11 +387,13 @@ double_lock_hb(struct futex_hash_bucket *hb1, struct futex_hash_bucket *hb2)
> >  }
> >  
> >  static inline void
> > -double_unlock_hb(struct futex_hash_bucket *hb1, struct futex_hash_bucket *hb2)
> > +double_unlock_hb_put(struct futex_hash_bucket *hb1, struct futex_hash_bucket *hb2)
> >  {
> >  	spin_unlock(&hb1->lock);
> >  	if (hb1 != hb2)
> >  		spin_unlock(&hb2->lock);
> > +	futex_hash_put(hb1);
> > +	futex_hash_put(hb2);
> >  }
> >  
> 
> This seems horribly inconsistent and makes my head hurt. Where are the
> matching gets for double_lock_hb() ?

There are in futex_hash().

Sebastian