All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] kexec: fix memory leak in function kimage_normal_alloc
@ 2013-02-22  4:36 ` Zhang Yanfei
  0 siblings, 0 replies; 8+ messages in thread
From: Zhang Yanfei @ 2013-02-22  4:36 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: Andrew Morton, Sasha Levin, kexec, linux-kernel

If kimage_normal_alloc() fails to alloc pages for image->swap_page, it
should call kimage_free_page_list() to free allocated pages in
image->control_pages list before it frees image.

Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 kernel/kexec.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 5e4bd78..a57face 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -223,6 +223,8 @@ out:
 
 }
 
+static void kimage_free_page_list(struct list_head *list);
+
 static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
 				unsigned long nr_segments,
 				struct kexec_segment __user *segments)
@@ -248,22 +250,22 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
 					   get_order(KEXEC_CONTROL_PAGE_SIZE));
 	if (!image->control_code_page) {
 		printk(KERN_ERR "Could not allocate control_code_buffer\n");
-		goto out;
+		goto out_free;
 	}
 
 	image->swap_page = kimage_alloc_control_pages(image, 0);
 	if (!image->swap_page) {
 		printk(KERN_ERR "Could not allocate swap buffer\n");
-		goto out;
+		goto out_free;
 	}
 
-	result = 0;
- out:
-	if (result == 0)
-		*rimage = image;
-	else
-		kfree(image);
+	*rimage = image;
+	return 0;
 
+out_free:
+	kimage_free_page_list(&image->control_pages);
+	kfree(image);
+out:
 	return result;
 }
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH] kexec: fix memory leak in function kimage_normal_alloc
@ 2013-02-22  4:36 ` Zhang Yanfei
  0 siblings, 0 replies; 8+ messages in thread
From: Zhang Yanfei @ 2013-02-22  4:36 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: Sasha Levin, Andrew Morton, kexec, linux-kernel

If kimage_normal_alloc() fails to alloc pages for image->swap_page, it
should call kimage_free_page_list() to free allocated pages in
image->control_pages list before it frees image.

Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 kernel/kexec.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 5e4bd78..a57face 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -223,6 +223,8 @@ out:
 
 }
 
+static void kimage_free_page_list(struct list_head *list);
+
 static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
 				unsigned long nr_segments,
 				struct kexec_segment __user *segments)
@@ -248,22 +250,22 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
 					   get_order(KEXEC_CONTROL_PAGE_SIZE));
 	if (!image->control_code_page) {
 		printk(KERN_ERR "Could not allocate control_code_buffer\n");
-		goto out;
+		goto out_free;
 	}
 
 	image->swap_page = kimage_alloc_control_pages(image, 0);
 	if (!image->swap_page) {
 		printk(KERN_ERR "Could not allocate swap buffer\n");
-		goto out;
+		goto out_free;
 	}
 
-	result = 0;
- out:
-	if (result == 0)
-		*rimage = image;
-	else
-		kfree(image);
+	*rimage = image;
+	return 0;
 
+out_free:
+	kimage_free_page_list(&image->control_pages);
+	kfree(image);
+out:
 	return result;
 }
 
-- 
1.7.1

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] kexec: fix memory leak in function kimage_normal_alloc
  2013-02-22  4:36 ` Zhang Yanfei
@ 2013-02-22 21:59   ` Andrew Morton
  -1 siblings, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2013-02-22 21:59 UTC (permalink / raw)
  To: Zhang Yanfei; +Cc: Eric W. Biederman, Sasha Levin, kexec, linux-kernel

On Fri, 22 Feb 2013 12:36:13 +0800
Zhang Yanfei <zhangyanfei@cn.fujitsu.com> wrote:

> If kimage_normal_alloc() fails to alloc pages for image->swap_page, it
> should call kimage_free_page_list() to free allocated pages in
> image->control_pages list before it frees image.
>
> ...
>
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -223,6 +223,8 @@ out:
>  
>  }
>  
> +static void kimage_free_page_list(struct list_head *list);
> +
>  static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  				unsigned long nr_segments,
>  				struct kexec_segment __user *segments)
> @@ -248,22 +250,22 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  					   get_order(KEXEC_CONTROL_PAGE_SIZE));
>  	if (!image->control_code_page) {
>  		printk(KERN_ERR "Could not allocate control_code_buffer\n");
> -		goto out;
> +		goto out_free;
>  	}
>  
>  	image->swap_page = kimage_alloc_control_pages(image, 0);
>  	if (!image->swap_page) {
>  		printk(KERN_ERR "Could not allocate swap buffer\n");
> -		goto out;
> +		goto out_free;
>  	}
>  
> -	result = 0;
> - out:
> -	if (result == 0)
> -		*rimage = image;
> -	else
> -		kfree(image);
> +	*rimage = image;
> +	return 0;
>  
> +out_free:
> +	kimage_free_page_list(&image->control_pages);
> +	kfree(image);
> +out:
>  	return result;
>  }

kimage_alloc_normal_control_pages() won't add any pages to the image if
one of its allocation attemtps failed.  So afaict the first `goto
out_free' could be just `goto out'.

The second `goto out_free' does appear to be needed: it frees the pages
allocated by the first call to kimage_alloc_control_pages().  I think. 
The kimage_alloc_control_pages() handling of image->type is a bit
twisty.

Please double-check the logic?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kexec: fix memory leak in function kimage_normal_alloc
@ 2013-02-22 21:59   ` Andrew Morton
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2013-02-22 21:59 UTC (permalink / raw)
  To: Zhang Yanfei; +Cc: Sasha Levin, kexec, Eric W. Biederman, linux-kernel

On Fri, 22 Feb 2013 12:36:13 +0800
Zhang Yanfei <zhangyanfei@cn.fujitsu.com> wrote:

> If kimage_normal_alloc() fails to alloc pages for image->swap_page, it
> should call kimage_free_page_list() to free allocated pages in
> image->control_pages list before it frees image.
>
> ...
>
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -223,6 +223,8 @@ out:
>  
>  }
>  
> +static void kimage_free_page_list(struct list_head *list);
> +
>  static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  				unsigned long nr_segments,
>  				struct kexec_segment __user *segments)
> @@ -248,22 +250,22 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  					   get_order(KEXEC_CONTROL_PAGE_SIZE));
>  	if (!image->control_code_page) {
>  		printk(KERN_ERR "Could not allocate control_code_buffer\n");
> -		goto out;
> +		goto out_free;
>  	}
>  
>  	image->swap_page = kimage_alloc_control_pages(image, 0);
>  	if (!image->swap_page) {
>  		printk(KERN_ERR "Could not allocate swap buffer\n");
> -		goto out;
> +		goto out_free;
>  	}
>  
> -	result = 0;
> - out:
> -	if (result == 0)
> -		*rimage = image;
> -	else
> -		kfree(image);
> +	*rimage = image;
> +	return 0;
>  
> +out_free:
> +	kimage_free_page_list(&image->control_pages);
> +	kfree(image);
> +out:
>  	return result;
>  }

kimage_alloc_normal_control_pages() won't add any pages to the image if
one of its allocation attemtps failed.  So afaict the first `goto
out_free' could be just `goto out'.

The second `goto out_free' does appear to be needed: it frees the pages
allocated by the first call to kimage_alloc_control_pages().  I think. 
The kimage_alloc_control_pages() handling of image->type is a bit
twisty.

Please double-check the logic?

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kexec: fix memory leak in function kimage_normal_alloc
  2013-02-22  4:36 ` Zhang Yanfei
@ 2013-02-22 22:54   ` Simon Horman
  -1 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2013-02-22 22:54 UTC (permalink / raw)
  To: Zhang Yanfei
  Cc: Eric W. Biederman, Sasha Levin, Andrew Morton, kexec, linux-kernel

On Fri, Feb 22, 2013 at 12:36:13PM +0800, Zhang Yanfei wrote:
> If kimage_normal_alloc() fails to alloc pages for image->swap_page, it
> should call kimage_free_page_list() to free allocated pages in
> image->control_pages list before it frees image.
> 
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>


Reviewed-by: Simon Horman <horms@verge.net.au>

> ---
>  kernel/kexec.c |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 5e4bd78..a57face 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -223,6 +223,8 @@ out:
>  
>  }
>  
> +static void kimage_free_page_list(struct list_head *list);
> +
>  static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  				unsigned long nr_segments,
>  				struct kexec_segment __user *segments)
> @@ -248,22 +250,22 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  					   get_order(KEXEC_CONTROL_PAGE_SIZE));
>  	if (!image->control_code_page) {
>  		printk(KERN_ERR "Could not allocate control_code_buffer\n");
> -		goto out;
> +		goto out_free;
>  	}
>  
>  	image->swap_page = kimage_alloc_control_pages(image, 0);
>  	if (!image->swap_page) {
>  		printk(KERN_ERR "Could not allocate swap buffer\n");
> -		goto out;
> +		goto out_free;
>  	}
>  
> -	result = 0;
> - out:
> -	if (result == 0)
> -		*rimage = image;
> -	else
> -		kfree(image);
> +	*rimage = image;
> +	return 0;
>  
> +out_free:
> +	kimage_free_page_list(&image->control_pages);
> +	kfree(image);
> +out:
>  	return result;
>  }
>  
> -- 
> 1.7.1
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kexec: fix memory leak in function kimage_normal_alloc
@ 2013-02-22 22:54   ` Simon Horman
  0 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2013-02-22 22:54 UTC (permalink / raw)
  To: Zhang Yanfei
  Cc: Sasha Levin, Andrew Morton, kexec, Eric W. Biederman, linux-kernel

On Fri, Feb 22, 2013 at 12:36:13PM +0800, Zhang Yanfei wrote:
> If kimage_normal_alloc() fails to alloc pages for image->swap_page, it
> should call kimage_free_page_list() to free allocated pages in
> image->control_pages list before it frees image.
> 
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>


Reviewed-by: Simon Horman <horms@verge.net.au>

> ---
>  kernel/kexec.c |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 5e4bd78..a57face 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -223,6 +223,8 @@ out:
>  
>  }
>  
> +static void kimage_free_page_list(struct list_head *list);
> +
>  static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  				unsigned long nr_segments,
>  				struct kexec_segment __user *segments)
> @@ -248,22 +250,22 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  					   get_order(KEXEC_CONTROL_PAGE_SIZE));
>  	if (!image->control_code_page) {
>  		printk(KERN_ERR "Could not allocate control_code_buffer\n");
> -		goto out;
> +		goto out_free;
>  	}
>  
>  	image->swap_page = kimage_alloc_control_pages(image, 0);
>  	if (!image->swap_page) {
>  		printk(KERN_ERR "Could not allocate swap buffer\n");
> -		goto out;
> +		goto out_free;
>  	}
>  
> -	result = 0;
> - out:
> -	if (result == 0)
> -		*rimage = image;
> -	else
> -		kfree(image);
> +	*rimage = image;
> +	return 0;
>  
> +out_free:
> +	kimage_free_page_list(&image->control_pages);
> +	kfree(image);
> +out:
>  	return result;
>  }
>  
> -- 
> 1.7.1
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kexec: fix memory leak in function kimage_normal_alloc
  2013-02-22 21:59   ` Andrew Morton
@ 2013-02-23 13:48     ` Yanfei Zhang
  -1 siblings, 0 replies; 8+ messages in thread
From: Yanfei Zhang @ 2013-02-23 13:48 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Zhang Yanfei, Eric W. Biederman, Sasha Levin, kexec, linux-kernel

2013/2/23 Andrew Morton <akpm@linux-foundation.org>:
> On Fri, 22 Feb 2013 12:36:13 +0800
> Zhang Yanfei <zhangyanfei@cn.fujitsu.com> wrote:
>
>> If kimage_normal_alloc() fails to alloc pages for image->swap_page, it
>> should call kimage_free_page_list() to free allocated pages in
>> image->control_pages list before it frees image.
>>
>> ...
>>
>> --- a/kernel/kexec.c
>> +++ b/kernel/kexec.c
>> @@ -223,6 +223,8 @@ out:
>>
>>  }
>>
>> +static void kimage_free_page_list(struct list_head *list);
>> +
>>  static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>>                               unsigned long nr_segments,
>>                               struct kexec_segment __user *segments)
>> @@ -248,22 +250,22 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>>                                          get_order(KEXEC_CONTROL_PAGE_SIZE));
>>       if (!image->control_code_page) {
>>               printk(KERN_ERR "Could not allocate control_code_buffer\n");
>> -             goto out;
>> +             goto out_free;
>>       }
>>
>>       image->swap_page = kimage_alloc_control_pages(image, 0);
>>       if (!image->swap_page) {
>>               printk(KERN_ERR "Could not allocate swap buffer\n");
>> -             goto out;
>> +             goto out_free;
>>       }
>>
>> -     result = 0;
>> - out:
>> -     if (result == 0)
>> -             *rimage = image;
>> -     else
>> -             kfree(image);
>> +     *rimage = image;
>> +     return 0;
>>
>> +out_free:
>> +     kimage_free_page_list(&image->control_pages);
>> +     kfree(image);
>> +out:
>>       return result;
>>  }
>
> kimage_alloc_normal_control_pages() won't add any pages to the image if
> one of its allocation attemtps failed.  So afaict the first `goto
> out_free' could be just `goto out'.

Nop, the first 'goto out_free' is to free allocated 'image' if the allocation of
image->control_code_page failed. If changed to 'goto out', 'image' cannot
be freed and leads to memory leak.

>
> The second `goto out_free' does appear to be needed: it frees the pages
> allocated by the first call to kimage_alloc_control_pages().  I think.
> The kimage_alloc_control_pages() handling of image->type is a bit
> twisty.

Yeah, it could just call kimage_alloc_normal_control_pages directly.

>
> Please double-check the logic?
> --
> 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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] kexec: fix memory leak in function kimage_normal_alloc
@ 2013-02-23 13:48     ` Yanfei Zhang
  0 siblings, 0 replies; 8+ messages in thread
From: Yanfei Zhang @ 2013-02-23 13:48 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Sasha Levin, Zhang Yanfei, Eric W. Biederman, linux-kernel, kexec

2013/2/23 Andrew Morton <akpm@linux-foundation.org>:
> On Fri, 22 Feb 2013 12:36:13 +0800
> Zhang Yanfei <zhangyanfei@cn.fujitsu.com> wrote:
>
>> If kimage_normal_alloc() fails to alloc pages for image->swap_page, it
>> should call kimage_free_page_list() to free allocated pages in
>> image->control_pages list before it frees image.
>>
>> ...
>>
>> --- a/kernel/kexec.c
>> +++ b/kernel/kexec.c
>> @@ -223,6 +223,8 @@ out:
>>
>>  }
>>
>> +static void kimage_free_page_list(struct list_head *list);
>> +
>>  static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>>                               unsigned long nr_segments,
>>                               struct kexec_segment __user *segments)
>> @@ -248,22 +250,22 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>>                                          get_order(KEXEC_CONTROL_PAGE_SIZE));
>>       if (!image->control_code_page) {
>>               printk(KERN_ERR "Could not allocate control_code_buffer\n");
>> -             goto out;
>> +             goto out_free;
>>       }
>>
>>       image->swap_page = kimage_alloc_control_pages(image, 0);
>>       if (!image->swap_page) {
>>               printk(KERN_ERR "Could not allocate swap buffer\n");
>> -             goto out;
>> +             goto out_free;
>>       }
>>
>> -     result = 0;
>> - out:
>> -     if (result == 0)
>> -             *rimage = image;
>> -     else
>> -             kfree(image);
>> +     *rimage = image;
>> +     return 0;
>>
>> +out_free:
>> +     kimage_free_page_list(&image->control_pages);
>> +     kfree(image);
>> +out:
>>       return result;
>>  }
>
> kimage_alloc_normal_control_pages() won't add any pages to the image if
> one of its allocation attemtps failed.  So afaict the first `goto
> out_free' could be just `goto out'.

Nop, the first 'goto out_free' is to free allocated 'image' if the allocation of
image->control_code_page failed. If changed to 'goto out', 'image' cannot
be freed and leads to memory leak.

>
> The second `goto out_free' does appear to be needed: it frees the pages
> allocated by the first call to kimage_alloc_control_pages().  I think.
> The kimage_alloc_control_pages() handling of image->type is a bit
> twisty.

Yeah, it could just call kimage_alloc_normal_control_pages directly.

>
> Please double-check the logic?
> --
> 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/

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-02-23 13:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-22  4:36 [PATCH] kexec: fix memory leak in function kimage_normal_alloc Zhang Yanfei
2013-02-22  4:36 ` Zhang Yanfei
2013-02-22 21:59 ` Andrew Morton
2013-02-22 21:59   ` Andrew Morton
2013-02-23 13:48   ` Yanfei Zhang
2013-02-23 13:48     ` Yanfei Zhang
2013-02-22 22:54 ` Simon Horman
2013-02-22 22:54   ` Simon Horman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.