2009-12-02 08:41:30

by J. R. Okajima

Subject: [PATCH 0/2] fix for kmemleak_erase() (Re: Q, slab, kmemleak_erase() and redzone?)

Pekka Enberg:
> No, you are absolutely correct. Can you please send an updated patch to
> Catalin that adds a comment on top of the cpu_cache_get() call that
> explains why we need it there?

J. R. Okajima (2):
slab, kmemleak, minor, stop calling kmemleak_erase() unconditionally
slab, kmemleak, bugfix, pass the correct pointer to kmemleak_erase()

mm/slab.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)

2009-12-02 08:21:48

by J. R. Okajima

Subject: [PATCH 1/2] slab, kmemleak, minor, stop calling kmemleak_erase() unconditionally

When the gotten object is NULL (probably due to ENOMEM),
kmemleak_erase() is unnecessary here, It just sets NULL to where already
is NULL.
Add a condition.

Signed-off-by: J. R. Okajima <[email protected]>
---
mm/slab.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 7dfa481..4e61449 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3109,7 +3109,8 @@ static inline void *____cache_alloc(struct kmem_cache *cachep, gfp_t flags)
* per-CPU caches is leaked, we need to make sure kmemleak doesn't
* treat the array pointers as a reference to the object.
*/
- kmemleak_erase(&ac->entry[ac->avail]);
+ if (objp)
+ kmemleak_erase(&ac->entry[ac->avail]);
return objp;
}

--
1.6.1.284.g5dc13

2009-12-02 08:04:22

by J. R. Okajima

Subject: [PATCH 2/2] slab, kmemleak, bugfix, pass the correct pointer to kmemleak_erase()

In ____cache_alloc(), the variable 'ac' may be changed after
cache_alloc_refill() and the following kmemleak_erase() may get an
incorrect pointer.
Update 'ac' after cache_alloc_refill() unconditionally.
cf. http://marc.info/?l=linux-kernel&m=125873373124187&w=2
and its thread.

Cc: Pekka Enberg <[email protected]>
Cc: Catalin Marinas <[email protected]>

Signed-off-by: J. R. Okajima <[email protected]>
---
mm/slab.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 4e61449..66e9047 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3103,6 +3103,11 @@ static inline void *____cache_alloc(struct kmem_cache *cachep, gfp_t flags)
} else {
STATS_INC_ALLOCMISS(cachep);
objp = cache_alloc_refill(cachep, flags);
+ /*
+ * the 'ac' may be updated by cache_alloc_refill(),
+ * and kmemleak_erase() requires its correct value.
+ */
+ ac = cpu_cache_get(cachep);
}
/*
* To avoid a false negative, if an object that is in one of the
--
1.6.1.284.g5dc13

2009-12-02 08:06:58

by Pekka Enberg

Subject: Re: [PATCH 0/2] fix for kmemleak_erase() (Re: Q, slab, kmemleak_erase() and redzone?)

J. R. Okajima kirjoitti:
> Pekka Enberg:
>> No, you are absolutely correct. Can you please send an updated patch to
>> Catalin that adds a comment on top of the cpu_cache_get() call that
>> explains why we need it there?
>
> J. R. Okajima (2):
> slab, kmemleak, minor, stop calling kmemleak_erase() unconditionally
> slab, kmemleak, bugfix, pass the correct pointer to kmemleak_erase()
>
> mm/slab.c | 8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletions(-)

Looks good to me. I'll pick these up in slab.git. Thanks!

2009-12-02 09:54:24

by Catalin Marinas

Subject: Re: [PATCH 1/2] slab, kmemleak, minor, stop calling kmemleak_erase() unconditionally

"J. R. Okajima" <[email protected]> wrote:
> When the gotten object is NULL (probably due to ENOMEM),
> kmemleak_erase() is unnecessary here, It just sets NULL to where already
> is NULL.
> Add a condition.
>
> Signed-off-by: J. R. Okajima <[email protected]>
> ---
> mm/slab.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 7dfa481..4e61449 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3109,7 +3109,8 @@ static inline void *____cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> * per-CPU caches is leaked, we need to make sure kmemleak doesn't
> * treat the array pointers as a reference to the object.
> */
> - kmemleak_erase(&ac->entry[ac->avail]);
> + if (objp)
> + kmemleak_erase(&ac->entry[ac->avail]);
> return objp;
> }

Acked-by: Catalin Marinas <[email protected]>

--
Catalin

2009-12-02 09:55:01

by Catalin Marinas

Subject: Re: [PATCH 2/2] slab, kmemleak, bugfix, pass the correct pointer to kmemleak_erase()

"J. R. Okajima" <[email protected]> wrote:
> In ____cache_alloc(), the variable 'ac' may be changed after
> cache_alloc_refill() and the following kmemleak_erase() may get an
> incorrect pointer.
> Update 'ac' after cache_alloc_refill() unconditionally.
> cf. http://marc.info/?l=linux-kernel&m=125873373124187&w=2
> and its thread.
>
> Cc: Pekka Enberg <[email protected]>
> Cc: Catalin Marinas <[email protected]>
>
> Signed-off-by: J. R. Okajima <[email protected]>
> ---
> mm/slab.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 4e61449..66e9047 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3103,6 +3103,11 @@ static inline void *____cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> } else {
> STATS_INC_ALLOCMISS(cachep);
> objp = cache_alloc_refill(cachep, flags);
> + /*
> + * the 'ac' may be updated by cache_alloc_refill(),
> + * and kmemleak_erase() requires its correct value.
> + */
> + ac = cpu_cache_get(cachep);
> }
> /*
> * To avoid a false negative, if an object that is in one of the

Acked-by: Catalin Marinas <[email protected]>

Thanks.

--
Catalin

2009-12-06 08:33:08

by Pekka Enberg

Subject: Re: [PATCH 1/2] slab, kmemleak, minor, stop calling kmemleak_erase() unconditionally

Catalin Marinas wrote:
> "J. R. Okajima" <[email protected]> wrote:
>> When the gotten object is NULL (probably due to ENOMEM),
>> kmemleak_erase() is unnecessary here, It just sets NULL to where already
>> is NULL.
>> Add a condition.
>>
>> Signed-off-by: J. R. Okajima <[email protected]>
>> ---
>> mm/slab.c | 3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/slab.c b/mm/slab.c
>> index 7dfa481..4e61449 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -3109,7 +3109,8 @@ static inline void *____cache_alloc(struct kmem_cache *cachep, gfp_t flags)
>> * per-CPU caches is leaked, we need to make sure kmemleak doesn't
>> * treat the array pointers as a reference to the object.
>> */
>> - kmemleak_erase(&ac->entry[ac->avail]);
>> + if (objp)
>> + kmemleak_erase(&ac->entry[ac->avail]);
>> return objp;
>> }
>
> Acked-by: Catalin Marinas <[email protected]>

Applied, thanks!

2009-12-06 08:33:31

by Pekka Enberg

Subject: Re: [PATCH 2/2] slab, kmemleak, bugfix, pass the correct pointer to kmemleak_erase()

Catalin Marinas wrote:
> "J. R. Okajima" <[email protected]> wrote:
>> In ____cache_alloc(), the variable 'ac' may be changed after
>> cache_alloc_refill() and the following kmemleak_erase() may get an
>> incorrect pointer.
>> Update 'ac' after cache_alloc_refill() unconditionally.
>> cf. http://marc.info/?l=linux-kernel&m=125873373124187&w=2
>> and its thread.
>>
>> Cc: Pekka Enberg <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>>
>> Signed-off-by: J. R. Okajima <[email protected]>
>> ---
>> mm/slab.c | 5 +++++
>> 1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/slab.c b/mm/slab.c
>> index 4e61449..66e9047 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -3103,6 +3103,11 @@ static inline void *____cache_alloc(struct kmem_cache *cachep, gfp_t flags)
>> } else {
>> STATS_INC_ALLOCMISS(cachep);
>> objp = cache_alloc_refill(cachep, flags);
>> + /*
>> + * the 'ac' may be updated by cache_alloc_refill(),
>> + * and kmemleak_erase() requires its correct value.
>> + */
>> + ac = cpu_cache_get(cachep);
>> }
>> /*
>> * To avoid a false negative, if an object that is in one of the
>
> Acked-by: Catalin Marinas <[email protected]>
>
> Thanks.

Applied, thanks!