linux-kernel - [RFC PATCH v4] cleanup: Add cond

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>] [day] [month] [year] [list]

Message-ID: <20240201011052.474129-1-fabio.maria.de.francesco@linux.intel.com>
Date: Thu,  1 Feb 2024 02:09:31 +0100
From: "Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>
To: Peter Zijlstra <peterz@...radead.org>,
	dan.j.williams@...el.com,
	linux-kernel@...r.kernel.org,
	Ingo Molnar <mingo@...nel.org>,
	linux-cxl@...r.kernel.org
Cc: "Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>,
	Ira Weiny <ira.weiny@...el.com>
Subject: [RFC PATCH v4] cleanup: Add cond_guard() to conditional guards

Add cond_guard() to conditional guards.

cond_guard() is used for the _interruptible(), _killable(), and _try
versions of locks.

It stores a return value to a variable whose address is given to its
second argument, that is either '-1' on failure or '0' on success to
acquire a lock. The returned value can be checked to act accordingly
(e.g., to call printk() and return -EINTR in case of failure of an
_interruptible() variant). It stores both success and failure to
always overwrite any old value which is possibly contained in the
return variable.

As the other guards, it avoids to open code the release of the lock
after a goto to an 'out' label.

This remains an RFC because Dan suggested a slightly different syntax.
Furthermore, I've only been able to set the return variable by
dereferencing its address. "_ret = _err" fails my tests. Any hint on why
it doesn't work for me is welcome.

The changes to the CXL code are provided only to show the use of this
macro. If consensus is gathered on this macro, the cleanup of
show_targetN() will be submitted later in a separate patch.

Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Dan Williams <dan.j.williams@...el.com>
Suggested-by: Ira Weiny <ira.weiny@...el.com>
Signed-off-by: Fabio M. De Francesco <fabio.maria.de.francesco@...ux.intel.com>
---

Changes from v1 and v2:
	Addressed Dan's comments (thanks).

Changes from v3:
	Fixed a grammar error in macro.

 drivers/cxl/core/region.c | 15 +++++----------
 include/linux/cleanup.h   | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 0f05692bfec3..560f25bdfd11 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -668,26 +668,21 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 	struct cxl_endpoint_decoder *cxled;
 	int rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
+	cond_guard(rwsem_read_intr, &rc, &cxl_region_rwsem);
 	if (rc)
-		return rc;
+		return -EINTR;
 
 	if (pos >= p->interleave_ways) {
 		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
 			p->interleave_ways);
-		rc = -ENXIO;
-		goto out;
+		return -ENXIO;
 	}
 
 	cxled = p->targets[pos];
 	if (!cxled)
-		rc = sysfs_emit(buf, "\n");
+		return sysfs_emit(buf, "\n");
 	else
-		rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
-out:
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+		return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
 }
 
 static int match_free_decoder(struct device *dev, void *data)
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..a72c52430665 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -134,6 +134,20 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *	an anonymous instance of the (guard) class, not recommended for
  *	conditional locks.
  *
+ * cond_guard(name, ret, args...):
+ * 	for conditional locks like mutex_trylock() or down_read_interruptible().
+ * 	'ret' is a pointer to a variable where this macro stores 0 on success
+ * 	and -1 on failure to acquire a lock.
+ *
+ * 	Example:
+ *
+ * 	int ret;
+ * 	cond_guard(rwsem_read_try, &ret, &sem);
+ * 	if (ret) {
+ * 		dev_dbg("down_read_trylock() failed to down 'sem')\n");
+ * 		return 0; // down_read_trylock() returns 0 on contention
+ * 	}
+ *
  * scoped_guard (name, args...) { }:
  *	similar to CLASS(name, scope)(args), except the variable (with the
  *	explicit name 'scope') is declard in a for-loop such that its scope is
@@ -165,6 +179,11 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
+#define cond_guard(_name, _ret, args...) \
+	CLASS(_name, scope)(args); \
+	if (!__guard_ptr(_name)(&scope)) *_ret = -1; \
+	else *_ret = 0
+
 #define scoped_guard(_name, args...)					\
 	for (CLASS(_name, scope)(args),					\
 	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
-- 
2.43.0