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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250508110043.GG4439@noisy.programming.kicks-ass.net>
Date: Thu, 8 May 2025 13:00:43 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Dan Williams <dan.j.williams@...el.com>
Cc: linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
	David Lechner <dlechner@...libre.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	"Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	Jonathan Cameron <jonathan.cameron@...wei.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>,
	Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH 1/7] cleanup: Introduce DEFINE_ACQUIRE() a CLASS() for
 conditional locking

On Wed, May 07, 2025 at 02:18:25PM -0700, Dan Williams wrote:
> Peter Zijlstra wrote:
> [..]
> > > @@ -202,6 +204,28 @@ DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
> > >  DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
> > >  DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
> > >  
> > > +/* mutex type that only implements scope-based unlock */
> > > +struct mutex_acquire {
> > > +	/* private: */
> > > +	struct mutex mutex;
> > > +};
> > > +DEFINE_GUARD(mutex_acquire, struct mutex_acquire *, mutex_lock(&_T->mutex),
> > > +	     mutex_unlock(&_T->mutex))
> > > +DEFINE_GUARD_COND(mutex_acquire, _try, mutex_trylock(&_T->mutex))
> > > +DEFINE_GUARD_COND(mutex_acquire, _intr, mutex_lock_interruptible(&_T->mutex) == 0)
> > > +DEFINE_ACQUIRE(mutex_intr_acquire, mutex, mutex_unlock,
> > > +	       mutex_lock_interruptible)
> > > +
> > > +static inline int mutex_try_or_busy(struct mutex *lock)
> > > +{
> > > +	int ret[] = { -EBUSY, 0 };
> > > +
> > > +	return ret[mutex_trylock(lock)];
> > > +}
> > > +
> > > +DEFINE_ACQUIRE(mutex_try_acquire, mutex, mutex_unlock,
> > > +	       mutex_try_or_busy)
> > > +
> > >  extern unsigned long mutex_get_owner(struct mutex *lock);
> > >  
> > >  #endif /* __LINUX_MUTEX_H */
> > 
> > I'm terribly confused...
> 
> I suspect the disconnect is that this proposal adds safety where guard()
> does not today. That was driven by the mistake that Linus caught in the
> RFC [1]
> 
> 	at the same time I note that your patch is horribly broken. Look
> 	at your change to drivers/cxl/core/mbox.c: you made it use
> 	
> 	+       struct mutex *lock __drop(mutex_unlock) =
> 	+               mutex_intr_acquire(&mds->poison.lock);
> 	
> 	but then you didn't remove the existing unlocking, so that
> 	function still has
> 
> [1]: http://lore.kernel.org/CAHk-=wgRPDGvofj1PU=NemF6iFu308pFZ=w5P+sQyOMGd978fA@mail.gmail.com
> 
> I.e. in my haste I forgot to cleanup a straggling open-coded
> mutex_unlock(), but that is something the compiler warns about iff we
> switch to parallel primitive universe.

AFAICT all you've done is an effective rename. You can still write:

	mutex_unlock(&foo->lock.lock);

and the compiler will again happily do so.

> > What's wrong with:
> > 
> > 	CLASS(mutex_intr, lock)(&foo);
> > 	if (IS_ERR(__guard_ptr(mutex_intr)(lock)))
> > 		return __guard_ptr(mutex_intr)(lock);
> 
> __guard_ptr() returns NULL on error, not an ERR_PTR, but I get the gist.

(Yeah, I realized after sending, but didn't want to self-reply for
something minor like that :-)

> > I mean, yeah __guard_ptr(mutex_intr) doesn't really roll of the tongue,
> > but if that is the whole objection, surely we can try and fix that bit
> > instead of building an entire parallel set of primitives.
> 
> Yes, the "entire set of parallel primitives" was the least confident
> part of this proposal, but the more I look, that is a feature (albeit
> inelegant) not a bug.
> 
> Today one can write:
> 
>     guard(mutex_intr)(&lock);
>     ...
>     mutex_unlock(lock);
> 
> ...and the compiler does not tell you that the lock may not even be held
> upon return, nor that this is unlocking a lock that will also be
> unlocked when @lock goes out of scope.
> 
> The only type safety today is the BUILD_BUG_ON() in scoped_cond_guard()
> when passing in the wrong lock class.
> 
> So the proposal is, if you know what you are doing, or have a need to
> switch back and forth between scope-based and explicit unlock for a give
> lock, use the base primitives. If instead you want to fully convert to
> scope-based lock management (excise all explicit unlock() calls) *and*
> you want the compiler to validate the conversion, switch to the _acquire
> parallel universe.

As with all refactoring ever, the rename trick always works. But I don't
think that warrants building a parallel infrastructure just for that.

Specifically, it very much does not disallow calling mutex_unlock() on
your new member. So all you get is some compiler help during refactor,
and again, just rename the lock member already.

Also, if we ever actually get LLVM's Thread Safety Analysis working,
that will help us with all these problems:

  https://lore.kernel.org/all/20250304092417.2873893-1-elver@google.com/

But the compiler needs a little more work go grok C :-)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ