linux-kernel - Re: [PATCH 1/7] cleanup: Introduce DEFINE_ACQUIRE() a CLASS() for conditional locking

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: <681bce2193f38_1229d6294c7@dwillia2-xfh.jf.intel.com.notmuch>
Date: Wed, 7 May 2025 14:18:25 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Peter Zijlstra <peterz@...radead.org>, 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

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.

> 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.

> 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.

> Notably, you're going to be running into trouble the moment you want to
> use your acquire stuff on things like raw_spin_trylock_irqsave(),
> because all that already wraps the return type, in order to hide the
> flags thing etc.

I think that is solvable, but only with a new DEFINE_LOCK_GUARD_1() that
knows that the @lock member of class_##name##_t needs to be cast to the
base lock type.