Re: [PATCH 2/2] slub: Add method to verify memory is not deleted.

[David Rientjes]

On Mon, 27 Jun 2011, greearb@xxxxxxxxxxxxxxx wrote:

> From: Ben Greear <greearb@xxxxxxxxxxxxxxx>
> 
> This is for tracking down suspect memory usage.
> 

Several things wrong with this:

 - I have no idea where patch 1/2 is.

 - the subject line is ambiguous, when you say memory is "deleted," I 
   thought at first you were talking about hot-removed, but it seems like 
   you're talking about "freed."

 - what "suspect memory usage" are you adding functionality to catch and
   how are you doing it?

 - you didn't cc the slab maintainers, Pekka Enberg and Christoph Lameter
   (I added them).

> Signed-off-by: Ben Greear <greearb@xxxxxxxxxxxxxxx>
> ---
> :100644 100644 ad4dd1c... 35a13d2... M	include/linux/slab.h
> :100644 100644 14d0135... b3d7680... M	mm/slub.c
>  include/linux/slab.h |    9 +++++++++
>  mm/slub.c            |   34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index ad4dd1c..35a13d2 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -302,6 +302,15 @@ extern void *__kmalloc_node_track_caller(size_t, gfp_t, int, unsigned long);
>  
>  #endif /* CONFIG_NUMA */
>  
> +/** Calling this on deleted objects will print some
> + * SLUB debugging information.
> + */

Ambiguous as to what it will be printing and violation of the comment 
style used in the kernel (see Documentation/CodingStyle).

> +#if defined(CONFIG_SLUB) && defined(CONFIG_SLUB_DEBUG)
> +extern bool verify_mem_not_deleted(const void *x);
> +#else
> +#define verify_mem_not_deleted(x)

This will surely break if anybody isn't using slub or CONFIG_SLUB_DEBUG 
and it testing verify_mem_not_deleted().  You probably want

	static inline bool verify_mem_not_deleted(const void *x)
	{
		return false;
	}

> +#endif
> +
>  /*
>   * Shortcuts
>   */
> diff --git a/mm/slub.c b/mm/slub.c
> index 14d0135..b3d7680 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2953,6 +2953,40 @@ size_t ksize(const void *object)
>  }
>  EXPORT_SYMBOL(ksize);
>  
> +#if defined(CONFIG_SLUB_DEBUG)

#ifdef CONFIG_SLUB_DEBUG

> +bool verify_mem_not_deleted(const void *x)
> +{
> +	struct page *page;
> +	void *object = (void *)x;
> +	struct kmem_cache *s;
> +	unsigned long flags;
> +	bool rv = false;
> +
> +	if (unlikely(ZERO_OR_NULL_PTR(x)))
> +		false;
> +

Did you even compile-test this?

> +	local_irq_save(flags);
> +
> +	page = virt_to_head_page(x);
> +	if (unlikely(!PageSlab(page))) {
> +		BUG_ON(!PageCompound(page));

Why is there a BUG_ON() here if we didn't pass a pointer to a slab object?

> +		goto out_unlock;
> +	}
> +
> +	s = page->slab;
> +
> +	if (on_freelist(s, page, object)) {
> +		object_err(s, page, object, "Object is on fee-list");

s/fee/free/

> +		goto out_unlock;
> +	}
> +	rv = true;
> +out_unlock:
> +	local_irq_restore(flags);
> +	return rv;
> +}
> +EXPORT_SYMBOL(verify_mem_not_deleted);
> +#endif
> +
--
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/