[PATCH] lib/scatterlist: Hook sg_kmalloc into kmemleak

[PATCH] lib/scatterlist: Hook sg_kmalloc into kmemleak

[Chris Wilson]

kmemleak ignores page_alloc() and so believes the final sub-page
allocation using the plain kmalloc is decoupled and lost. This leads to
lots of false-positives with code that uses scatterlists.

The options seem to be either to tell kmemleak that the kmalloc is not
leaked or to notify kmemleak of the page allocations. The danger of the
first approach is that we may hide a real leak, so choose the latter
approach (of which I am not sure of the downsides).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 lib/scatterlist.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 9afa25b..6e557b1 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -115,17 +115,20 @@ EXPORT_SYMBOL(sg_init_one);
  */
 static struct scatterlist *sg_kmalloc(unsigned int nents, gfp_t gfp_mask)
 {
-	if (nents == SG_MAX_SINGLE_ALLOC)
-		return (struct scatterlist *) __get_free_page(gfp_mask);
-	else
+	if (nents == SG_MAX_SINGLE_ALLOC) {
+		void *ptr = (void *) __get_free_page(gfp_mask);
+		kmemleak_alloc(ptr, PAGE_SIZE, 1, gfp_mask);
+		return ptr;
+	} else
 		return kmalloc(nents * sizeof(struct scatterlist), gfp_mask);
 }
 
 static void sg_kfree(struct scatterlist *sg, unsigned int nents)
 {
-	if (nents == SG_MAX_SINGLE_ALLOC)
+	if (nents == SG_MAX_SINGLE_ALLOC) {
+		kmemleak_free(sg);
 		free_page((unsigned long) sg);
-	else
+	} else
 		kfree(sg);
 }
 
-- 
1.7.1

Re: [PATCH] lib/scatterlist: Hook sg_kmalloc into kmemleak

[Catalin Marinas]

On Sun, 2010-07-25 at 15:21 +0100, Chris Wilson wrote:
> kmemleak ignores page_alloc() and so believes the final sub-page
> allocation using the plain kmalloc is decoupled and lost. This leads to
> lots of false-positives with code that uses scatterlists.
> 
> The options seem to be either to tell kmemleak that the kmalloc is not
> leaked or to notify kmemleak of the page allocations. The danger of the
> first approach is that we may hide a real leak, so choose the latter
> approach (of which I am not sure of the downsides).

The patch looks fine to me. It would be useful to have some comment
where kmemleak_*() functions are called so that people reading the code
know why they are needed.

Thanks.

-- 
Catalin

[PATCH] lib/scatterlist: Hook sg_kmalloc into kmemleak (v2)

[Chris Wilson]

kmemleak ignores page_alloc() and so believes the final sub-page
allocation using the plain kmalloc is decoupled and lost. This leads to
lots of false-positives with code that uses scatterlists.

The options seem to be either to tell kmemleak that the kmalloc is not
leaked or to notify kmemleak of the page allocations. The danger of the
first approach is that we may hide a real leak, so choose the latter
approach (of which I am not sure of the downsides).

v2: Added comments on the suggestion of Catalin.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jaxboe@fusionio.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 lib/scatterlist.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 9afa25b..fc85552 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -115,17 +115,28 @@ EXPORT_SYMBOL(sg_init_one);
  */
 static struct scatterlist *sg_kmalloc(unsigned int nents, gfp_t gfp_mask)
 {
-	if (nents == SG_MAX_SINGLE_ALLOC)
-		return (struct scatterlist *) __get_free_page(gfp_mask);
-	else
+	if (nents == SG_MAX_SINGLE_ALLOC) {
+		/* kmemleak doesn't track page allocations as they are not
+		 * commonly used (in a raw form) for kernel data structures.
+		 * As we chain together a list of pages and then a normal
+		 * kmalloc (tracked by kmemleak), in order to for that last
+		 * allocation not to become decoupled (and thus a
+		 * false-positive) we need to inform kmemleak of all the
+		 * intermediate allocations.
+		 */
+		void *ptr = (void *) __get_free_page(gfp_mask);
+		kmemleak_alloc(ptr, PAGE_SIZE, 1, gfp_mask);
+		return ptr;
+	} else
 		return kmalloc(nents * sizeof(struct scatterlist), gfp_mask);
 }
 
 static void sg_kfree(struct scatterlist *sg, unsigned int nents)
 {
-	if (nents == SG_MAX_SINGLE_ALLOC)
+	if (nents == SG_MAX_SINGLE_ALLOC) {
+		kmemleak_free(sg);
 		free_page((unsigned long) sg);
-	else
+	} else
 		kfree(sg);
 }
 
-- 
1.7.1

Re: [PATCH] lib/scatterlist: Hook sg_kmalloc into kmemleak (v2)

[Catalin Marinas]

On Tue, 2010-07-27 at 09:44 +0100, Chris Wilson wrote:
> kmemleak ignores page_alloc() and so believes the final sub-page
> allocation using the plain kmalloc is decoupled and lost. This leads to
> lots of false-positives with code that uses scatterlists.
> 
> The options seem to be either to tell kmemleak that the kmalloc is not
> leaked or to notify kmemleak of the page allocations. The danger of the
> first approach is that we may hide a real leak, so choose the latter
> approach (of which I am not sure of the downsides).
> 
> v2: Added comments on the suggestion of Catalin.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks. The patch looks good. I'll place it in my kmemleak branch and
send it to Linus together with other kmemleak patches (hopefully I won't
miss the merging window as I'm going on holiday for a week).

-- 
Catalin

Re: [PATCH] lib/scatterlist: Hook sg_kmalloc into kmemleak (v2)

[Catalin Marinas]

Jens,

On Tue, 2010-07-27 at 09:44 +0100, Chris Wilson wrote:
> kmemleak ignores page_alloc() and so believes the final sub-page
> allocation using the plain kmalloc is decoupled and lost. This leads to
> lots of false-positives with code that uses scatterlists.
> 
> The options seem to be either to tell kmemleak that the kmalloc is not
> leaked or to notify kmemleak of the page allocations. The danger of the
> first approach is that we may hide a real leak, so choose the latter
> approach (of which I am not sure of the downsides).
> 
> v2: Added comments on the suggestion of Catalin.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <jaxboe@fusionio.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>

Since you are the author of lib/scatterlist.c would you (or Tejun) mind
ack'ing this patch? Thanks.

> ---
>  lib/scatterlist.c |   21 ++++++++++++++++-----
>  1 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
> index 9afa25b..fc85552 100644
> --- a/lib/scatterlist.c
> +++ b/lib/scatterlist.c
> @@ -115,17 +115,28 @@ EXPORT_SYMBOL(sg_init_one);
>   */
>  static struct scatterlist *sg_kmalloc(unsigned int nents, gfp_t gfp_mask)
>  {
> -       if (nents == SG_MAX_SINGLE_ALLOC)
> -               return (struct scatterlist *) __get_free_page(gfp_mask);
> -       else
> +       if (nents == SG_MAX_SINGLE_ALLOC) {
> +               /* kmemleak doesn't track page allocations as they are not
> +                * commonly used (in a raw form) for kernel data structures.
> +                * As we chain together a list of pages and then a normal
> +                * kmalloc (tracked by kmemleak), in order to for that last
> +                * allocation not to become decoupled (and thus a
> +                * false-positive) we need to inform kmemleak of all the
> +                * intermediate allocations.
> +                */
> +               void *ptr = (void *) __get_free_page(gfp_mask);
> +               kmemleak_alloc(ptr, PAGE_SIZE, 1, gfp_mask);
> +               return ptr;
> +       } else
>                 return kmalloc(nents * sizeof(struct scatterlist), gfp_mask);
>  }
> 
>  static void sg_kfree(struct scatterlist *sg, unsigned int nents)
>  {
> -       if (nents == SG_MAX_SINGLE_ALLOC)
> +       if (nents == SG_MAX_SINGLE_ALLOC) {
> +               kmemleak_free(sg);
>                 free_page((unsigned long) sg);
> -       else
> +       } else
>                 kfree(sg);
>  }

-- 
Catalin

Re: [PATCH] lib/scatterlist: Hook sg_kmalloc into kmemleak (v2)

[Jens Axboe]

On 08/10/2010 06:01 PM, Catalin Marinas wrote:
> Jens,
> 
> On Tue, 2010-07-27 at 09:44 +0100, Chris Wilson wrote:
>> kmemleak ignores page_alloc() and so believes the final sub-page
>> allocation using the plain kmalloc is decoupled and lost. This leads to
>> lots of false-positives with code that uses scatterlists.
>>
>> The options seem to be either to tell kmemleak that the kmalloc is not
>> leaked or to notify kmemleak of the page allocations. The danger of the
>> first approach is that we may hide a real leak, so choose the latter
>> approach (of which I am not sure of the downsides).
>>
>> v2: Added comments on the suggestion of Catalin.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Jens Axboe <jaxboe@fusionio.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> 
> Since you are the author of lib/scatterlist.c would you (or Tejun) mind
> ack'ing this patch? Thanks.

Looks completely straight forward.

Acked-by: Jens Axboe <jaxboe@fusionio.com>

-- 
Jens Axboe


Confidentiality Notice: This e-mail message, its contents and any attachments to it are confidential to the intended recipient, and may contain information that is privileged and/or exempt from disclosure under applicable law. If you are not the intended recipient, please immediately notify the sender and destroy the original e-mail message and any attachments (and any copies that may have been made) from your system or otherwise. Any unauthorized use, copying, disclosure or distribution of this information is strictly prohibited.