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

PATCH Superseded Minfei Huang 3 participants 8 messages Jul 2, 2015 – Jul 10, 2015

Subsystems (4)

s390 Live update kexec handover (KHO) kexec

lore

Share & tools

lore

Minfei Huang

author

2015-07-02 01:45 UTC #0 lore raw

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

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 <[email protected]>

---
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(-)

kernel/kexec.c +14 ~~-12~~

index 4589899..9cb09d9 100644
@@ -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);

Signature · 1 line

2.2.2

Vivek Goyal

2015-07-07 21:18 UTC #1 lore raw

Quoted 10 lines · 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 <[email protected]>

Quoted 9 lines

---
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(-)

kernel/kexec.c +56 ~~-34~~

Index: rhvgoyal-linux/kernel/kexec.c
===================================================================
@@ -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);

Minfei Huang

author

2015-07-08 12:06 UTC #2 lore raw

Quoted 21 lines · Vivek Goyal

Quoted 10 lines · 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 <[email protected]>
Quoted 9 lines
---
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(-)

Quoted 4 lines

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.

Quoted 5 lines

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

Quoted 120 lines

Thanks
Vivek

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

Index: rhvgoyal-linux/kernel/kexec.c
===================================================================

kernel/kexec.c +56 ~~-34~~

@@ -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);

Minfei Huang

author

2015-07-08 12:09 UTC #3 lore raw

Quoted 31 lines · Minfei Huang

Quoted 21 lines · Vivek Goyal
Quoted 10 lines · 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 <[email protected]>
Quoted 9 lines
---
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(-)
Quoted 4 lines
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.
Quoted 1 line

Quoted 6 lines

Quoted 4 lines
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

Quoted 125 lines

logic, I think there is no risky.

Thanks
Minfei

Quoted 120 lines

Thanks
Vivek

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

Index: rhvgoyal-linux/kernel/kexec.c
===================================================================

kernel/kexec.c +56 ~~-34~~

@@ -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);

Michael Holzheu

2015-07-09 15:54 UTC #4 lore raw

In reply to Vivek Goyal

Quoted 1 line · On Tue, 7 Jul 2015 17:18:40 -0400 Vivek Goyal <[email protected]> wrote:

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

[snip]

Quoted 6 lines

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(-)

kernel/kexec.c +17 ~~-33~~

@@ -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;
 }

Minfei Huang

author

2015-07-09 23:37 UTC #5 lore raw

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

Quoted 129 lines · Michael Holzheu

Quoted 1 line · On Tue, 7 Jul 2015 17:18:40 -0400 Vivek Goyal <[email protected]> wrote:

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

[snip]

Quoted 6 lines

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(-)

kernel/kexec.c +17 ~~-33~~

@@ -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;
 }

Minfei Huang

author

2015-07-10 04:05 UTC #6 lore raw

In reply to Michael Holzheu

Quoted 31 lines · Michael Holzheu

Quoted 1 line · On Tue, 7 Jul 2015 17:18:40 -0400 Vivek Goyal <[email protected]> wrote:
On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote:
[snip]
Quoted 6 lines
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(-)
kernel/kexec.c +1 -2
@@ -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)

kernel/kexec.c int kexec_load_disabled; · line 1250

Quoted 10 lines

kernel/kexec.c +6 -1

 {
+	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.

Quoted 31 lines

kernel/kexec.c +4 -6

 	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)

kernel/kexec.c static int __kexec_load(struct kimage ** · line 1289

Quoted 10 lines

kernel/kexec.c +5 -4

 		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

Quoted 47 lines

kernel/kexec.c +1 ~~-20~~

 	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;
 }

Michael Holzheu

2015-07-10 08:28 UTC #7 lore raw

Quoted 81 lines · On Fri, 10 Jul 2015 12:05:27 +0800 Minfei Huang <[email protected]> wrote:

Quoted 31 lines · Michael Holzheu
Quoted 1 line · On Tue, 7 Jul 2015 17:18:40 -0400 Vivek Goyal <[email protected]> wrote:
On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote:
[snip]
Quoted 6 lines
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(-)
kernel/kexec.c +1 -2
@@ -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)
kernel/kexec.c int kexec_load_disabled; · line 1250
Quoted 10 lines
kernel/kexec.c +6 -1
 {
+	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.
Quoted 35 lines
kernel/kexec.c +4 -9
 	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)

Quoted 8 lines

kernel/kexec.c static int __kexec_load(struct kimage ** · line 1289
Quoted 6 lines
kernel/kexec.c +5 -1
-		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.

Quoted 2 lines

, 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