linux-kernel - [PATCH 2/3] cleanup: Introduce cond_no_free

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: <170905253897.2268463.13371523233762430828.stgit@dwillia2-xfh.jf.intel.com>
Date: Tue, 27 Feb 2024 08:48:59 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: torvalds@...ux-foundation.org, peterz@...radead.org,
 gregkh@...uxfoundation.org
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org
Subject: [PATCH 2/3] cleanup: Introduce cond_no_free_ptr()

The no_free_ptr() helper cancels automatic cleanup for cases where
assigning the pointer transfers ownership for freeing it. However, it
gets awkward to use when multiple allocations need to be cancelled in
response to one registration call. For example:

    1/ name = kasprintf(...);
    2/ res = kmalloc(...);
    3/ res->name = name;
    4/ rc = insert_resource(..., res);
    5/ if (rc) return rc;

no_free_ptr() cannot be used for 3 since insert_resource() does not
cleanup on failure. no_free_ptr() could be used at 4, but if
insert_resource() fails, the no_free_ptr() was premature. After 5 is
when it is known that it is safe to free @res and @name. However,
no_free_ptr() is awkward there as well because of __must_check().

The options are:
 * Just open code @res = NULL and @name = NULL, but that is a
   non-idiomatic way to use the cleanup helpers.
 * Introduce a no_free_ptr() variant that drops the __must_check, but
   that defeats the purpose of mandating that the caller understands
   that responsibility for freeing has been handed off.
 * Introduce a new helper that combines a condition check to supersede
   the __must_check of no_free_ptr()

So, per that last option, line 5/ from the example becomes:

    5/ cond_no_free_ptr(rc == 0, return rc, res, name);

..and that handles calling no_free_ptr() while also mandating the
negative condition be handled. It is inspired by scoped_cond_guard()
which also takes a statement for the negative condition case.

Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Signed-off-by: Dan Williams <dan.j.williams@...el.com>
---
 include/linux/cleanup.h |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 602afb85da34..a6d593a60611 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -77,6 +77,28 @@ const volatile void * __must_check_fn(const volatile void *val)

 #define return_ptr(p)	return no_free_ptr(p)

+#define __cond_no_free_ptrs(p) ({__auto_type __always_unused __ptr = no_free_ptr(p);})
+#define __cond_no_free_ptrs1(p, ...) __cond_no_free_ptrs(p)
+#define __cond_no_free_ptrs2(p, ...) \
+	__cond_no_free_ptrs(p), __cond_no_free_ptrs1(__VA_ARGS__)
+#define __cond_no_free_ptrs3(p, ...) \
+	__cond_no_free_ptrs(p), __cond_no_free_ptrs2(__VA_ARGS__)
+
+/*
+ * When an object is built up by an amalgamation of multiple allocations
+ * each of those need to be cleaned up on error, but there are occasions
+ * where once the object is registered all of those cleanups can be
+ * cancelled.  cond_no_free_ptr() arranges to call no_free_ptr() on all
+ * its arguments (up to 3) if @condition is true and runs @_fail
+ * otherwise (typically to return and trigger auto-cleanup).
+ */
+#define cond_no_free_ptr(condition, _fail, ...)                           \
+	if (condition) {                                                  \
+		CONCATENATE(__cond_no_free_ptrs, COUNT_ARGS(__VA_ARGS__)) \
+		(__VA_ARGS__);                                            \
+	} else {                                                          \
+		_fail;                                                    \
+	}

 /*
  * DEFINE_CLASS(name, type, exit, init, init_args...):