[PATCH] kmemcheck: Test the full object in kmemcheck_is_obj_initialized()

[PATCH] kmemcheck: Test the full object in kmemcheck_is_obj_initialized()

[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() may be false leading to a kmemcheck
warning.

Note that kmemcheck_is_obj_initialized() is currently only used by
kmemleak before scanning a memory location.

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>
Cc: Vegard Nossum <vegardno@ifi.uio.no>
---
 arch/x86/mm/kmemcheck/kmemcheck.c |    2 +-
 arch/x86/mm/kmemcheck/shadow.c    |   16 ++++++++++++++--
 arch/x86/mm/kmemcheck/shadow.h    |    2 ++
 3 files changed, 17 insertions(+), 3 deletions(-)

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..aec1242 100644
--- a/arch/x86/mm/kmemcheck/shadow.c
+++ b/arch/x86/mm/kmemcheck/shadow.c
@@ -125,12 +125,12 @@ void kmemcheck_mark_initialized_pages(struct page *p, unsigned int n)
 
 enum kmemcheck_shadow kmemcheck_shadow_test(void *shadow, unsigned int size)
 {
+#ifdef CONFIG_KMEMCHECK_PARTIAL_OK
 	uint8_t *x;
 	unsigned int i;
 
 	x = shadow;
 
-#ifdef CONFIG_KMEMCHECK_PARTIAL_OK
 	/*
 	 * Make sure _some_ bytes are initialized. Gcc frequently generates
 	 * code to access neighboring bytes.
@@ -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

Re: [PATCH] kmemcheck: Test the full object in kmemcheck_is_obj_initialized()

[Pekka Enberg]

On Mon, Feb 8, 2010 at 1:16 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> 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() may be false leading to a kmemcheck
> warning.
>
> Note that kmemcheck_is_obj_initialized() is currently only used by
> kmemleak before scanning a memory location.
>
> 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>
> Cc: Vegard Nossum <vegardno@ifi.uio.no>

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

Andrew, Vegard has been rather quiet lately so do you mind picking up
this patch? Alternatively, I can pick it up in slab.git if you so
prefer.

> ---
>  arch/x86/mm/kmemcheck/kmemcheck.c |    2 +-
>  arch/x86/mm/kmemcheck/shadow.c    |   16 ++++++++++++++--
>  arch/x86/mm/kmemcheck/shadow.h    |    2 ++
>  3 files changed, 17 insertions(+), 3 deletions(-)
>
> 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..aec1242 100644
> --- a/arch/x86/mm/kmemcheck/shadow.c
> +++ b/arch/x86/mm/kmemcheck/shadow.c
> @@ -125,12 +125,12 @@ void kmemcheck_mark_initialized_pages(struct page *p, unsigned int n)
>
>  enum kmemcheck_shadow kmemcheck_shadow_test(void *shadow, unsigned int size)
>  {
> +#ifdef CONFIG_KMEMCHECK_PARTIAL_OK
>        uint8_t *x;
>        unsigned int i;
>
>        x = shadow;
>
> -#ifdef CONFIG_KMEMCHECK_PARTIAL_OK
>        /*
>         * Make sure _some_ bytes are initialized. Gcc frequently generates
>         * code to access neighboring bytes.
> @@ -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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] kmemcheck: Test the full object in kmemcheck_is_obj_initialized()

[Catalin Marinas]

On Mon, 2010-02-08 at 12:54 +0000, Pekka Enberg wrote:
> On Mon, Feb 8, 2010 at 1:16 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 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() may be false leading to a kmemcheck
> > warning.
> >
> > Note that kmemcheck_is_obj_initialized() is currently only used by
> > kmemleak before scanning a memory location.
> >
> > 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>
> > Cc: Vegard Nossum <vegardno@ifi.uio.no>
> 
> Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> Andrew, Vegard has been rather quiet lately so do you mind picking up
> this patch? Alternatively, I can pick it up in slab.git if you so
> prefer.

It's been pretty late indeed, sorry about that (I mostly work on ARM
hardware where kmemcheck isn't available and couldn't test the patch
earlier).

Anyway, IMHO it's not critical to be merged in 2.6.33 but it's up to
you.

Thanks.

-- 
Catalin

Re: [PATCH] kmemcheck: Test the full object in kmemcheck_is_obj_initialized()

[Pekka Enberg]

On Mon, Feb 8, 2010 at 2:58 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Mon, 2010-02-08 at 12:54 +0000, Pekka Enberg wrote:
>> On Mon, Feb 8, 2010 at 1:16 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> > 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() may be false leading to a kmemcheck
>> > warning.
>> >
>> > Note that kmemcheck_is_obj_initialized() is currently only used by
>> > kmemleak before scanning a memory location.
>> >
>> > 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>
>> > Cc: Vegard Nossum <vegardno@ifi.uio.no>
>>
>> Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>
>>
>> Andrew, Vegard has been rather quiet lately so do you mind picking up
>> this patch? Alternatively, I can pick it up in slab.git if you so
>> prefer.
>
> It's been pretty late indeed, sorry about that (I mostly work on ARM
> hardware where kmemcheck isn't available and couldn't test the patch
> earlier).
>
> Anyway, IMHO it's not critical to be merged in 2.6.33 but it's up to
> you.

I think it's definitely .34 material but probably should go to -stable.

                        Pekka

Re: [PATCH] kmemcheck: Test the full object in kmemcheck_is_obj_initialized()

[roel kluin]

On Mon, Feb 8, 2010 at 12:16 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> 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() may be false leading to a kmemcheck
> warning.
>
> Note that kmemcheck_is_obj_initialized() is currently only used by
> kmemleak before scanning a memory location.

>  enum kmemcheck_shadow kmemcheck_shadow_test(void *shadow, unsigned int size)
>  {
> +#ifdef CONFIG_KMEMCHECK_PARTIAL_OK
>        uint8_t *x;
>        unsigned int i;
>
>        x = shadow;
>
> -#ifdef CONFIG_KMEMCHECK_PARTIAL_OK
>        /*
>         * Make sure _some_ bytes are initialized. Gcc frequently generates
>         * code to access neighboring bytes.
> @@ -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];
>  }

Are we certain that size cannot be 0 in kmemcheck_shadow_test()
and kmemcheck_shadow_test_all() or other functions in
arch/x86/mm/kmemcheck/shadow.c with these unsigned
comparisons in loops?

Roel

Re: [PATCH] kmemcheck: Test the full object in kmemcheck_is_obj_initialized()

[Catalin Marinas]

On Mon, 2010-02-08 at 14:38 +0000, roel kluin wrote:
> On Mon, Feb 8, 2010 at 12:16 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > 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() may be false leading to a kmemcheck
> > warning.
> >
> > Note that kmemcheck_is_obj_initialized() is currently only used by
> > kmemleak before scanning a memory location.
> 
> >  enum kmemcheck_shadow kmemcheck_shadow_test(void *shadow, unsigned int size)
> >  {
> > +#ifdef CONFIG_KMEMCHECK_PARTIAL_OK
> >        uint8_t *x;
> >        unsigned int i;
> >
> >        x = shadow;
> >
> > -#ifdef CONFIG_KMEMCHECK_PARTIAL_OK
> >        /*
> >         * Make sure _some_ bytes are initialized. Gcc frequently generates
> >         * code to access neighboring bytes.
> > @@ -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];
> >  }
> 
> Are we certain that size cannot be 0 in kmemcheck_shadow_test()
> and kmemcheck_shadow_test_all() or other functions in
> arch/x86/mm/kmemcheck/shadow.c with these unsigned
> comparisons in loops?

At least in the kmemleak use-case, I don't think one can allocate a
zero-size object, so it should be safe. I can't tell about the other
cases but AFAICT, this function is called as a result of a memory access
which is always greater than 0.

Thanks.

-- 
Catalin

Re: [PATCH] kmemcheck: Test the full object in kmemcheck_is_obj_initialized()

[Pekka Enberg]

Catalin Marinas wrote:
> 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() may be false leading to a kmemcheck
> warning.
> 
> Note that kmemcheck_is_obj_initialized() is currently only used by
> kmemleak before scanning a memory location.
> 
> 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>
> Cc: Vegard Nossum <vegardno@ifi.uio.no>

Applied to slab.git.