| 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
| ||
|
Message-ID: <681ea7d5ea04b_2a2bb100cf@dwillia2-mobl4.notmuch>
Date: Fri, 9 May 2025 18:11:49 -0700
From: <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:
> On Thu, May 08, 2025 at 10:04:32PM -0700, Dan Williams wrote:
> > Peter Zijlstra wrote:
> > [..]
> > > > 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/
> >
> > That looks lovely.
> >
> > > But the compiler needs a little more work go grok C :-)
> >
> > Ok, here is a last shot that incorporates all the feedback:
> >
> > 1/ Conceptually no need for a new CLASS() save for the fact that
> > __guard_ptr() returns NULL on failure, not an ERR_PTR().
> >
> > 2/ The rename trick is not true type safety, especially if it leads to
> > parallel universe of primitives, but it is a useful trick.
> >
> > 3/ "IS_ERR(__guard_ptr(mutex_intr)(lock))" is a mouthful, would be nice
> > to have something more succint while maintaining some safety.
> >
> > That leads me to a scheme like the following:
> >
> > DEFINE_GUARD_ERR(mutex, _intr, mutex_lock_interruptible(_T))
> > ...
> > ACQUIRE(mutex_intr, lock)(&obj->lock);
> > if (IS_ERR(lock))
> > return PTR_ERR(lock);
>
> Urgh.. can you live with something like this?
>
> ---
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d72764056ce6..6b0ca400b393 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1394,8 +1394,8 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
> int nr_records = 0;
> int rc;
>
> - rc = mutex_lock_interruptible(&mds->poison.lock);
> - if (rc)
> + ACQUIRE(mutex_intr, lock)(&mds->poison.poison_lock);
> + if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
> return rc;
Yes, I can live with that, and I like the compactness of the resulting
cleanup.h macros better than my attempt.
Although, how about this small ergonomic fixup for more symmetry between
ACQUIRE() and ACQUIRE_ERR()?
---
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 6b0ca400b393..e5d2171c9a48 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1395,7 +1395,7 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
int rc;
ACQUIRE(mutex_intr, lock)(&mds->poison.poison_lock);
- if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
+ if ((rc = ACQUIRE_ERR(mutex_intr, lock)))
return rc;
po = mds->poison.list_out;
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 17d4655a6b73..b379ff445179 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -335,7 +342,7 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
CLASS(_name, _var)
#define ACQUIRE_ERR(_name, _var) \
- ({ long _rc = PTR_ERR(__guard_ptr(_name)(_var)); \
+ ({ long _rc = PTR_ERR(__guard_ptr(_name)(&(_var))); \
if (!_rc) _rc = -EBUSY; \
if (!IS_ERR_VALUE(_rc)) _rc = 0; \
_rc; })
---
Also, I needed the following to get this to compile:
---
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 17d4655a6b73..052bbad6ac68 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -305,8 +305,15 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
__DEFINE_CLASS_IS_CONDITIONAL(_name, true); \
__DEFINE_GUARD_LOCK_PTR(_name, _T)
+/*
+ * For guard with a potential percpu lock, the address space needs to be
+ * cast away.
+ */
+#define IS_ERR_OR_NULL_ANY(x) \
+IS_ERR_OR_NULL((const void *)(__force const unsigned long)(x))
+
#define DEFINE_GUARD(_name, _type, _lock, _unlock) \
- DEFINE_CLASS(_name, _type, if (!IS_ERR_OR_NULL(_T)) { _unlock; }, ({ _lock; _T; }), _type _T); \
+ DEFINE_CLASS(_name, _type, if (!IS_ERR_OR_NULL_ANY(_T)) { _unlock; }, ({ _lock; _T; }), _type _T); \
DEFINE_CLASS_IS_GUARD(_name)
#define DEFINE_GUARD_COND_4(_name, _ext, _lock, _cond) \
@@ -401,7 +408,7 @@ typedef struct { \
\
static inline void class_##_name##_destructor(class_##_name##_t *_T) \
{ \
- if (!IS_ERR_OR_NULL(_T->lock)) { _unlock; } \
+ if (!IS_ERR_OR_NULL_ANY(_T->lock)) { _unlock; } \
} \
\
__DEFINE_GUARD_LOCK_PTR(_name, &_T->lock)
Powered by blists - more mailing lists