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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Christian Casteyde <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: Vegard Nossum <[email protected]>
---
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
On Mon, Feb 8, 2010 at 1:16 PM, Catalin Marinas <[email protected]> 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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Christian Casteyde <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Cc: Vegard Nossum <[email protected]>
Acked-by: Pekka Enberg <[email protected]>
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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Mon, 2010-02-08 at 12:54 +0000, Pekka Enberg wrote:
> On Mon, Feb 8, 2010 at 1:16 PM, Catalin Marinas <[email protected]> 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 <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Christian Casteyde <[email protected]>
> > Cc: Pekka Enberg <[email protected]>
> > Cc: Vegard Nossum <[email protected]>
>
> Acked-by: Pekka Enberg <[email protected]>
>
> 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
On Mon, Feb 8, 2010 at 2:58 PM, Catalin Marinas <[email protected]> wrote:
> On Mon, 2010-02-08 at 12:54 +0000, Pekka Enberg wrote:
>> On Mon, Feb 8, 2010 at 1:16 PM, Catalin Marinas <[email protected]> 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 <[email protected]>
>> > Cc: Andrew Morton <[email protected]>
>> > Cc: Christian Casteyde <[email protected]>
>> > Cc: Pekka Enberg <[email protected]>
>> > Cc: Vegard Nossum <[email protected]>
>>
>> Acked-by: Pekka Enberg <[email protected]>
>>
>> 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
On Mon, Feb 8, 2010 at 12:16 PM, Catalin Marinas
<[email protected]> 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
On Mon, 2010-02-08 at 14:38 +0000, roel kluin wrote:
> On Mon, Feb 8, 2010 at 12:16 PM, Catalin Marinas <[email protected]> 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
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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Christian Casteyde <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Cc: Vegard Nossum <[email protected]>
Applied to slab.git.