| 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: <20250507072145.3614298-2-dan.j.williams@intel.com>
Date: Wed, 7 May 2025 00:21:39 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: <linux-cxl@...r.kernel.org>
CC: <linux-kernel@...r.kernel.org>, David Lechner <dlechner@...libre.com>,
Peter Zijlstra <peterz@...radead.org>, 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: [PATCH 1/7] cleanup: Introduce DEFINE_ACQUIRE() a CLASS() for conditional locking
The scoped_cond_guard() helper has two problems. First, with "convert to
cleanup" style patches, it results in messy diffs that re-indent large
swaths of code. Second, it swallows return values from interruptible /
killable locks, i.e. the return value of those is not always -EINTR.
The last attempt at improving this situation, if_not_guard() [1], was
reverted with a suggestion:
"I really think the basic issue is that 'cond_guard' itself is a pretty
broken concept. It simply doesn't work very well in the C syntax."
However, the observation is that just exposing the CLASS() to callers
solves: the safety concern (compiler enforced "expected expression"
checks), conveying the conditional acquisition state of the lock, and
avoiding re-indentation caused by scoped_cond_guard(). See the commit below
for the analysis of the revert.
Commit b4d83c8323b0 ("headers/cleanup.h: Remove the if_not_guard() facility")
The DEFINE_ACQUIRE() macro wraps a lock type like 'struct mutex' into a
'struct mutex_acquire' type that can only be acquired and automatically
released by scope-based helpers. E.g.:
[scoped_]guard(mutex_acquire)(...)
CLASS(mutex_intr_acquire, ...)
CLASS(mutex_try_acquire, ...)
Use DEFINE_ACQUIRE to create the new classes above and use
mutex_intr_acquire in the CXL subsystem to demonstrate the conversion.
Link: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com [1]
Cc: David Lechner <dlechner@...libre.com>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Ingo Molnar <mingo@...nel.org>
Cc: "Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>
Cc: Davidlohr Bueso <dave@...olabs.net>
Cc: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: Dave Jiang <dave.jiang@...el.com>
Cc: Alison Schofield <alison.schofield@...el.com>
Cc: Vishal Verma <vishal.l.verma@...el.com>
Cc: Ira Weiny <ira.weiny@...el.com>
Signed-off-by: Dan Williams <dan.j.williams@...el.com>
---
drivers/cxl/core/mbox.c | 9 +++---
drivers/cxl/cxlmem.h | 2 +-
include/linux/cleanup.h | 62 +++++++++++++++++++++++++++++++++++++++++
include/linux/mutex.h | 24 ++++++++++++++++
4 files changed, 91 insertions(+), 6 deletions(-)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d72764056ce6..cec9dfb22567 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1394,9 +1394,9 @@ 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)
- return rc;
+ CLASS(mutex_intr_acquire, lock)(&mds->poison.lock);
+ if (IS_ERR(lock))
+ return PTR_ERR(lock);
po = mds->poison.list_out;
pi.offset = cpu_to_le64(offset);
@@ -1430,7 +1430,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
}
} while (po->flags & CXL_POISON_FLAG_MORE);
- mutex_unlock(&mds->poison.lock);
return rc;
}
EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison, "CXL");
@@ -1466,7 +1465,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
return rc;
}
- mutex_init(&mds->poison.lock);
+ mutex_acquire_init(&mds->poison.lock);
return 0;
}
EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, "CXL");
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 3ec6b906371b..9b4ab5d1a7c4 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -257,7 +257,7 @@ struct cxl_poison_state {
u32 max_errors;
DECLARE_BITMAP(enabled_cmds, CXL_POISON_ENABLED_MAX);
struct cxl_mbox_poison_out *list_out;
- struct mutex lock; /* Protect reads of poison list */
+ struct mutex_acquire lock; /* Protect reads of poison list */
};
/*
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 7e57047e1564..926b95daa4b5 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -424,5 +424,67 @@ __DEFINE_LOCK_GUARD_0(_name, _lock)
static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
{ return class_##_name##_lock_ptr(_T); }
+/*
+ * DEFINE_ACQUIRE(acquire_class_name, lock_type, unlock, cond_lock):
+ * Define a CLASS() that instantiates and acquires a conditional lock
+ * within an existing scope. In contrast to DEFINE_GUARD[_COND](), which
+ * hides the variable tracking the lock scope, CLASS(@acquire_class_name,
+ * @lock) instantiates @lock as either an ERR_PTR() or a cookie that drops
+ * the lock when it goes out of scope. An "_acquire" suffix is appended to
+ * @lock_type to provide type-safety against mixing explicit and implicit
+ * (scope-based) cleanup.
+ *
+ * Ex.
+ *
+ * DEFINE_ACQUIRE(mutex_intr_acquire, mutex, mutex_unlock,
+ * mutex_lock_interruptible)
+ *
+ * int interruptible_operation(...)
+ * {
+ * ...
+ * CLASS(mutex_intr_acquire, lock)(&obj->lock);
+ * if (IS_ERR(lock))
+ * return PTR_ERR(lock);
+ * ...
+ * } <= obj->lock dropped here.
+ *
+ * Attempts to perform:
+ *
+ * mutex_unlock(&obj->lock);
+ *
+ * ...fail because obj->lock is a 'struct mutex_acquire' not 'struct mutex'
+ * instance.
+ *
+ * Also, attempts to use the CLASS() conditionally require the ambiguous
+ * scope to be clarified (compiler enforced):
+ *
+ * if (...)
+ * CLASS(mutex_intr_acquire, lock)(&obj->lock); // <-- "error: expected expression"
+ * if (IS_ERR(lock))
+ * return PTR_ERR(lock);
+ *
+ * vs:
+ *
+ * if (...) {
+ * CLASS(mutex_intr_acquire, lock)(&obj->lock);
+ * if (IS_ERR(lock))
+ * return PTR_ERR(lock);
+ * } // <-- lock released here
+ */
+#define DEFINE_ACQUIRE(_name, _locktype, _unlock, _cond_lock) \
+ DEFINE_CLASS(_name, struct _locktype##_acquire *, \
+ if (!IS_ERR_OR_NULL(_T)) _unlock(&_T->_locktype), ({ \
+ struct _locktype##_acquire *lock_result; \
+ int ret = _cond_lock(&to_lock->_locktype); \
+ \
+ if (ret) \
+ lock_result = ERR_PTR(ret); \
+ else \
+ lock_result = \
+ (struct _locktype##_acquire \
+ *)&to_lock->_locktype; \
+ lock_result; \
+ }), \
+ struct _locktype##_acquire *to_lock)
#endif /* _LINUX_CLEANUP_H */
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2143d05116be..283111f43b0f 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -64,6 +64,8 @@ do { \
__mutex_init((mutex), #mutex, &__key); \
} while (0)
+#define mutex_acquire_init(lock) mutex_init(&(lock)->mutex)
+
/**
* mutex_init_with_key - initialize a mutex with a given lockdep key
* @mutex: the mutex to be initialized
@@ -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 */
--
2.49.0
Powered by blists - more mailing lists