[PATCH v3] profiling: don't free prof_cpu_mask on init failure

[PATCH v3] profiling: don't free prof_cpu_mask on init failure

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

When profiling is enabled at runtime via /sys/kernel/profiling,
profile_init() allocates prof_cpu_mask then attempts to allocate
prof_buffer. If all prof_buffer allocations fail, the error path
frees prof_cpu_mask but leaves prof_on set.

Since profile_tick() runs from timer interrupt context and reads
prof_cpu_mask on every tick, it can access the freed cpumask between
the free and the next reboot.

Don't free prof_cpu_mask in the error path. The cpumask allocation
already succeeded and is small; keeping it on this rare failure path
avoids the stale access without additional synchronization.

Note: mainline removed prof_cpu_mask entirely in commit 7c51f7bbf057
("profiling: remove prof_cpu_mask"). This is a minimal fix for stable
trees where the variable is still present.

Fixes: c309b917cab55 ("cpumask: convert kernel/profile.c")
Cc: stable@vger.kernel.org
Suggested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
Changes in v3:
- Added comment explaining deliberate leak (Andrew Morton)
- Corrected Fixes tag from 22b8ce94708f to c309b917cab55 (Tetsuo Handa)
- Added stable-only context in commit message

Changes in v2:
- Remove the free_cpumask_var() call instead of adding a prof_on
  guard in profile_tick(), which still raced with the free (Tetsuo Handa)
 kernel/profile.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/profile.c b/kernel/profile.c
index 984f819b701c9..dcb65a2501558 100644
--- a/kernel/profile.c
+++ b/kernel/profile.c
@@ -123,7 +123,14 @@ int __ref profile_init(void)
 	if (prof_buffer)
 		return 0;
 
-	free_cpumask_var(prof_cpu_mask);
+	/*
+	 * Do not free prof_cpu_mask here. profile_tick() accesses it from
+	 * timer interrupt context without synchronization, so freeing it
+	 * while prof_on is set leads to a stale read. The cpumask is small
+	 * and this error path is rare, so the leak is harmless.
+	 * This code was removed entirely by commit 7c51f7bbf057
+	 * ("profiling: remove prof_cpu_mask") in v6.11.
+	 */
 	return -ENOMEM;
 }
 
-- 
2.47.3

Re: [PATCH v3] profiling: don't free prof_cpu_mask on init failure

[Bradley Morgan]

Ummmm, not sure you should replace it with that long of a comment,

What you did was meant to cleanup useless crap. Let's not add more 
useless crap by adding 7 new lines. When it could be explained shorter
IMHO, Here's a example, I mean, its not the best, but it works:

/* No need to free prof_cpu_mask()
 * anymore, the error path is rare
 * and the leak is harmless IMHO 
 */


feel free to bikeshed. As per usual.


Also, here's some stable "rules" I tell most people

- Please please please add why this is bad in the commit description (you did that! Yay)

- A splat, please. It helps the stable folks not think that this is another "bug" (Do this if possible)


If I had to say "Bradley, just review this patch for what it does for f*ck
sake", I'd say this patch is useful (sorry for my french!)

So.. here is the tag to add for V4,

Reviewed-by: Bradley Morgan <include@grrlz.net>


Thanks!