[PATCH] kmemleak: Do not enable KMEMCHECK_PARTIAL_OK if DEBUG_KMEMLEAK

[PATCH] kmemleak: Do not enable KMEMCHECK_PARTIAL_OK if DEBUG_KMEMLEAK

[Catalin Marinas]

This is a fix for bug #14845 (bugzilla.kernel.org). The
update_checksum() function in mm/kmemleak.c calls
kmemcheck_is_obj_initialised() before scanning an object. When
KMEMCHECK_PARTIAL_OK is enabled, this function returns true. However,
the crc32_le() reads smaller intervals (32-bit) for which
kmemleak_is_obj_initialised() is may be false leading to a kmemcheck
warning.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christian Casteyde <casteyde.christian@free.fr>
Cc: Pekka Enberg <penberg@cs.helsinki.fi>
---
 lib/Kconfig.kmemcheck |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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

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

[Pekka Enberg]

Hi Catalin,

Catalin Marinas kirjoitti:
> This is a fix for bug #14845 (bugzilla.kernel.org). The
> update_checksum() function in mm/kmemleak.c calls
> kmemcheck_is_obj_initialised() before scanning an object. When
> KMEMCHECK_PARTIAL_OK is enabled, this function returns true. However,
> the crc32_le() reads smaller intervals (32-bit) for which
> kmemleak_is_obj_initialised() is may be false leading to a kmemcheck
> warning.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Christian Casteyde <casteyde.christian@free.fr>
> Cc: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
>  lib/Kconfig.kmemcheck |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> 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?

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

[Catalin Marinas]

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?

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;
+}
+#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


-- 
Catalin

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

[Pekka Enberg]

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
> 
> 

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

[Catalin Marinas]

On Wed, 2010-01-27 at 15:09 +0000, Pekka Enberg wrote:
> Catalin Marinas wrote:
> > 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.

IIRC, it's only kmemleak using kmemcheck_is_obj_initialized(), we could
convert this to check the full object. Something like below (again, not
tested yet but if you are ok with the idea I'll test it a bit more and
add a commit log):


diff --git a/arch/x86/mm/kmemcheck/kmemcheck.c b/arch/x86/mm/kmemcheck/kmemcheck.c
index 8cc1833..b3b531a 100644
--- a/arch/x86/mm/kmemcheck/kmemcheck.c
+++ b/arch/x86/mm/kmemcheck/kmemcheck.c
@@ -337,7 +337,7 @@ bool kmemcheck_is_obj_initialized(unsigned long addr, size_t size)
 	if (!shadow)
 		return true;
 
-	status = kmemcheck_shadow_test(shadow, size);
+	status = kmemcheck_shadow_test_all(shadow, size);
 
 	return status == KMEMCHECK_SHADOW_INITIALIZED;
 }
diff --git a/arch/x86/mm/kmemcheck/shadow.c b/arch/x86/mm/kmemcheck/shadow.c
index 3f66b82..16c6d9f 100644
--- a/arch/x86/mm/kmemcheck/shadow.c
+++ b/arch/x86/mm/kmemcheck/shadow.c
@@ -139,13 +139,25 @@ enum kmemcheck_shadow kmemcheck_shadow_test(void *shadow, unsigned int size)
 		if (x[i] == KMEMCHECK_SHADOW_INITIALIZED)
 			return x[i];
 	}
+
+	return x[0];
 #else
+	return kmemcheck_shadow_test_all(shadow, size);
+#endif
+}
+
+enum kmemcheck_shadow kmemcheck_shadow_test_all(void *shadow, unsigned int size)
+{
+	uint8_t *x;
+	unsigned int i;
+
+	x = shadow;
+
 	/* All bytes must be initialized. */
 	for (i = 0; i < size; ++i) {
 		if (x[i] != KMEMCHECK_SHADOW_INITIALIZED)
 			return x[i];
 	}
-#endif
 
 	return x[0];
 }
diff --git a/arch/x86/mm/kmemcheck/shadow.h b/arch/x86/mm/kmemcheck/shadow.h
index af46d9a..ff0b2f7 100644
--- a/arch/x86/mm/kmemcheck/shadow.h
+++ b/arch/x86/mm/kmemcheck/shadow.h
@@ -11,6 +11,8 @@ enum kmemcheck_shadow {
 void *kmemcheck_shadow_lookup(unsigned long address);
 
 enum kmemcheck_shadow kmemcheck_shadow_test(void *shadow, unsigned int size);
+enum kmemcheck_shadow kmemcheck_shadow_test_all(void *shadow,
+						unsigned int size);
 void kmemcheck_shadow_set(void *shadow, unsigned int size);
 
 #endif


-- 
Catalin

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

[Pekka Enberg]

Catalin Marinas wrote:
> On Wed, 2010-01-27 at 15:09 +0000, Pekka Enberg wrote:
>> Catalin Marinas wrote:
>>> 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.
> 
> IIRC, it's only kmemleak using kmemcheck_is_obj_initialized(), we could
> convert this to check the full object. Something like below (again, not
> tested yet but if you are ok with the idea I'll test it a bit more and
> add a commit log):

Looks good to me!

> 
> diff --git a/arch/x86/mm/kmemcheck/kmemcheck.c b/arch/x86/mm/kmemcheck/kmemcheck.c
> index 8cc1833..b3b531a 100644
> --- a/arch/x86/mm/kmemcheck/kmemcheck.c
> +++ b/arch/x86/mm/kmemcheck/kmemcheck.c
> @@ -337,7 +337,7 @@ bool kmemcheck_is_obj_initialized(unsigned long addr, size_t size)
>  	if (!shadow)
>  		return true;
>  
> -	status = kmemcheck_shadow_test(shadow, size);
> +	status = kmemcheck_shadow_test_all(shadow, size);
>  
>  	return status == KMEMCHECK_SHADOW_INITIALIZED;
>  }
> diff --git a/arch/x86/mm/kmemcheck/shadow.c b/arch/x86/mm/kmemcheck/shadow.c
> index 3f66b82..16c6d9f 100644
> --- a/arch/x86/mm/kmemcheck/shadow.c
> +++ b/arch/x86/mm/kmemcheck/shadow.c
> @@ -139,13 +139,25 @@ enum kmemcheck_shadow kmemcheck_shadow_test(void *shadow, unsigned int size)
>  		if (x[i] == KMEMCHECK_SHADOW_INITIALIZED)
>  			return x[i];
>  	}
> +
> +	return x[0];
>  #else
> +	return kmemcheck_shadow_test_all(shadow, size);
> +#endif
> +}
> +
> +enum kmemcheck_shadow kmemcheck_shadow_test_all(void *shadow, unsigned int size)
> +{
> +	uint8_t *x;
> +	unsigned int i;
> +
> +	x = shadow;
> +
>  	/* All bytes must be initialized. */
>  	for (i = 0; i < size; ++i) {
>  		if (x[i] != KMEMCHECK_SHADOW_INITIALIZED)
>  			return x[i];
>  	}
> -#endif
>  
>  	return x[0];
>  }
> diff --git a/arch/x86/mm/kmemcheck/shadow.h b/arch/x86/mm/kmemcheck/shadow.h
> index af46d9a..ff0b2f7 100644
> --- a/arch/x86/mm/kmemcheck/shadow.h
> +++ b/arch/x86/mm/kmemcheck/shadow.h
> @@ -11,6 +11,8 @@ enum kmemcheck_shadow {
>  void *kmemcheck_shadow_lookup(unsigned long address);
>  
>  enum kmemcheck_shadow kmemcheck_shadow_test(void *shadow, unsigned int size);
> +enum kmemcheck_shadow kmemcheck_shadow_test_all(void *shadow,
> +						unsigned int size);
>  void kmemcheck_shadow_set(void *shadow, unsigned int size);
>  
>  #endif
> 
>