[PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start

[PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start

[Minfei Huang]

For some arch, kexec shall map the reserved pages, then use them, when
we try to start the kdump service.

Now kexec will never unmap the reserved pages, once it fails to continue
starting the kdump service.

Make a pair of reserved pages in kdump starting path, whatever kexec
fails or not.

Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
---
v2:
- replace the "failure" label with "fail_unmap_pages"
v1:
- reconstruct the patch code
---
 kernel/kexec.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 4589899..9cb09d9 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1291,35 +1291,37 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 			 */
 
 			kimage_free(xchg(&kexec_crash_image, NULL));
-			result = kimage_alloc_init(&image, entry, nr_segments,
-						   segments, flags);
-			crash_map_reserved_pages();
-		} else {
-			/* Loading another kernel to reboot into. */
-
-			result = kimage_alloc_init(&image, entry, nr_segments,
-						   segments, flags);
 		}
+
+		result = kimage_alloc_init(&image, entry, nr_segments,
+				segments, flags);
 		if (result)
 			goto out;
 
+		if (flags & KEXEC_ON_CRASH)
+			crash_map_reserved_pages();
+
 		if (flags & KEXEC_PRESERVE_CONTEXT)
 			image->preserve_context = 1;
 		result = machine_kexec_prepare(image);
 		if (result)
-			goto out;
+			goto fail_unmap_pages;
 
 		for (i = 0; i < nr_segments; i++) {
 			result = kimage_load_segment(image, &image->segment[i]);
 			if (result)
-				goto out;
+				goto fail_unmap_pages;
 		}
 		kimage_terminate(image);
+
+fail_unmap_pages:
 		if (flags & KEXEC_ON_CRASH)
 			crash_unmap_reserved_pages();
 	}
-	/* Install the new kernel, and  Uninstall the old */
-	image = xchg(dest_image, image);
+
+	if (result == 0)
+		/* Install the new kernel, and  Uninstall the old */
+		image = xchg(dest_image, image);
 
 out:
 	mutex_unlock(&kexec_mutex);
-- 
2.2.2

Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start

[Vivek Goyal]

On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote:
> For some arch, kexec shall map the reserved pages, then use them, when
> we try to start the kdump service.
> 
> Now kexec will never unmap the reserved pages, once it fails to continue
> starting the kdump service.
> 
> Make a pair of reserved pages in kdump starting path, whatever kexec
> fails or not.
> 
> Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> ---
> v2:
> - replace the "failure" label with "fail_unmap_pages"
> v1:
> - reconstruct the patch code
> ---
>  kernel/kexec.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 

Hi Minfei,

I am thinking of moving kernel loading code in a separate function to
make things little simpler. Right now it is confusing.

Can you please test attached patch. I have only compile tested it. This
is primarily doing what you are doing but in a separate function. It
seems more readable now.

Thanks
Vivek


---
 kernel/kexec.c |   90 +++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 56 insertions(+), 34 deletions(-)

Index: rhvgoyal-linux/kernel/kexec.c
===================================================================
--- rhvgoyal-linux.orig/kernel/kexec.c	2015-07-06 13:59:35.088129148 -0400
+++ rhvgoyal-linux/kernel/kexec.c	2015-07-07 17:14:23.593175644 -0400
@@ -1247,6 +1247,57 @@ int kexec_load_disabled;
 
 static DEFINE_MUTEX(kexec_mutex);
 
+static int __kexec_load(struct kimage **rimage, unsigned long entry,
+			unsigned long nr_segments,
+			struct kexec_segment __user * segments,
+			unsigned long flags)
+{
+	unsigned long i;
+	int result;
+	struct kimage *image;
+
+	if (flags & KEXEC_ON_CRASH) {
+		/*
+		 * Loading another kernel to switch to if this one
+		 * crashes.  Free any current crash dump kernel before
+		 * we corrupt it.
+		 */
+
+		kimage_free(xchg(&kexec_crash_image, NULL));
+	}
+
+	result = kimage_alloc_init(&image, entry, nr_segments, segments, flags);
+	if (result)
+		return result;
+
+	if (flags & KEXEC_ON_CRASH)
+		crash_map_reserved_pages();
+
+	if (flags & KEXEC_PRESERVE_CONTEXT)
+		image->preserve_context = 1;
+
+	result = machine_kexec_prepare(image);
+	if (result)
+		goto out;
+
+	for (i = 0; i < nr_segments; i++) {
+		result = kimage_load_segment(image, &image->segment[i]);
+		if (result)
+			goto out;
+	}
+
+	kimage_terminate(image);
+	*rimage = image;
+out:
+	if (flags & KEXEC_ON_CRASH)
+		crash_unmap_reserved_pages();
+
+	/* Free image if there was an error */
+	if (result)
+		kimage_free(image);
+	return result;
+}
+
 SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 		struct kexec_segment __user *, segments, unsigned long, flags)
 {
@@ -1292,44 +1343,15 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
 	dest_image = &kexec_image;
 	if (flags & KEXEC_ON_CRASH)
 		dest_image = &kexec_crash_image;
-	if (nr_segments > 0) {
-		unsigned long i;
 
-		if (flags & KEXEC_ON_CRASH) {
-			/*
-			 * Loading another kernel to switch to if this one
-			 * crashes.  Free any current crash dump kernel before
-			 * we corrupt it.
-			 */
-
-			kimage_free(xchg(&kexec_crash_image, NULL));
-			result = kimage_alloc_init(&image, entry, nr_segments,
-						   segments, flags);
-			crash_map_reserved_pages();
-		} else {
-			/* Loading another kernel to reboot into. */
-
-			result = kimage_alloc_init(&image, entry, nr_segments,
-						   segments, flags);
-		}
-		if (result)
-			goto out;
-
-		if (flags & KEXEC_PRESERVE_CONTEXT)
-			image->preserve_context = 1;
-		result = machine_kexec_prepare(image);
+	/* Load new kernel */
+	if (nr_segments > 0) {
+		result = __kexec_load(&image, entry, nr_segments, segments,
+				      flags);
 		if (result)
 			goto out;
-
-		for (i = 0; i < nr_segments; i++) {
-			result = kimage_load_segment(image, &image->segment[i]);
-			if (result)
-				goto out;
-		}
-		kimage_terminate(image);
-		if (flags & KEXEC_ON_CRASH)
-			crash_unmap_reserved_pages();
 	}
+
 	/* Install the new kernel, and  Uninstall the old */
 	image = xchg(dest_image, image);
 

Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start

[Minfei Huang]

On 07/07/15 at 05:18P, Vivek Goyal wrote:
> On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote:
> > For some arch, kexec shall map the reserved pages, then use them, when
> > we try to start the kdump service.
> > 
> > Now kexec will never unmap the reserved pages, once it fails to continue
> > starting the kdump service.
> > 
> > Make a pair of reserved pages in kdump starting path, whatever kexec
> > fails or not.
> > 
> > Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> > ---
> > v2:
> > - replace the "failure" label with "fail_unmap_pages"
> > v1:
> > - reconstruct the patch code
> > ---
> >  kernel/kexec.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> > 
> 
> Hi Minfei,
> 
> I am thinking of moving kernel loading code in a separate function to
> make things little simpler. Right now it is confusing.

In my patch, I think maybe the label confuses with the reviewer, which
does not express the intention clearly. 

> 
> Can you please test attached patch. I have only compile tested it. This
> is primarily doing what you are doing but in a separate function. It
> seems more readable now.
> 

The patch passed the simple testcase. Since it does change the code
logic, I think there is no risky.

Thanks
Minfei

> Thanks
> Vivek
> 
> 
> ---
>  kernel/kexec.c |   90 +++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 56 insertions(+), 34 deletions(-)
> 
> Index: rhvgoyal-linux/kernel/kexec.c
> ===================================================================
> --- rhvgoyal-linux.orig/kernel/kexec.c	2015-07-06 13:59:35.088129148 -0400
> +++ rhvgoyal-linux/kernel/kexec.c	2015-07-07 17:14:23.593175644 -0400
> @@ -1247,6 +1247,57 @@ int kexec_load_disabled;
>  
>  static DEFINE_MUTEX(kexec_mutex);
>  
> +static int __kexec_load(struct kimage **rimage, unsigned long entry,
> +			unsigned long nr_segments,
> +			struct kexec_segment __user * segments,
> +			unsigned long flags)
> +{
> +	unsigned long i;
> +	int result;
> +	struct kimage *image;
> +
> +	if (flags & KEXEC_ON_CRASH) {
> +		/*
> +		 * Loading another kernel to switch to if this one
> +		 * crashes.  Free any current crash dump kernel before
> +		 * we corrupt it.
> +		 */
> +
> +		kimage_free(xchg(&kexec_crash_image, NULL));
> +	}
> +
> +	result = kimage_alloc_init(&image, entry, nr_segments, segments, flags);
> +	if (result)
> +		return result;
> +
> +	if (flags & KEXEC_ON_CRASH)
> +		crash_map_reserved_pages();
> +
> +	if (flags & KEXEC_PRESERVE_CONTEXT)
> +		image->preserve_context = 1;
> +
> +	result = machine_kexec_prepare(image);
> +	if (result)
> +		goto out;
> +
> +	for (i = 0; i < nr_segments; i++) {
> +		result = kimage_load_segment(image, &image->segment[i]);
> +		if (result)
> +			goto out;
> +	}
> +
> +	kimage_terminate(image);
> +	*rimage = image;
> +out:
> +	if (flags & KEXEC_ON_CRASH)
> +		crash_unmap_reserved_pages();
> +
> +	/* Free image if there was an error */
> +	if (result)
> +		kimage_free(image);
> +	return result;
> +}
> +
>  SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
>  		struct kexec_segment __user *, segments, unsigned long, flags)
>  {
> @@ -1292,44 +1343,15 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
>  	dest_image = &kexec_image;
>  	if (flags & KEXEC_ON_CRASH)
>  		dest_image = &kexec_crash_image;
> -	if (nr_segments > 0) {
> -		unsigned long i;
>  
> -		if (flags & KEXEC_ON_CRASH) {
> -			/*
> -			 * Loading another kernel to switch to if this one
> -			 * crashes.  Free any current crash dump kernel before
> -			 * we corrupt it.
> -			 */
> -
> -			kimage_free(xchg(&kexec_crash_image, NULL));
> -			result = kimage_alloc_init(&image, entry, nr_segments,
> -						   segments, flags);
> -			crash_map_reserved_pages();
> -		} else {
> -			/* Loading another kernel to reboot into. */
> -
> -			result = kimage_alloc_init(&image, entry, nr_segments,
> -						   segments, flags);
> -		}
> -		if (result)
> -			goto out;
> -
> -		if (flags & KEXEC_PRESERVE_CONTEXT)
> -			image->preserve_context = 1;
> -		result = machine_kexec_prepare(image);
> +	/* Load new kernel */
> +	if (nr_segments > 0) {
> +		result = __kexec_load(&image, entry, nr_segments, segments,
> +				      flags);
>  		if (result)
>  			goto out;
> -
> -		for (i = 0; i < nr_segments; i++) {
> -			result = kimage_load_segment(image, &image->segment[i]);
> -			if (result)
> -				goto out;
> -		}
> -		kimage_terminate(image);
> -		if (flags & KEXEC_ON_CRASH)
> -			crash_unmap_reserved_pages();
>  	}
> +
>  	/* Install the new kernel, and  Uninstall the old */
>  	image = xchg(dest_image, image);
>  

Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start

[Minfei Huang]

On 07/08/15 at 08:06P, Minfei Huang wrote:
> On 07/07/15 at 05:18P, Vivek Goyal wrote:
> > On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote:
> > > For some arch, kexec shall map the reserved pages, then use them, when
> > > we try to start the kdump service.
> > > 
> > > Now kexec will never unmap the reserved pages, once it fails to continue
> > > starting the kdump service.
> > > 
> > > Make a pair of reserved pages in kdump starting path, whatever kexec
> > > fails or not.
> > > 
> > > Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> > > ---
> > > v2:
> > > - replace the "failure" label with "fail_unmap_pages"
> > > v1:
> > > - reconstruct the patch code
> > > ---
> > >  kernel/kexec.c | 26 ++++++++++++++------------
> > >  1 file changed, 14 insertions(+), 12 deletions(-)
> > > 
> > 
> > Hi Minfei,
> > 
> > I am thinking of moving kernel loading code in a separate function to
> > make things little simpler. Right now it is confusing.
> 
> In my patch, I think maybe the label confuses with the reviewer, which
> does not express the intention clearly. 
> 
> > 
> > Can you please test attached patch. I have only compile tested it. This
> > is primarily doing what you are doing but in a separate function. It
> > seems more readable now.
> > 
> 
> The patch passed the simple testcase. Since it does change the code
						   ^^^^^
						 does not change
> logic, I think there is no risky.
> 
> Thanks
> Minfei
> 
> > Thanks
> > Vivek
> > 
> > 
> > ---
> >  kernel/kexec.c |   90 +++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 56 insertions(+), 34 deletions(-)
> > 
> > Index: rhvgoyal-linux/kernel/kexec.c
> > ===================================================================
> > --- rhvgoyal-linux.orig/kernel/kexec.c	2015-07-06 13:59:35.088129148 -0400
> > +++ rhvgoyal-linux/kernel/kexec.c	2015-07-07 17:14:23.593175644 -0400
> > @@ -1247,6 +1247,57 @@ int kexec_load_disabled;
> >  
> >  static DEFINE_MUTEX(kexec_mutex);
> >  
> > +static int __kexec_load(struct kimage **rimage, unsigned long entry,
> > +			unsigned long nr_segments,
> > +			struct kexec_segment __user * segments,
> > +			unsigned long flags)
> > +{
> > +	unsigned long i;
> > +	int result;
> > +	struct kimage *image;
> > +
> > +	if (flags & KEXEC_ON_CRASH) {
> > +		/*
> > +		 * Loading another kernel to switch to if this one
> > +		 * crashes.  Free any current crash dump kernel before
> > +		 * we corrupt it.
> > +		 */
> > +
> > +		kimage_free(xchg(&kexec_crash_image, NULL));
> > +	}
> > +
> > +	result = kimage_alloc_init(&image, entry, nr_segments, segments, flags);
> > +	if (result)
> > +		return result;
> > +
> > +	if (flags & KEXEC_ON_CRASH)
> > +		crash_map_reserved_pages();
> > +
> > +	if (flags & KEXEC_PRESERVE_CONTEXT)
> > +		image->preserve_context = 1;
> > +
> > +	result = machine_kexec_prepare(image);
> > +	if (result)
> > +		goto out;
> > +
> > +	for (i = 0; i < nr_segments; i++) {
> > +		result = kimage_load_segment(image, &image->segment[i]);
> > +		if (result)
> > +			goto out;
> > +	}
> > +
> > +	kimage_terminate(image);
> > +	*rimage = image;
> > +out:
> > +	if (flags & KEXEC_ON_CRASH)
> > +		crash_unmap_reserved_pages();
> > +
> > +	/* Free image if there was an error */
> > +	if (result)
> > +		kimage_free(image);
> > +	return result;
> > +}
> > +
> >  SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
> >  		struct kexec_segment __user *, segments, unsigned long, flags)
> >  {
> > @@ -1292,44 +1343,15 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
> >  	dest_image = &kexec_image;
> >  	if (flags & KEXEC_ON_CRASH)
> >  		dest_image = &kexec_crash_image;
> > -	if (nr_segments > 0) {
> > -		unsigned long i;
> >  
> > -		if (flags & KEXEC_ON_CRASH) {
> > -			/*
> > -			 * Loading another kernel to switch to if this one
> > -			 * crashes.  Free any current crash dump kernel before
> > -			 * we corrupt it.
> > -			 */
> > -
> > -			kimage_free(xchg(&kexec_crash_image, NULL));
> > -			result = kimage_alloc_init(&image, entry, nr_segments,
> > -						   segments, flags);
> > -			crash_map_reserved_pages();
> > -		} else {
> > -			/* Loading another kernel to reboot into. */
> > -
> > -			result = kimage_alloc_init(&image, entry, nr_segments,
> > -						   segments, flags);
> > -		}
> > -		if (result)
> > -			goto out;
> > -
> > -		if (flags & KEXEC_PRESERVE_CONTEXT)
> > -			image->preserve_context = 1;
> > -		result = machine_kexec_prepare(image);
> > +	/* Load new kernel */
> > +	if (nr_segments > 0) {
> > +		result = __kexec_load(&image, entry, nr_segments, segments,
> > +				      flags);
> >  		if (result)
> >  			goto out;
> > -
> > -		for (i = 0; i < nr_segments; i++) {
> > -			result = kimage_load_segment(image, &image->segment[i]);
> > -			if (result)
> > -				goto out;
> > -		}
> > -		kimage_terminate(image);
> > -		if (flags & KEXEC_ON_CRASH)
> > -			crash_unmap_reserved_pages();
> >  	}
> > +
> >  	/* Install the new kernel, and  Uninstall the old */
> >  	image = xchg(dest_image, image);
> >  

Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start

[Michael Holzheu]

On Tue, 7 Jul 2015 17:18:40 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote:

[snip]

> I am thinking of moving kernel loading code in a separate function to
> make things little simpler. Right now it is confusing.
> 
> Can you please test attached patch. I have only compile tested it. This
> is primarily doing what you are doing but in a separate function. It
> seems more readable now.

The patch looks good to me. What about the following patch on top
to make things even more readable?
---
 kernel/kexec.c |   50 +++++++++++++++++---------------------------------
 1 file changed, 17 insertions(+), 33 deletions(-)

--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1236,14 +1236,18 @@ int kexec_load_disabled;
 
 static DEFINE_MUTEX(kexec_mutex);
 
-static int __kexec_load(struct kimage **rimage, unsigned long entry,
-			unsigned long nr_segments,
+static int __kexec_load(unsigned long entry, unsigned long nr_segments,
 			struct kexec_segment __user * segments,
 			unsigned long flags)
 {
+	struct kimage *image, **dest_image;
 	unsigned long i;
 	int result;
-	struct kimage *image;
+
+	dest_image = (flags & KEXEC_ON_CRASH) ? &kexec_crash_image : &kexec_image;
+
+	if (nr_segments == 0)
+		return 0;
 
 	if (flags & KEXEC_ON_CRASH) {
 		/*
@@ -1251,7 +1255,6 @@ static int __kexec_load(struct kimage **
 		 * crashes.  Free any current crash dump kernel before
 		 * we corrupt it.
 		 */
-
 		kimage_free(xchg(&kexec_crash_image, NULL));
 	}
 
@@ -1267,30 +1270,29 @@ static int __kexec_load(struct kimage **
 
 	result = machine_kexec_prepare(image);
 	if (result)
-		goto out;
+		goto fail;
 
 	for (i = 0; i < nr_segments; i++) {
 		result = kimage_load_segment(image, &image->segment[i]);
 		if (result)
-			goto out;
+			goto fail;
 	}
-
 	kimage_terminate(image);
-	*rimage = image;
-out:
+	/* Install the new kernel, and  uninstall the old */
+	kimage_free(xchg(dest_image, image));
 	if (flags & KEXEC_ON_CRASH)
 		crash_unmap_reserved_pages();
-
-	/* Free image if there was an error */
-	if (result)
-		kimage_free(image);
+	return 0;
+fail:
+	if (flags & KEXEC_ON_CRASH)
+		crash_unmap_reserved_pages();
+	kimage_free(image);
 	return result;
 }
 
 SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 		struct kexec_segment __user *, segments, unsigned long, flags)
 {
-	struct kimage **dest_image, *image;
 	int result;
 
 	/* We only trust the superuser with rebooting the system. */
@@ -1315,9 +1317,6 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
 	if (nr_segments > KEXEC_SEGMENT_MAX)
 		return -EINVAL;
 
-	image = NULL;
-	result = 0;
-
 	/* Because we write directly to the reserved memory
 	 * region when loading crash kernels we need a mutex here to
 	 * prevent multiple crash  kernels from attempting to load
@@ -1329,24 +1328,9 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
 	if (!mutex_trylock(&kexec_mutex))
 		return -EBUSY;
 
-	dest_image = &kexec_image;
-	if (flags & KEXEC_ON_CRASH)
-		dest_image = &kexec_crash_image;
-
 	/* Load new kernel */
-	if (nr_segments > 0) {
-		result = __kexec_load(&image, entry, nr_segments, segments,
-				      flags);
-		if (result)
-			goto out;
-	}
-
-	/* Install the new kernel, and  Uninstall the old */
-	image = xchg(dest_image, image);
-
-out:
+	result = __kexec_load(entry, nr_segments, segments, flags);
 	mutex_unlock(&kexec_mutex);
-	kimage_free(image);
 
 	return result;
 }

Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start

[Minfei Huang]

Hi, Michael.

Yes, The code is more readable after wrapping the all of function code
in a function. Since this is a small issue, I think it is better to use
one patch to fix it. I am glad to repost a patch to merge you and
Vivek's patches as one patch.

Thanks
Minfei

On 07/09/15 at 05:54P, Michael Holzheu wrote:
> On Tue, 7 Jul 2015 17:18:40 -0400
> Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote:
> 
> [snip]
> 
> > I am thinking of moving kernel loading code in a separate function to
> > make things little simpler. Right now it is confusing.
> > 
> > Can you please test attached patch. I have only compile tested it. This
> > is primarily doing what you are doing but in a separate function. It
> > seems more readable now.
> 
> The patch looks good to me. What about the following patch on top
> to make things even more readable?
> ---
>  kernel/kexec.c |   50 +++++++++++++++++---------------------------------
>  1 file changed, 17 insertions(+), 33 deletions(-)
> 
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -1236,14 +1236,18 @@ int kexec_load_disabled;
>  
>  static DEFINE_MUTEX(kexec_mutex);
>  
> -static int __kexec_load(struct kimage **rimage, unsigned long entry,
> -			unsigned long nr_segments,
> +static int __kexec_load(unsigned long entry, unsigned long nr_segments,
>  			struct kexec_segment __user * segments,
>  			unsigned long flags)
>  {
> +	struct kimage *image, **dest_image;
>  	unsigned long i;
>  	int result;
> -	struct kimage *image;
> +
> +	dest_image = (flags & KEXEC_ON_CRASH) ? &kexec_crash_image : &kexec_image;
> +
> +	if (nr_segments == 0)
> +		return 0;
>  
>  	if (flags & KEXEC_ON_CRASH) {
>  		/*
> @@ -1251,7 +1255,6 @@ static int __kexec_load(struct kimage **
>  		 * crashes.  Free any current crash dump kernel before
>  		 * we corrupt it.
>  		 */
> -
>  		kimage_free(xchg(&kexec_crash_image, NULL));
>  	}
>  
> @@ -1267,30 +1270,29 @@ static int __kexec_load(struct kimage **
>  
>  	result = machine_kexec_prepare(image);
>  	if (result)
> -		goto out;
> +		goto fail;
>  
>  	for (i = 0; i < nr_segments; i++) {
>  		result = kimage_load_segment(image, &image->segment[i]);
>  		if (result)
> -			goto out;
> +			goto fail;
>  	}
> -
>  	kimage_terminate(image);
> -	*rimage = image;
> -out:
> +	/* Install the new kernel, and  uninstall the old */
> +	kimage_free(xchg(dest_image, image));
>  	if (flags & KEXEC_ON_CRASH)
>  		crash_unmap_reserved_pages();
> -
> -	/* Free image if there was an error */
> -	if (result)
> -		kimage_free(image);
> +	return 0;
> +fail:
> +	if (flags & KEXEC_ON_CRASH)
> +		crash_unmap_reserved_pages();
> +	kimage_free(image);
>  	return result;
>  }
>  
>  SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
>  		struct kexec_segment __user *, segments, unsigned long, flags)
>  {
> -	struct kimage **dest_image, *image;
>  	int result;
>  
>  	/* We only trust the superuser with rebooting the system. */
> @@ -1315,9 +1317,6 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
>  	if (nr_segments > KEXEC_SEGMENT_MAX)
>  		return -EINVAL;
>  
> -	image = NULL;
> -	result = 0;
> -
>  	/* Because we write directly to the reserved memory
>  	 * region when loading crash kernels we need a mutex here to
>  	 * prevent multiple crash  kernels from attempting to load
> @@ -1329,24 +1328,9 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
>  	if (!mutex_trylock(&kexec_mutex))
>  		return -EBUSY;
>  
> -	dest_image = &kexec_image;
> -	if (flags & KEXEC_ON_CRASH)
> -		dest_image = &kexec_crash_image;
> -
>  	/* Load new kernel */
> -	if (nr_segments > 0) {
> -		result = __kexec_load(&image, entry, nr_segments, segments,
> -				      flags);
> -		if (result)
> -			goto out;
> -	}
> -
> -	/* Install the new kernel, and  Uninstall the old */
> -	image = xchg(dest_image, image);
> -
> -out:
> +	result = __kexec_load(entry, nr_segments, segments, flags);
>  	mutex_unlock(&kexec_mutex);
> -	kimage_free(image);
>  
>  	return result;
>  }
> 

Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start

[Minfei Huang]

On 07/09/15 at 05:54P, Michael Holzheu wrote:
> On Tue, 7 Jul 2015 17:18:40 -0400
> Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote:
> 
> [snip]
> 
> > I am thinking of moving kernel loading code in a separate function to
> > make things little simpler. Right now it is confusing.
> > 
> > Can you please test attached patch. I have only compile tested it. This
> > is primarily doing what you are doing but in a separate function. It
> > seems more readable now.
> 
> The patch looks good to me. What about the following patch on top
> to make things even more readable?
> ---
>  kernel/kexec.c |   50 +++++++++++++++++---------------------------------
>  1 file changed, 17 insertions(+), 33 deletions(-)
> 
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -1236,14 +1236,18 @@ int kexec_load_disabled;
>  
>  static DEFINE_MUTEX(kexec_mutex);
>  
> -static int __kexec_load(struct kimage **rimage, unsigned long entry,
> -			unsigned long nr_segments,
> +static int __kexec_load(unsigned long entry, unsigned long nr_segments,
>  			struct kexec_segment __user * segments,
>  			unsigned long flags)
>  {
> +	struct kimage *image, **dest_image;
>  	unsigned long i;
>  	int result;
> -	struct kimage *image;
> +
> +	dest_image = (flags & KEXEC_ON_CRASH) ? &kexec_crash_image : &kexec_image;
> +
> +	if (nr_segments == 0)
> +		return 0;

It is fine, if nr_segments is 0. So we should deal with this case like
original kexec code.

>  
>  	if (flags & KEXEC_ON_CRASH) {
>  		/*
> @@ -1251,7 +1255,6 @@ static int __kexec_load(struct kimage **
>  		 * crashes.  Free any current crash dump kernel before
>  		 * we corrupt it.
>  		 */
> -
>  		kimage_free(xchg(&kexec_crash_image, NULL));
>  	}
>  
> @@ -1267,30 +1270,29 @@ static int __kexec_load(struct kimage **
>  
>  	result = machine_kexec_prepare(image);
>  	if (result)
> -		goto out;
> +		goto fail;
>  
>  	for (i = 0; i < nr_segments; i++) {
>  		result = kimage_load_segment(image, &image->segment[i]);
>  		if (result)
> -			goto out;
> +			goto fail;
>  	}
> -
>  	kimage_terminate(image);
> -	*rimage = image;
> -out:
> +	/* Install the new kernel, and  uninstall the old */
> +	kimage_free(xchg(dest_image, image));
>  	if (flags & KEXEC_ON_CRASH)
>  		crash_unmap_reserved_pages();
> -
> -	/* Free image if there was an error */
> -	if (result)
> -		kimage_free(image);
> +	return 0;
> +fail:
> +	if (flags & KEXEC_ON_CRASH)
> +		crash_unmap_reserved_pages();
> +	kimage_free(image);

Kernel release image again, and will crash in here, since we do not
assign the image to NULL when we release the image above.

Thanks
Minfei

>  	return result;
>  }
>  
>  SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
>  		struct kexec_segment __user *, segments, unsigned long, flags)
>  {
> -	struct kimage **dest_image, *image;
>  	int result;
>  
>  	/* We only trust the superuser with rebooting the system. */
> @@ -1315,9 +1317,6 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
>  	if (nr_segments > KEXEC_SEGMENT_MAX)
>  		return -EINVAL;
>  
> -	image = NULL;
> -	result = 0;
> -
>  	/* Because we write directly to the reserved memory
>  	 * region when loading crash kernels we need a mutex here to
>  	 * prevent multiple crash  kernels from attempting to load
> @@ -1329,24 +1328,9 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
>  	if (!mutex_trylock(&kexec_mutex))
>  		return -EBUSY;
>  
> -	dest_image = &kexec_image;
> -	if (flags & KEXEC_ON_CRASH)
> -		dest_image = &kexec_crash_image;
> -
>  	/* Load new kernel */
> -	if (nr_segments > 0) {
> -		result = __kexec_load(&image, entry, nr_segments, segments,
> -				      flags);
> -		if (result)
> -			goto out;
> -	}
> -
> -	/* Install the new kernel, and  Uninstall the old */
> -	image = xchg(dest_image, image);
> -
> -out:
> +	result = __kexec_load(entry, nr_segments, segments, flags);
>  	mutex_unlock(&kexec_mutex);
> -	kimage_free(image);
>  
>  	return result;
>  }
> 

Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start

[Michael Holzheu]

On Fri, 10 Jul 2015 12:05:27 +0800
Minfei Huang <mnfhuang@gmail.com> wrote:

> On 07/09/15 at 05:54P, Michael Holzheu wrote:
> > On Tue, 7 Jul 2015 17:18:40 -0400
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> > 
> > > On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote:
> > 
> > [snip]
> > 
> > > I am thinking of moving kernel loading code in a separate function to
> > > make things little simpler. Right now it is confusing.
> > > 
> > > Can you please test attached patch. I have only compile tested it. This
> > > is primarily doing what you are doing but in a separate function. It
> > > seems more readable now.
> > 
> > The patch looks good to me. What about the following patch on top
> > to make things even more readable?
> > ---
> >  kernel/kexec.c |   50 +++++++++++++++++---------------------------------
> >  1 file changed, 17 insertions(+), 33 deletions(-)
> > 
> > --- a/kernel/kexec.c
> > +++ b/kernel/kexec.c
> > @@ -1236,14 +1236,18 @@ int kexec_load_disabled;
> >  
> >  static DEFINE_MUTEX(kexec_mutex);
> >  
> > -static int __kexec_load(struct kimage **rimage, unsigned long entry,
> > -			unsigned long nr_segments,
> > +static int __kexec_load(unsigned long entry, unsigned long nr_segments,
> >  			struct kexec_segment __user * segments,
> >  			unsigned long flags)
> >  {
> > +	struct kimage *image, **dest_image;
> >  	unsigned long i;
> >  	int result;
> > -	struct kimage *image;
> > +
> > +	dest_image = (flags & KEXEC_ON_CRASH) ? &kexec_crash_image : &kexec_image;
> > +
> > +	if (nr_segments == 0)
> > +		return 0;
> 
> It is fine, if nr_segments is 0. So we should deal with this case like
> original kexec code.
> 
> >  
> >  	if (flags & KEXEC_ON_CRASH) {
> >  		/*
> > @@ -1251,7 +1255,6 @@ static int __kexec_load(struct kimage **
> >  		 * crashes.  Free any current crash dump kernel before
> >  		 * we corrupt it.
> >  		 */
> > -
> >  		kimage_free(xchg(&kexec_crash_image, NULL));
> >  	}
> >  
> > @@ -1267,30 +1270,29 @@ static int __kexec_load(struct kimage **
> >  
> >  	result = machine_kexec_prepare(image);
> >  	if (result)
> > -		goto out;
> > +		goto fail;
> >  
> >  	for (i = 0; i < nr_segments; i++) {
> >  		result = kimage_load_segment(image, &image->segment[i]);
> >  		if (result)
> > -			goto out;
> > +			goto fail;
> >  	}
> > -
> >  	kimage_terminate(image);
> > -	*rimage = image;
> > -out:
> > +	/* Install the new kernel, and  uninstall the old */
> > +	kimage_free(xchg(dest_image, image));
> >  	if (flags & KEXEC_ON_CRASH)
> >  		crash_unmap_reserved_pages();
> > -
> > -	/* Free image if there was an error */
> > -	if (result)
> > -		kimage_free(image);
> > +	return 0;
> > +fail:
> > +	if (flags & KEXEC_ON_CRASH)
> > +		crash_unmap_reserved_pages();
> > +	kimage_free(image);
> 
> Kernel release image again

Again? This is only done in the error case.

> , and will crash in here, since we do not
> assign the image to NULL when we release the image above.

Good catch, I should have set image=NULL at the beginning of __kexec_load().

Michael