Skip to contentSkip to conversation

Revert "Revert "kdump, vmcoreinfo: report memory sections virtual addresses""

PATCH NAK Thomas Garnier 3 participants 5 messages Dec 15, 2016 – Dec 16, 2016

Subsystems (4)
x86 Live update kexec handover (KHO) kexec
lore
Share & tools
lore

Thomas Garnier

author
#0 lore raw

PATCH 1/1 Revert "Revert "kdump, vmcoreinfo: report memory sections virtual addresses""

This reverts commit 49fd897573c97b0eaf10f47d850027d78c456cd7.

Reverting back to commit 0549a3c because the values are used by crash and other tools already. I expected this commit would not go through given the unresolved comments. I want it to be easy to resolve major memory section positions when KASLR memory randomization is enabled.

Signed-off-by Thomas Garnier <[email protected]>
---
 arch/x86/kernel/machine_kexec_64.c | 3 +++
 include/linux/kexec.h              | 6 ++++++
 2 files changed, 9 insertions(+)
arch/x86/kernel/machine_kexec_64.c +3 -0
index 2e3c34b..05f3367 100644
@@ -339,6 +339,9 @@ void arch_crash_save_vmcoreinfo(void)
 			      kaslr_offset());
 	VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
 	VMCOREINFO_PHYS_BASE(phys_base);
+	VMCOREINFO_PAGE_OFFSET(PAGE_OFFSET);
+	VMCOREINFO_VMALLOC_START(VMALLOC_START);
+	VMCOREINFO_VMEMMAP_START(VMEMMAP_START);
 }
 
 /* arch-dependent functionality related to kexec file-based syscall */
include/linux/kexec.h +6 -0
index e98e546..ff9c876 100644
@@ -285,6 +285,12 @@ phys_addr_t paddr_vmcoreinfo_note(void);
 	vmcoreinfo_append_str("CONFIG_%s=y\n", #name)
 #define VMCOREINFO_PHYS_BASE(value) \
 	vmcoreinfo_append_str("PHYS_BASE=%lx\n", (unsigned long)value)
+#define VMCOREINFO_PAGE_OFFSET(value) \
+	vmcoreinfo_append_str("PAGE_OFFSET=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMALLOC_START(value) \
+	vmcoreinfo_append_str("VMALLOC_START=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMEMMAP_START(value) \
+	vmcoreinfo_append_str("VMEMMAP_START=%lx\n", (unsigned long)value)
 
 extern struct kimage *kexec_image;
 extern struct kimage *kexec_crash_image;
Signature · 1 line
2.8.0.rc3.226.g39d4020
Nacked-by on v1 1/1 NAK
#1 lore raw
Quoted 6 lines · Thomas Garnier

This reverts commit 49fd897573c97b0eaf10f47d850027d78c456cd7.

Reverting back to commit 0549a3c because the values are used by crash and other tools already. I expected this commit would not go through given the unresolved comments. I want it to be easy to resolve major memory section positions when KASLR memory randomization is enabled.

This patch is broken. The commit referenced is wrong, as is the justification.

These values are not in fact widely used by userspace (they are brand new).

This is a very fragile approach relying on kernel implementation details, so if we can do anything else that is more robust it is much more likely to pass the test of time.

And yes a more robust implementation has been already discussed.

Nacked-by "Eric W. Biederman" <[email protected]>
Quoted 38 lines
Signed-off-by Thomas Garnier <[email protected]>
---
 arch/x86/kernel/machine_kexec_64.c | 3 +++
 include/linux/kexec.h              | 6 ++++++
 2 files changed, 9 insertions(+)
arch/x86/kernel/machine_kexec_64.c +3 -0
index 2e3c34b..05f3367 100644
@@ -339,6 +339,9 @@ void arch_crash_save_vmcoreinfo(void)
 			      kaslr_offset());
 	VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
 	VMCOREINFO_PHYS_BASE(phys_base);
+	VMCOREINFO_PAGE_OFFSET(PAGE_OFFSET);
+	VMCOREINFO_VMALLOC_START(VMALLOC_START);
+	VMCOREINFO_VMEMMAP_START(VMEMMAP_START);
 }
 
 /* arch-dependent functionality related to kexec file-based syscall */
include/linux/kexec.h +6 -0
index e98e546..ff9c876 100644
@@ -285,6 +285,12 @@ phys_addr_t paddr_vmcoreinfo_note(void);
 	vmcoreinfo_append_str("CONFIG_%s=y\n", #name)
 #define VMCOREINFO_PHYS_BASE(value) \
 	vmcoreinfo_append_str("PHYS_BASE=%lx\n", (unsigned long)value)
+#define VMCOREINFO_PAGE_OFFSET(value) \
+	vmcoreinfo_append_str("PAGE_OFFSET=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMALLOC_START(value) \
+	vmcoreinfo_append_str("VMALLOC_START=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMEMMAP_START(value) \
+	vmcoreinfo_append_str("VMEMMAP_START=%lx\n", (unsigned long)value)
 
 extern struct kimage *kexec_image;
 extern struct kimage *kexec_crash_image;
author
#2 lore raw
Quoted 10 lines · Eric W. Biederman
Quoted 6 lines · Thomas Garnier

This reverts commit 49fd897573c97b0eaf10f47d850027d78c456cd7.

Reverting back to commit 0549a3c because the values are used by crash and other tools already. I expected this commit would not go through given the unresolved comments. I want it to be easy to resolve major memory section positions when KASLR memory randomization is enabled.

This patch is broken. The commit referenced is wrong,

Yes, I based them on linux-next. I can update them to linux main tree. Sorry about that.

Quoted 4 lines

as is the justification.

These values are not in fact widely used by userspace (they are brand new).

They were new and got through to the master tree before and on 4.9. I didn't get any feedback on that when it went through.

Quoted 6 lines

This is a very fragile approach relying on kernel implementation details, so if we can do anything else that is more robust it is much more likely to pass the test of time.

And yes a more robust implementation has been already discussed.

There were discussed for the PAGE_OFFSET but not VMALLOC_START. What's your approach for it?

Quoted 40 lines
Nacked-by "Eric W. Biederman" <[email protected]>
Quoted 38 lines
Signed-off-by Thomas Garnier <[email protected]>
---
 arch/x86/kernel/machine_kexec_64.c | 3 +++
 include/linux/kexec.h              | 6 ++++++
 2 files changed, 9 insertions(+)
arch/x86/kernel/machine_kexec_64.c +3 -0
index 2e3c34b..05f3367 100644
@@ -339,6 +339,9 @@ void arch_crash_save_vmcoreinfo(void)
                            kaslr_offset());
      VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
      VMCOREINFO_PHYS_BASE(phys_base);
+     VMCOREINFO_PAGE_OFFSET(PAGE_OFFSET);
+     VMCOREINFO_VMALLOC_START(VMALLOC_START);
+     VMCOREINFO_VMEMMAP_START(VMEMMAP_START);
 }
 
 /* arch-dependent functionality related to kexec file-based syscall */
include/linux/kexec.h +6 -0
index e98e546..ff9c876 100644
@@ -285,6 +285,12 @@ phys_addr_t paddr_vmcoreinfo_note(void);
      vmcoreinfo_append_str("CONFIG_%s=y\n", #name)
 #define VMCOREINFO_PHYS_BASE(value) \
      vmcoreinfo_append_str("PHYS_BASE=%lx\n", (unsigned long)value)
+#define VMCOREINFO_PAGE_OFFSET(value) \
+     vmcoreinfo_append_str("PAGE_OFFSET=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMALLOC_START(value) \
+     vmcoreinfo_append_str("VMALLOC_START=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMEMMAP_START(value) \
+     vmcoreinfo_append_str("VMEMMAP_START=%lx\n", (unsigned long)value)
 
 extern struct kimage *kexec_image;
 extern struct kimage *kexec_crash_image;
Signature · 1 line
Thomas
author
#3 lore raw
Quoted 28 lines · Thomas Garnier
Quoted 10 lines · Eric W. Biederman
Quoted 6 lines · Thomas Garnier

This reverts commit 49fd897573c97b0eaf10f47d850027d78c456cd7.

Reverting back to commit 0549a3c because the values are used by crash and other tools already. I expected this commit would not go through given the unresolved comments. I want it to be easy to resolve major memory section positions when KASLR memory randomization is enabled.

This patch is broken. The commit referenced is wrong,

Yes, I based them on linux-next. I can update them to linux main tree. Sorry about that.

Quoted 4 lines

as is the justification.

These values are not in fact widely used by userspace (they are brand new).

They were new and got through to the master tree before and on 4.9. I didn't get any feedback on that when it went through.

Quoted 4 lines

This is a very fragile approach relying on kernel implementation details, so if we can do anything else that is more robust it is much more likely to pass the test of time.

Quoted 6 lines
Quoted 2 lines

And yes a more robust implementation has been already discussed.

There were discussed for the PAGE_OFFSET but not VMALLOC_START. What's your approach for it?

Also with improvement on KASLR memory randomization, I don't think we should assume memory sections by looking at PT_LOAD first entries. We will certainly try to randomize the order. That's why I thought clearly indicating which VA define which memory section was a good idea.

I also want to find a time proof approach where we can use it no matter the changes to KASLR.

Quoted 45 lines
Quoted 38 lines
Nacked-by "Eric W. Biederman" <[email protected]>
Quoted 36 lines
Signed-off-by Thomas Garnier <[email protected]>
---
 arch/x86/kernel/machine_kexec_64.c | 3 +++
 include/linux/kexec.h              | 6 ++++++
 2 files changed, 9 insertions(+)
arch/x86/kernel/machine_kexec_64.c +3 -0
index 2e3c34b..05f3367 100644
@@ -339,6 +339,9 @@ void arch_crash_save_vmcoreinfo(void)
                            kaslr_offset());
      VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
      VMCOREINFO_PHYS_BASE(phys_base);
+     VMCOREINFO_PAGE_OFFSET(PAGE_OFFSET);
+     VMCOREINFO_VMALLOC_START(VMALLOC_START);
+     VMCOREINFO_VMEMMAP_START(VMEMMAP_START);
 }
 
 /* arch-dependent functionality related to kexec file-based syscall */
include/linux/kexec.h +6 -0
index e98e546..ff9c876 100644
@@ -285,6 +285,12 @@ phys_addr_t paddr_vmcoreinfo_note(void);
      vmcoreinfo_append_str("CONFIG_%s=y\n", #name)
 #define VMCOREINFO_PHYS_BASE(value) \
      vmcoreinfo_append_str("PHYS_BASE=%lx\n", (unsigned long)value)
+#define VMCOREINFO_PAGE_OFFSET(value) \
+     vmcoreinfo_append_str("PAGE_OFFSET=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMALLOC_START(value) \
+     vmcoreinfo_append_str("VMALLOC_START=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMEMMAP_START(value) \
+     vmcoreinfo_append_str("VMEMMAP_START=%lx\n", (unsigned long)value)
Quoted 2 lines

include/linux/kexec.h phys_addr_t paddr_vmcoreinfo_note(void); · line 296

Quoted 2 lines
include/linux/kexec.h +0 -0
 extern struct kimage *kexec_image;
 extern struct kimage *kexec_crash_image;

--
Thomas

Signature · 1 line
Thomas
#4 lore raw
Quoted 12 lines · Thomas Garnier
Quoted 12 lines
Quoted 4 lines

as is the justification.

These values are not in fact widely used by userspace (they are brand new).

They were new and got through to the master tree before and on 4.9. I didn't get any feedback on that when it went through.

Quoted 4 lines

This is a very fragile approach relying on kernel implementation details, so if we can do anything else that is more robust it is much more likely to pass the test of time.

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

And yes a more robust implementation has been already discussed.

There were discussed for the PAGE_OFFSET but not VMALLOC_START. What's your approach for it?

Sorry, Thomas. I didn't realize these are also required other than Makedumpfile. So just revert it after we use a new way to get page_offset and translate VA to PA by page table in makedumpfile. For the time being they are not needed any more by makedumpfile.

While from below discussion we can see Atsushi, maintainer of Makedumpfile, still worry about the parsing PT_LOAD segment header way. I personally think exporting them is easier and more understandable for user space utility.

http://lists.infradead.org/pipermail/kexec/2016-December/017815.html

I can't think of other way for your tool, maybe page_offset you can use the similar way makedumpfile is taking. As for VMALLOC_START and VMEMMAP_START, it could be necessary. And if you are planning to randomize the memory section order, thinking about them carefully now is a good chance lest they need be adjusted again later.

Sorry again for this.

Quoted 66 lines
Quoted 1 line

Also with improvement on KASLR memory randomization, I don't think we should assume memory sections by looking at PT_LOAD first entries. We will certainly try to randomize the order. That's why I thought clearly indicating which VA define which memory section was a good idea.

I also want to find a time proof approach where we can use it no matter the changes to KASLR.

Quoted 45 lines
Quoted 38 lines
Nacked-by "Eric W. Biederman" <[email protected]>
Quoted 36 lines
Signed-off-by Thomas Garnier <[email protected]>
---
 arch/x86/kernel/machine_kexec_64.c | 3 +++
 include/linux/kexec.h              | 6 ++++++
 2 files changed, 9 insertions(+)
arch/x86/kernel/machine_kexec_64.c +3 -0
index 2e3c34b..05f3367 100644
@@ -339,6 +339,9 @@ void arch_crash_save_vmcoreinfo(void)
                            kaslr_offset());
      VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
      VMCOREINFO_PHYS_BASE(phys_base);
+     VMCOREINFO_PAGE_OFFSET(PAGE_OFFSET);
+     VMCOREINFO_VMALLOC_START(VMALLOC_START);
+     VMCOREINFO_VMEMMAP_START(VMEMMAP_START);
 }
 
 /* arch-dependent functionality related to kexec file-based syscall */
include/linux/kexec.h +6 -0
index e98e546..ff9c876 100644
@@ -285,6 +285,12 @@ phys_addr_t paddr_vmcoreinfo_note(void);
      vmcoreinfo_append_str("CONFIG_%s=y\n", #name)
 #define VMCOREINFO_PHYS_BASE(value) \
      vmcoreinfo_append_str("PHYS_BASE=%lx\n", (unsigned long)value)
+#define VMCOREINFO_PAGE_OFFSET(value) \
+     vmcoreinfo_append_str("PAGE_OFFSET=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMALLOC_START(value) \
+     vmcoreinfo_append_str("VMALLOC_START=%lx\n", (unsigned long)value)
+#define VMCOREINFO_VMEMMAP_START(value) \
+     vmcoreinfo_append_str("VMEMMAP_START=%lx\n", (unsigned long)value)
Quoted 2 lines

include/linux/kexec.h phys_addr_t paddr_vmcoreinfo_note(void); · line 296

Quoted 2 lines
include/linux/kexec.h +0 -0
 extern struct kimage *kexec_image;
 extern struct kimage *kexec_crash_image;

--
Thomas

Signature · 6 lines
Thomas

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec