| 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: <20250502234818.1744432-1-dan.j.williams@intel.com>
Date: Fri, 2 May 2025 16:48:18 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: <torvalds@...ux-foundation.org>, <peterz@...radead.org>
CC: Dan Williams <dan.j.williams@...el.com>, David Lechner
<dlechner@...libre.com>, "Fabio M. De Francesco"
<fabio.maria.de.francesco@...ux.intel.com>, Ingo Molnar <mingo@...nel.org>,
<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>
Subject: [RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers for interruptible locks
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.
I wish people just gave up on it entirely rather than try to work
around that fundamental fact."
Take that to heart and abandon a "guard-like" statement. Instead, formalize
an off-the-cuff Linus proposal that was suggested along the way towards
scoped_cond_guard() [2].
"You should aim for a nice
struct rw_semaphore *struct rw_semaphore *exec_update_lock
__cleanup(release_exec_update_lock) = get_exec_update_lock(task);"
Introduce DEFINE_DROP(), to create a cleanup function for releasing locks,
and __drop() to pair with "acquire" helpers. Note, while this could use
DEFINE_FREE()/__free() directly, that leads to confusing a "resource
leak" semantic with "missing unlock". Also, note the name "drop" is a
second option. The first option of calling this semantic "release" collides
with the "__release()" macro already being taken by sparse.
Lastly, just manually define mutex_intr_acquire() and
rwsem_read_intr_acquire() helpers as it was not readily apparent how to
create a new DEFINE_ACQUIRE() macro to automate these definitions. Use the
helpers in at least one location to test out the proposal.
Link: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com [1]
Link: https://lore.kernel.org/all/CAHk-=whvOGL3aNhtps0YksGtzvaob_bvZpbaTcVEqGwNMxB6xg@mail.gmail.com [2]
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>
Signed-off-by: Dan Williams <dan.j.williams@...el.com>
---
drivers/cxl/core/mbox.c | 7 ++++---
drivers/cxl/core/memdev.c | 18 ++++++++----------
include/linux/cleanup.h | 7 +++++++
include/linux/mutex.h | 10 ++++++++++
include/linux/rwsem.h | 11 +++++++++++
5 files changed, 40 insertions(+), 13 deletions(-)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d72764056ce6..e3e851813c2a 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1394,9 +1394,10 @@ 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;
+ struct mutex *lock __drop(mutex_unlock) =
+ mutex_intr_acquire(&mds->poison.lock);
+ if (IS_ERR(lock))
+ return PTR_ERR(lock);
po = mds->poison.list_out;
pi.offset = cpu_to_le64(offset);
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index a16a5886d40a..9fbe83f868c6 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -231,15 +231,15 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
if (!port || !is_cxl_endpoint(port))
return -EINVAL;
- rc = down_read_interruptible(&cxl_region_rwsem);
- if (rc)
- return rc;
+ struct rw_semaphore *region_rwsem __drop(up_read) =
+ rwsem_read_intr_acquire(&cxl_region_rwsem);
+ if (IS_ERR(region_rwsem))
+ return PTR_ERR(region_rwsem);
- rc = down_read_interruptible(&cxl_dpa_rwsem);
- if (rc) {
- up_read(&cxl_region_rwsem);
- return rc;
- }
+ struct rw_semaphore *dpa_rwsem __drop(up_read) =
+ rwsem_read_intr_acquire(&cxl_dpa_rwsem);
+ if (IS_ERR(dpa_rwsem))
+ return PTR_ERR(dpa_rwsem);
if (cxl_num_decoders_committed(port) == 0) {
/* No regions mapped to this memdev */
@@ -248,8 +248,6 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
/* Regions mapped, collect poison by endpoint */
rc = cxl_get_poison_by_endpoint(port);
}
- up_read(&cxl_dpa_rwsem);
- up_read(&cxl_region_rwsem);
return rc;
}
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 7e57047e1564..3e96edb1b373 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -199,6 +199,13 @@
#define __free(_name) __cleanup(__free_##_name)
+/*
+ * For lock "acquire"/"drop" helpers the lock is not "freed", but the
+ * cleanup mechanics are identical
+ */
+#define DEFINE_DROP(_name, _type, _drop) DEFINE_FREE(_name, _type, _drop)
+#define __drop(_name) __free(_name)
+
#define __get_and_null(p, nullvalue) \
({ \
__auto_type __ptr = &(p); \
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2143d05116be..9fef4077604c 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -202,6 +202,16 @@ 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)
+DEFINE_DROP(mutex_unlock, struct mutex *, if (!IS_ERR_OR_NULL(_T)) mutex_unlock(_T))
+static inline __must_check struct mutex *mutex_intr_acquire(struct mutex *lock)
+{
+ int ret = mutex_lock_interruptible(lock);
+
+ if (ret)
+ return ERR_PTR(ret);
+ return lock;
+}
+
extern unsigned long mutex_get_owner(struct mutex *lock);
#endif /* __LINUX_MUTEX_H */
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index c8b543d428b0..d1c992b45625 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -242,6 +242,17 @@ DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T))
DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0)
+DEFINE_DROP(up_read, struct rw_semaphore *, if (!IS_ERR_OR_NULL(_T)) up_read(_T))
+static inline __must_check struct rw_semaphore *
+rwsem_read_intr_acquire(struct rw_semaphore *sem)
+{
+ int ret = down_read_interruptible(sem);
+
+ if (ret)
+ return ERR_PTR(ret);
+ return sem;
+}
+
DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))
base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e
--
2.49.0
Powered by blists - more mailing lists