Re: [PATCH] kmemleak: Do not enable KMEMCHECK_PARTIAL_OK if DEBUG_KMEMLEAK

[Pekka Enberg <penberg@cs.helsinki.fi>]

Catalin Marinas wrote:
> Hi Pekka,
> 
> On Wed, 2010-01-27 at 06:30 +0000, Pekka Enberg wrote:
>> Catalin Marinas kirjoitti:
>>> diff --git a/lib/Kconfig.kmemcheck b/lib/Kconfig.kmemcheck
>>> index 846e039..80660e9 100644
>>> --- a/lib/Kconfig.kmemcheck
>>> +++ b/lib/Kconfig.kmemcheck
>>> @@ -75,7 +75,7 @@ config KMEMCHECK_SHADOW_COPY_SHIFT
>>>  config KMEMCHECK_PARTIAL_OK
>>>       bool "kmemcheck: allow partially uninitialized memory"
>>>       depends on KMEMCHECK
>>> -     default y
>>> +     default y if !DEBUG_KMEMLEAK
>>>       help
>>>         This option works around certain GCC optimizations that produce
>>>         32-bit reads from 16-bit variables where the upper 16 bits are
>>>
>> Disabling KMEMCHECK_PARTIAL_OK can cause other false positives so maybe
>> we should add a new function to kmemcheck for kmemleak that only reads
>> full intervals?
> 
> There are two uses of kmemcheck_is_obj_initialized() in kmemleak: (1)
> before reading a value to check for valid pointer (sizeof long) and (2)
> before computing a CRC sum.
> 
> (1) is fine since it only does this for sizeof(long) before reading the
> same size. (2) has an issues since kmemleak checks the size of an
> allocated memory block while crc32 reads individual u32's.
> 
> Is there a way in kmemcheck to mark a false positive?

IIRC, no. The false positives come from the code generated by GCC so 
we'd need to sprinkle annotations here and there.

> The alternative, assuming that CRC_LE_BITS is 8 (that's what it seems to
> be defined to), the crc32 computation uses u32 values and something like
> below should work, though slower (not yet tested):
> 
> 
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 5b069e4..dfd39ba 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -948,6 +948,25 @@ EXPORT_SYMBOL(kmemleak_no_scan);
>  /*
>   * Update an object's checksum and return true if it was modified.
>   */
> +#ifdef CONFIG_KMEMCHECK_PARTIAL_OK
> +static bool update_checksum(struct kmemleak_object *object)
> +{
> +	u32 crc = 0;
> +	unsigned long ptr;
> +
> +	for (ptr = ALIGN(object->pointer, 4);
> +	     ptr < ((object->pointer + object->size) & ~3); ptr += 4) {
> +		if (!kmemcheck_is_obj_initialized(ptr, 4))
> +			continue;
> +		crc = crc32(crc, (void *)ptr, 4);
> +	}
> +
> +	if (object->checksum == crc)
> +		return false;
> +	object->checksum = crc;
> +	return true;

I think it would be better to add a function to _kmemcheck_ that checks 
the full object regardless of CONFIG_KMEMCHECK_PARTIAL_OK and use it here.

> +}
> +#else	/* !CONFIG_KMEMCHECK_PARTIAL_OK */
>  static bool update_checksum(struct kmemleak_object *object)
>  {
>  	u32 old_csum = object->checksum;
> @@ -958,6 +977,7 @@ static bool update_checksum(struct kmemleak_object *object)
>  	object->checksum = crc32(0, (void *)object->pointer, object->size);
>  	return object->checksum != old_csum;
>  }
> +#endif	/* CONFIG_KMEMCHECK_PARTIAL_OK */
>  
>  /*
>   * Memory scanning is a long process and it needs to be interruptable. This
> 
>