Skip to content

Commit

Permalink
kernel: Fix massive cpufreq stats memory leaks
Browse files Browse the repository at this point in the history
Every time _cpu_up() is called for a CPU, idle_thread_get() is called which
then re-initializes a CPU's idle thread that was already previously created
and cached in a global variable in smpboot.c. idle_thread_get() calls
init_idle() which then calls __sched_fork(). __sched_fork() is where
cpufreq_task_stats_init() is, and cpufreq_task_stats_init() allocates
512 bytes of memory to a pointer in the task struct.

Since idle_thread_get() reuses a task struct instance that was already
previously created, this means that every time it calls init_idle(),
cpufreq_task_stats_init() allocates 512 bytes again and overwrites the
existing 512-byte allocation that the idle thread already had.

This causes 512 bytes to be leaked every time a CPU is onlined. This is
significant when non-boot CPUs are enabled during resume from suspend; this
means that (NR_CPUS - 1) * 512 bytes are leaked every time the device exits
suspend (this turned out to be ~500 kiB leaked in 20 minutes with the
device left on a desk with the screen off).

In order to fix this, don't initialize cpufreq stats at all for the idle
threads. The cpufreq stats interface is intended to be used for tracking
userspace tasks, so we can safely remove it from the kernel's idle threads
without killing any functionality.

But that's not all!

Task structs can be freed outside of release_task(), which creates another
memory leak because a task struct can be freed without having its cpufreq
stats allocation freed. To fix this, free the cpufreq stats allocation at
the same time that task struct allocations are freed.

Signed-off-by: Sultan Alsawaf <sultanxda@gmail.com>
  • Loading branch information
kerneltoast authored and khusika committed Jun 15, 2018
1 parent 88f3d2f commit 15612b8
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 8 deletions.
4 changes: 0 additions & 4 deletions kernel/exit.c
Expand Up @@ -54,7 +54,6 @@
#include <linux/writeback.h>
#include <linux/shm.h>
#include <linux/kcov.h>
#include <linux/cpufreq.h>

#include <asm/uaccess.h>
#include <asm/unistd.h>
Expand Down Expand Up @@ -175,9 +174,6 @@ void release_task(struct task_struct *p)
{
struct task_struct *leader;
int zap_leader;
#ifdef CONFIG_CPU_FREQ_STAT
cpufreq_task_stats_exit(p);
#endif
repeat:
/* don't need to get the RCU readlock here - the process is dead and
* can't be modifying its own credentials. But shut RCU-lockdep up */
Expand Down
4 changes: 4 additions & 0 deletions kernel/fork.c
Expand Up @@ -76,6 +76,7 @@
#include <linux/aio.h>
#include <linux/compiler.h>
#include <linux/kcov.h>
#include <linux/cpufreq.h>

#include <asm/pgtable.h>
#include <asm/pgalloc.h>
Expand Down Expand Up @@ -216,6 +217,9 @@ static void account_kernel_stack(struct thread_info *ti, int account)

void free_task(struct task_struct *tsk)
{
#ifdef CONFIG_CPU_FREQ_STAT
cpufreq_task_stats_exit(tsk);
#endif
account_kernel_stack(tsk->stack, -1);
arch_release_thread_info(tsk->stack);
free_thread_info(tsk->stack);
Expand Down
8 changes: 4 additions & 4 deletions kernel/sched/core.c
Expand Up @@ -5308,10 +5308,6 @@ static void __sched_fork(unsigned long clone_flags, struct task_struct *p)
memset(&p->se.statistics, 0, sizeof(p->se.statistics));
#endif

#ifdef CONFIG_CPU_FREQ_STAT
cpufreq_task_stats_init(p);
#endif

RB_CLEAR_NODE(&p->dl.rb_node);
hrtimer_init(&p->dl.dl_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
__dl_clear_params(p);
Expand Down Expand Up @@ -5400,6 +5396,10 @@ int sched_fork(unsigned long clone_flags, struct task_struct *p)
unsigned long flags;
int cpu = get_cpu();

#ifdef CONFIG_CPU_FREQ_STAT
cpufreq_task_stats_init(p);
#endif

__sched_fork(clone_flags, p);
/*
* We mark the process as running here. This guarantees that
Expand Down

0 comments on commit 15612b8

Please sign in to comment.