netdev - [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

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: <20240926134347.19371-1-przemyslaw.kitszel@intel.com>
Date: Thu, 26 Sep 2024 15:41:38 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: linux-kernel@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>
Cc: amadeuszx.slawinski@...ux.intel.com,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	nex.sw.ncis.osdt.itp.upstreaming@...el.com,
	netdev@...r.kernel.org,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Andy Shevchenko <andriy.shevchenko@...el.com>
Subject: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

Simply enable one to write code like:

int foo(struct my_drv *adapter)
{
	scoped_guard(spinlock, &adapter->some_spinlock)
		return adapter->spinlock_protected_var;
}

Current scoped_guard() implementation does not support that,
due to compiler complaining:
error: control reaches end of non-void function [-Werror=return-type]

One could argue that for such use case it would be better to use
guard(spinlock)(&adapter->some_spinlock), I disagree. I could also say
that coding with my proposed locking style is also very pleasant, as I'm
doing so for a few weeks already.

Technical stuff about the change:
scoped_guard() macro uses common idiom of using "for" statement to declare
a scoped variable. Unfortunately, current logic is too hard for compiler
diagnostics to be sure that there is exactly one loop step; fix that.

To make any loop so trivial that there is no above warning, it must not
depend on any variable to tell if there are more steps. There is no
obvious solution for that in C, but one could use the compound statement
expression with "goto" jumping past the "loop", effectively leaving only
the subscope part of the loop semantics.

More impl details:
one more level of macro indirection is now needed to avoid duplicating
label names;
I didn't spot any other place that is using
	if (0) past_the_loop:; else for (...; 1; ({goto past_the_loop}))
idiom, so it's not packed for reuse what makes actual macros code cleaner.

NAKed-by: Andy Shevchenko <andriy.shevchenko@...el.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
---
Andy believes that this change is completely wrong C, and wants me
to keep the following 4 corncers attached (I either don't agree
or they are irrelevant), but here we go:
1. wrong usage of scoped_guard().
   In the described cases the guard() needs to be used.
2. the code like:
	int foo(...)
	{
		my_macro(...)
			return X;
	}
   without return 0; (which is a known dead code) is counter intuitive
   from the C language perspective.
3. [about netdev not liking guard()]
   I do not buy "too hard" when it's too easy to get a preprocessed *.i
   file if needed for any diagnosis which makes things quite clear.
   Moreover, once done the developer will much easier understands how this
   "magic" works (there is no rocket science, but yes, the initial
   threshold probably a bit higher than just pure C).
4. Besides that (if you was following the minmax discussion in LKML) the
   macro expansion may be problematic and lead to the unbelievable huge .i
   files that compiles dozens of seconds on modern CPUs (I experienced
   myself that with AtomISP driver which drove the above mentioned minmax
   discussion).
   [Przemek - nested scoped_guard() usage expands linearly]
---
 include/linux/cleanup.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index d9e613803df1..6b568a8a7f9c 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
-#define scoped_guard(_name, args...)					\
-	for (CLASS(_name, scope)(args),					\
-	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
+#define scoped_guard(_name, args...)	\
+	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
+
+#define __scoped_guard_labeled(_label, _name, args...)	\
+	if (0)						\
+		_label: ;				\
+	else						\
+		for (CLASS(_name, scope)(args);		\
+		     __guard_ptr(_name)(&scope), 1;	\
+		     ({ goto _label; }))
 
 #define scoped_cond_guard(_name, _fail, args...) \
 	for (CLASS(_name, scope)(args), \

base-commit: 151ac45348afc5b56baa584c7cd4876addf461ff
-- 
2.46.0