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

[Catalin Marinas <catalin.marinas@arm.com>]

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