[PATCH 1/2] lkdtm: Make lkdtm_do_action() return to avoid tail call optimization

[PATCH 1/2] lkdtm: Make lkdtm_do_action() return to avoid tail call optimization

[Douglas Anderson]

The comments for lkdtm_do_action() explicitly call out that it
shouldn't be inlined because we want it to show up in stack
crawls. However, at least with some compilers / options it's still
vanishing due to tail call optimization. Let's add a return value to
the function to make it harder for the compiler to do tail call
optimization here.

Now that we have a return value, we can actually use it in the
callers, which is a minor improvement in the code.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/misc/lkdtm/core.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index 0772e4a4757e..5732fd59a227 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -153,12 +153,17 @@ static const struct crashtype *find_crashtype(const char *name)
 /*
  * This is forced noinline just so it distinctly shows up in the stackdump
  * which makes validation of expected lkdtm crashes easier.
+ *
+ * NOTE: having a valid return value helps prevent the compiler from doing
+ * tail call optimizations and taking this out of the stack trace.
  */
-static noinline void lkdtm_do_action(const struct crashtype *crashtype)
+static noinline int lkdtm_do_action(const struct crashtype *crashtype)
 {
 	if (WARN_ON(!crashtype || !crashtype->func))
-		return;
+		return -EINVAL;
 	crashtype->func();
+
+	return 0;
 }
 
 static int lkdtm_register_cpoint(struct crashpoint *crashpoint,
@@ -167,10 +172,8 @@ static int lkdtm_register_cpoint(struct crashpoint *crashpoint,
 	int ret;
 
 	/* If this doesn't have a symbol, just call immediately. */
-	if (!crashpoint->kprobe.symbol_name) {
-		lkdtm_do_action(crashtype);
-		return 0;
-	}
+	if (!crashpoint->kprobe.symbol_name)
+		return lkdtm_do_action(crashtype);
 
 	if (lkdtm_kprobe != NULL)
 		unregister_kprobe(lkdtm_kprobe);
@@ -216,7 +219,7 @@ static int lkdtm_kprobe_handler(struct kprobe *kp, struct pt_regs *regs)
 	spin_unlock_irqrestore(&crash_count_lock, flags);
 
 	if (do_it)
-		lkdtm_do_action(lkdtm_crashtype);
+		return lkdtm_do_action(lkdtm_crashtype);
 
 	return 0;
 }
@@ -303,6 +306,7 @@ static ssize_t direct_entry(struct file *f, const char __user *user_buf,
 {
 	const struct crashtype *crashtype;
 	char *buf;
+	int err;
 
 	if (count >= PAGE_SIZE)
 		return -EINVAL;
@@ -326,9 +330,11 @@ static ssize_t direct_entry(struct file *f, const char __user *user_buf,
 		return -EINVAL;
 
 	pr_info("Performing direct entry %s\n", crashtype->name);
-	lkdtm_do_action(crashtype);
+	err = lkdtm_do_action(crashtype);
 	*off += count;
 
+	if (err)
+		return err;
 	return count;
 }
 
-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH 2/2] lkdtm/bugs: Adjust lkdtm_HUNG_TASK() to avoid tail call optimization

[Douglas Anderson]

When testing with lkdtm_HUNG_TASK() and looking at the output, I
expected to see lkdtm_HUNG_TASK() in the stack crawl but it wasn't
there. Instead, the top function on at least some devices was
schedule() due to tail call optimization.

Let's do two things to help here:
1. We'll mark this as "__noreturn". On GCC at least this is documented
   to prevent tail call optimization. The docs [1] say "In order to
   preserve backtraces, GCC will never turn calls to noreturn
   functions into tail calls."
2. We'll add a BUG_ON(1) at the end which means that schedule() is no
   longer a tail call. Note that this is potentially important because
   if we _did_ end up returning from schedule() due to some weird
   issue then we'd potentially be violating the "noreturn" that we
   told the compiler about. BUG is the right thing to do here.

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/misc/lkdtm/bugs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
index b080eb2335eb..d1222d3eda2f 100644
--- a/drivers/misc/lkdtm/bugs.c
+++ b/drivers/misc/lkdtm/bugs.c
@@ -294,10 +294,11 @@ static void lkdtm_SPINLOCKUP(void)
 	__release(&lock_me_up);
 }
 
-static void lkdtm_HUNG_TASK(void)
+static void __noreturn lkdtm_HUNG_TASK(void)
 {
 	set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule();
+	BUG_ON(1);
 }
 
 static volatile unsigned int huge = INT_MAX - 2;
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH 1/2] lkdtm: Make lkdtm_do_action() return to avoid tail call optimization

[Kees Cook]

On Mon, 22 Jan 2024 16:49:34 -0800, Douglas Anderson wrote:
> The comments for lkdtm_do_action() explicitly call out that it
> shouldn't be inlined because we want it to show up in stack
> crawls. However, at least with some compilers / options it's still
> vanishing due to tail call optimization. Let's add a return value to
> the function to make it harder for the compiler to do tail call
> optimization here.
> 
> [...]

Applied to for-next/hardening, thanks!

[1/2] lkdtm: Make lkdtm_do_action() return to avoid tail call optimization
      https://git.kernel.org/kees/c/189a4edb774b
[2/2] lkdtm/bugs: Adjust lkdtm_HUNG_TASK() to avoid tail call optimization
      https://git.kernel.org/kees/c/edb6538da3df

Take care,

-- 
Kees Cook