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

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