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

[Pekka Enberg]

Hi Catalin,

Few minor nits below.

On Wed, Dec 10, 2008 at 8:26 PM, Catalin Marinas
<catalin.marinas@xxxxxxx> wrote:
> +static void put_object(struct memleak_object *object)
> +{
> +       if (!atomic_dec_and_test(&object->use_count))
> +               return;
> +
> +       /* should only get here after delete_object was called */
> +       BUG_ON(object->flags & OBJECT_ALLOCATED);

This could be

    if (WARN_ON(object->flags & OBJECT_ALLOCATED))
            return;

> +static void create_object(unsigned long ptr, size_t size, int min_count,
> +                         gfp_t gfp)
> +{
> +       unsigned long flags;
> +       struct memleak_object *object;
> +       struct prio_tree_node *node;
> +       struct stack_trace trace;
> +
> +       object = kmem_cache_alloc(object_cache, gfp);
> +       if (!object)
> +               memleak_panic("kmemleak: Cannot allocate a memleak_object "
> +                             "structure\n");

Don't you want to exit early here if object == NULL?

> +
> +       INIT_LIST_HEAD(&object->object_list);
> +       INIT_LIST_HEAD(&object->gray_list);
> +       INIT_HLIST_HEAD(&object->area_list);
> +       spin_lock_init(&object->lock);
> +       atomic_set(&object->use_count, 1);
> +       object->flags = OBJECT_ALLOCATED;
> +       object->pointer = ptr;
> +       object->size = size;
> +       object->min_count = min_count;
> +       object->count = -1;                     /* no color initially */
> +       object->jiffies = jiffies;
> +
> +       /* task information */
> +       if (in_irq()) {
> +               object->pid = 0;
> +               strncpy(object->comm, "hardirq", TASK_COMM_LEN);
> +       } else if (in_softirq()) {
> +               object->pid = 0;
> +               strncpy(object->comm, "softirq", TASK_COMM_LEN);
> +       } else {
> +               object->pid = current->pid;
> +               get_task_comm(object->comm, current);
> +       }
> +
> +       /* kernel backtrace */
> +       trace.max_entries = MAX_TRACE;
> +       trace.nr_entries = 0;
> +       trace.entries = object->trace;
> +       trace.skip = 1;
> +       save_stack_trace(&trace);
> +       object->trace_len = trace.nr_entries;
> +
> +       INIT_PRIO_TREE_NODE(&object->tree_node);
> +       object->tree_node.start = ptr;
> +       object->tree_node.last = ptr + size - 1;
> +
> +       write_lock_irqsave(&memleak_lock, flags);
> +       min_addr = min(min_addr, ptr);
> +       max_addr = max(max_addr, ptr + size);
> +       node = prio_tree_insert(&object_tree_root, &object->tree_node);
> +       /*
> +        * The code calling the kernel does not yet have the pointer to the
> +        * memory block to be able to free it.  However, we still hold the
> +        * memleak_lock here in case parts of the kernel started freeing
> +        * random memory blocks.
> +        */
> +       if (node != &object->tree_node) {
> +               unsigned long flags;
> +
> +               pr_warning("kmemleak: Existing pointer\n");
> +               dump_stack();

How come you don't dump_stack() or even WARN_ON() unconditionally in
kmemleak_panic() which is called bit later so you can remove this kind
of ad hoc logging?

> +
> +               object = lookup_object(ptr, 1);
> +               spin_lock_irqsave(&object->lock, flags);
> +               dump_object_info(object);
> +               spin_unlock_irqrestore(&object->lock, flags);
> +
> +               memleak_panic("kmemleak: Cannot insert 0x%lx into the object "
> +                             "search tree\n", ptr);


> +       }
> +       list_add_tail_rcu(&object->object_list, &object_list);
> +       write_unlock_irqrestore(&memleak_lock, flags);
> +}
> +
> +/*
> + * Remove the metadata (struct memleak_object) for a memory block from the
> + * object_list and object_tree_root and decrement its use_count.
> + */
> +static void delete_object(unsigned long ptr)
> +{
> +       unsigned long flags;
> +       struct memleak_object *object;
> +
> +       write_lock_irqsave(&memleak_lock, flags);
> +       object = lookup_object(ptr, 0);
> +       if (!object) {
> +               pr_warning("kmemleak: Freeing unknown object at 0x%08lx\n",
> +                          ptr);
> +               dump_stack();

Hmm, dump_stack() is called in quite a few places. Might make sense to
add a memleak_report() function that does this in an uniform way.

> +               write_unlock_irqrestore(&memleak_lock, flags);
> +               return;
> +       }
> +       prio_tree_remove(&object_tree_root, &object->tree_node);
> +       list_del_rcu(&object->object_list);
> +       write_unlock_irqrestore(&memleak_lock, flags);
> +
> +       BUG_ON(!(object->flags & OBJECT_ALLOCATED));
> +       BUG_ON(atomic_read(&object->use_count) < 1);

These could be converted to WARN_ON() calls, I think?

> +
> +       /*
> +        * Locking here also ensures that the corresponding memory block
> +        * cannot be freed when it is being scanned.
> +        */
> +       spin_lock_irqsave(&object->lock, flags);
> +       object->flags &= ~OBJECT_ALLOCATED;
> +#ifdef REPORT_ORPHAN_FREEING
> +       if (color_white(object)) {
> +               pr_warning("kmemleak: Freeing orphan object 0x%08lx\n", ptr);
> +               dump_stack();
> +               dump_object_info(object);
> +       }
> +#endif
> +       spin_unlock_irqrestore(&object->lock, flags);
> +       put_object(object);
> +}
> +
> +/*
> + * Make a object permanently as gray-colored so that it can no longer be
> + * reported as a leak. This is used in general to mark a false positive.
> + */
> +static void make_gray_object(unsigned long ptr)
> +{
> +       unsigned long flags;
> +       struct memleak_object *object;
> +
> +       object = find_and_get_object(ptr, 0);
> +       if (!object) {
> +               dump_stack();
> +               memleak_panic("kmemleak: Graying unknown object at 0x%08lx\n",
> +                             ptr);

Early return here?

> +       }
> +
> +       spin_lock_irqsave(&object->lock, flags);
> +       object->min_count = 0;
> +       spin_unlock_irqrestore(&object->lock, flags);
> +       put_object(object);
> +}
> +
> +/*
> + * Mark the object as black-colored so that it is ignored from scans and
> + * reporting.
> + */
> +static void make_black_object(unsigned long ptr)
> +{
> +       unsigned long flags;
> +       struct memleak_object *object;
> +
> +       object = find_and_get_object(ptr, 0);
> +       if (!object) {
> +               dump_stack();
> +               memleak_panic("kmemleak: Blacking unknown object at 0x%08lx\n",
> +                             ptr);

Ditto.

> +       }
> +
> +       spin_lock_irqsave(&object->lock, flags);
> +       object->min_count = -1;
> +       spin_unlock_irqrestore(&object->lock, flags);
> +       put_object(object);
> +}
> +
> +/*
> + * Add a scanning area to the object. If at least one such area is added,
> + * kmemleak will only scan these ranges rather than the whole memory block.
> + */
> +static void add_scan_area(unsigned long ptr, unsigned long offset,
> +                         size_t length, gfp_t gfp)
> +{
> +       unsigned long flags;
> +       struct memleak_object *object;
> +       struct memleak_scan_area *area;
> +
> +       object = find_and_get_object(ptr, 0);
> +       if (!object) {
> +               dump_stack();
> +               memleak_panic("kmemleak: Adding scan area to unknown "
> +                             "object at 0x%08lx\n", ptr);

Ditto.

> +       }
> +
> +       area = kmem_cache_alloc(scan_area_cache, gfp);
> +       if (!area)
> +               memleak_panic("kmemleak: Cannot allocate a scan area\n");
> +
> +       spin_lock_irqsave(&object->lock, flags);
> +       if (offset + length > object->size) {
> +               dump_stack();
> +               dump_object_info(object);
> +               memleak_panic("kmemleak: Scan area larger than object "
> +                             "0x%08lx\n", ptr);
> +       }
> +
> +       INIT_HLIST_NODE(&area->node);
> +       area->offset = offset;
> +       area->length = length;
> +
> +       hlist_add_head(&area->node, &object->area_list);
> +       spin_unlock_irqrestore(&object->lock, flags);
> +       put_object(object);
> +}
> +
> +/*
> + * Log an early memleak_* call to the early_log buffer. These calls will be
> + * processed later once kmemleak is fully initialized.
> + */
> +static void __init log_early(int op_type, const void *ptr, size_t size,
> +                            int min_count,
> +                            unsigned long offset, size_t length)
> +{
> +       unsigned long flags;
> +       struct early_log *log;
> +
> +       if (crt_early_log >= ARRAY_SIZE(early_log))
> +               memleak_panic("kmemleak: Early log buffer exceeded\n");

Here as well.

> +
> +       /*
> +        * There is no need for locking since the kernel is still in UP mode
> +        * at this stage. Disabling the IRQs is enough.
> +        */
> +       local_irq_save(flags);
> +       log = &early_log[crt_early_log];
> +       log->op_type = op_type;
> +       log->ptr = ptr;
> +       log->size = size;
> +       log->min_count = min_count;
> +       log->offset = offset;
> +       log->length = length;
> +       crt_early_log++;
> +       local_irq_restore(flags);
> +}
--
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/