Re: [PATCH 01/14] kmemleak: Add the base support

From: Andrew Morton
Date: Mon Dec 29 2008 - 19:23:53 EST


On Fri, 19 Dec 2008 18:13:02 +0000
Catalin Marinas <catalin.marinas@xxxxxxx> wrote:

> This patch adds the base support for the kernel memory leak
> detector. It traces the memory allocation/freeing in a way similar to
> the Boehm's conservative garbage collector, the difference being that
> the unreferenced objects are not freed but only shown in
> /sys/kernel/debug/kmemleak. Enabling this feature introduces an
> overhead to memory allocations.
>
>
> ...
>
> --- a/init/main.c
> +++ b/init/main.c
> @@ -56,6 +56,7 @@
> #include <linux/debug_locks.h>
> #include <linux/debugobjects.h>
> #include <linux/lockdep.h>
> +#include <linux/kmemleak.h>
> #include <linux/pid_namespace.h>
> #include <linux/device.h>
> #include <linux/kthread.h>
> @@ -653,6 +654,8 @@ asmlinkage void __init start_kernel(void)
> enable_debug_pagealloc();
> cpu_hotplug_init();
> kmem_cache_init();
> + prio_tree_init();
> + kmemleak_init();

prio_tree_init() can be moved waaaay early, so we might as well do that
now, rather than just moving it a little bit.

> debug_objects_mem_init();
> idr_init_cache();
> setup_per_cpu_pageset();
> @@ -662,7 +665,6 @@ asmlinkage void __init start_kernel(void)
> calibrate_delay();
> pidmap_init();
> pgtable_cache_init();
> - prio_tree_init();
> anon_vma_init();
> #ifdef CONFIG_X86
> if (efi_enabled)
>
> ...
>
> +#define print_helper(seq, x...) do { \
> + if (seq) \
> + seq_printf(seq, x); \
> + else \
> + pr_info(x); \
> +} while (0)

grumblemutter. Evaluates `seq' more than once.

> +static void print_unreferenced(struct seq_file *seq,
> + struct kmemleak_object *object)
> +{
> + char namebuf[KSYM_NAME_LEN + 1] = "";
> + char *modname;
> + unsigned long symsize;
> + int i;
> +
> + print_helper(seq, "unreferenced object 0x%08lx (size %zu):\n",
> + object->pointer, object->size);
> + print_helper(seq, " comm \"%s\", pid %d, jiffies %lu\n",
> + object->comm, object->pid, object->jiffies);
> + print_helper(seq, " backtrace:\n");
> +
> + for (i = 0; i < object->trace_len; i++) {
> + unsigned long trace = object->trace[i];
> + unsigned long offset = 0;
> +
> + kallsyms_lookup(trace, &symsize, &offset, &modname, namebuf);
> + print_helper(seq, " [<%08lx>] %s\n", trace, namebuf);

Can this use the %p magic?

> + }
> +}
> +
>
> ...
>
> +static void scan_yield(void)
> +{
> + might_sleep();
> +
> + if (time_is_before_eq_jiffies(next_scan_yield)) {
> + schedule();
> + next_scan_yield = jiffies + jiffies_scan_yield;
> + }

I bet you could use __ratelimit() here. Although that probably won't
clarify anything, and it's slower ;)

> +}
> +
> +/*
> + * Memory scanning is a long process and it needs to be interruptable. This
> + * function checks whether such interrupt condition occured.
> + */
> +static int scan_should_stop(void)
> +{
> + if (!atomic_read(&kmemleak_enabled))
> + return 1;
> + /*
> + * This function may be called from either process or kthread context,
> + * hence the need to check for both stop conditions.
> + */
> + if ((current->mm && signal_pending(current)) ||
> + (!current->mm && kthread_should_stop()))
> + return 1;
> + return 0;
> +}

if (current->mm)
return signal_pending(current);
else
return kthread_should_stop();

nicer, no?

>
> ...
>
> +/*
> + * Stop the automatic memory scanning thread. This function must be called
> + * with the kmemleak_mutex held.
> + */
> +void stop_scan_thread(void)
> +{
> + if (scan_thread) {
> + kthread_stop(scan_thread);
> + scan_thread = NULL;
> + }
> +}

so... why do we need a kernel thread?

We could have (for the sake of argument) a sys_kmemleak_scan() which
does a single scan then returns. Or something like that. That way,
userspace directly gets to set the scanning frequency, thread priority,
etc.

>
> ...
>
> +static ssize_t kmemleak_write(struct file *file, const char __user *user_buf,
> + size_t size, loff_t *ppos)
> +{
> + char buf[64];
> + int buf_size;
> +
> + if (!atomic_read(&kmemleak_enabled))
> + return -EBUSY;
> +
> + buf_size = min(size, (sizeof(buf) - 1));
> + if (copy_from_user(buf, user_buf, buf_size))
> + return -EFAULT;
> + buf[buf_size] = 0;

maybe strncpy_from_user()?

> + if (strncmp(buf, "off", 3) == 0)
> + kmemleak_disable();
> + else if (strncmp(buf, "stack=on", 8) == 0)
> + kmemleak_stack_scan = 1;
> + else if (strncmp(buf, "stack=off", 9) == 0)
> + kmemleak_stack_scan = 0;
> + else if (strncmp(buf, "scan=on", 7) == 0)
> + start_scan_thread();
> + else if (strncmp(buf, "scan=off", 8) == 0)
> + stop_scan_thread();
> + else if (strncmp(buf, "scan=", 5) == 0) {
> + unsigned long secs;
> + int err;
> +
> + err = strict_strtoul(buf + 5, 0, &secs);
> + if (err < 0)
> + return err;
> + stop_scan_thread();
> + if (secs) {
> + jiffies_scan_wait = msecs_to_jiffies(secs * 1000);
> + start_scan_thread();
> + }
> + } else
> + return -EINVAL;
> +
> + /* ignore the rest of the buffer, only one command at a time */
> + *ppos += size;
> + return size;
> +}
> +
>
> ...
>
> +static void kmemleak_cleanup(void)
> +{
> + struct task_struct *cleanup_thread;
> +
> + cleanup_thread = kthread_run(kmemleak_cleanup_thread, NULL,
> + "kmemleak-cleanup");

#define TASK_COMM_LEN 16

So the above kernel thread will appear in `ps' output as "kmemleak-cleanu",
won't it?

> + if (IS_ERR(cleanup_thread))
> + pr_warning("kmemleak: Failed to create the clean-up thread\n");
> +}
> +
>
> ...
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/