* [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux @ 2022-04-20 23:58 HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-24 10:18 ` Pingfan Liu ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= @ 2022-04-20 23:58 UTC (permalink / raw) To: kexec Currently get_mem_section() validates if SYMBOL(mem_section) is the address of the mem_section array first. But there was a report that the first validation wrongly returned TRUE with -x vmlinux and SPARSEMEM_EXTREME (4.15+) on s390x. This leads to crash failing statup with the following seek error: crash: seek error: kernel virtual address: 67fffc2800 type: "memory section root table" Skip the first validation when satisfying the conditions. Reported-by: Dave Wysochanski <dwysocha@redhat.com> Signed-off-by: Kazuhito Hagio <k-hagio-ab@nec.com> --- makedumpfile.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/makedumpfile.c b/makedumpfile.c index a2f45c84cee3..65d1c7c2f02c 100644 --- a/makedumpfile.c +++ b/makedumpfile.c @@ -3698,6 +3698,22 @@ validate_mem_section(unsigned long *mem_sec, return ret; } +/* + * SYMBOL(mem_section) varies with the combination of memory model and + * its source: + * + * SPARSEMEM + * vmcoreinfo: address of mem_section root array + * -x vmlinux: address of mem_section root array + * + * SPARSEMEM_EXTREME v1 + * vmcoreinfo: address of mem_section root array + * -x vmlinux: address of mem_section root array + * + * SPARSEMEM_EXTREME v2 (with 83e3c48729d9 and a0b1280368d1) 4.15+ + * vmcoreinfo: address of mem_section root array + * -x vmlinux: address of pointer to mem_section root array + */ static int get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, unsigned int num_section) @@ -3710,12 +3726,27 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, strerror(errno)); return FALSE; } + + /* + * There was a report that the first validation wrongly returned TRUE + * with -x vmlinux and SPARSEMEM_EXTREME v2 on s390x, so skip it. + * Howerver, leave the fallback validation as it is for the -i option. + */ + if (is_sparsemem_extreme() && info->name_vmlinux) { + unsigned long flag = 0; + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, + NULL, &flag) + && !(flag & TYPE_ARRAY)) + goto skip_1st_validation; + } + ret = validate_mem_section(mem_sec, SYMBOL(mem_section), mem_section_size, mem_maps, num_section); if (!ret && is_sparsemem_extreme()) { unsigned long mem_section_ptr; +skip_1st_validation: if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, sizeof(mem_section_ptr))) goto out; -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux 2022-04-20 23:58 [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= @ 2022-04-24 10:18 ` Pingfan Liu 2022-04-25 0:48 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-25 9:21 ` Philipp Rudo 2022-04-27 2:01 ` Pingfan Liu 2 siblings, 1 reply; 8+ messages in thread From: Pingfan Liu @ 2022-04-24 10:18 UTC (permalink / raw) To: kexec On Wed, Apr 20, 2022 at 11:58:29PM +0000, HAGIO KAZUHITO(?? ??) wrote: > Currently get_mem_section() validates if SYMBOL(mem_section) is the address > of the mem_section array first. But there was a report that the first > validation wrongly returned TRUE with -x vmlinux and SPARSEMEM_EXTREME > (4.15+) on s390x. This leads to crash failing statup with the following > seek error: > > crash: seek error: kernel virtual address: 67fffc2800 type: "memory section root table" > > Skip the first validation when satisfying the conditions. > I still prefer to your V1, which is discussed internally. In which, the logic was made straight forward. And I suggest some slight change to your V1, which folds "-x vmlinux" logic into is_sparsemem_extreme(). What about the following: (not tested yet, if it is good, I can test it) diff --git a/makedumpfile.c b/makedumpfile.c index a2f45c8..3ebe372 100644 --- a/makedumpfile.c +++ b/makedumpfile.c @@ -2240,11 +2240,18 @@ int is_sparsemem_extreme(void) { if ((ARRAY_LENGTH(mem_section) - == divideup(NR_MEM_SECTIONS(), _SECTIONS_PER_ROOT_EXTREME())) - || (ARRAY_LENGTH(mem_section) == NOT_FOUND_STRUCTURE)) - return TRUE; - else + == divideup(NR_MEM_SECTIONS(), _SECTIONS_PER_ROOT_EXTREME())) { + return TRUE; + } else if (info->name_vmlinux) { + unsigned long flag = 0; + + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, + NULL, &flag) + && !(flag & TYPE_ARRAY)) + return TRUE; + } else { return FALSE; + } } int @@ -3704,28 +3711,24 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, { int ret = FALSE; unsigned long *mem_sec = NULL; + unsigned long mem_section_ptr = SYMBOL(mem_section); if ((mem_sec = malloc(mem_section_size)) == NULL) { ERRMSG("Can't allocate memory for the mem_section. %s\n", strerror(errno)); return FALSE; } - ret = validate_mem_section(mem_sec, SYMBOL(mem_section), - mem_section_size, mem_maps, num_section); - - if (!ret && is_sparsemem_extreme()) { - unsigned long mem_section_ptr; + if (is_sparsemem_extreme()) { if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, sizeof(mem_section_ptr))) goto out; + } - ret = validate_mem_section(mem_sec, mem_section_ptr, - mem_section_size, mem_maps, num_section); - - if (!ret) + ret = validate_mem_section(mem_sec, mem_section_ptr, + mem_section_size, mem_maps, num_section); + if (!ret) ERRMSG("Could not validate mem_section.\n"); - } out: if (mem_sec != NULL) free(mem_sec); -- Thanks, Pingfan > Reported-by: Dave Wysochanski <dwysocha@redhat.com> > Signed-off-by: Kazuhito Hagio <k-hagio-ab@nec.com> > --- > makedumpfile.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/makedumpfile.c b/makedumpfile.c > index a2f45c84cee3..65d1c7c2f02c 100644 > --- a/makedumpfile.c > +++ b/makedumpfile.c > @@ -3698,6 +3698,22 @@ validate_mem_section(unsigned long *mem_sec, > return ret; > } > > +/* > + * SYMBOL(mem_section) varies with the combination of memory model and > + * its source: > + * > + * SPARSEMEM > + * vmcoreinfo: address of mem_section root array > + * -x vmlinux: address of mem_section root array > + * > + * SPARSEMEM_EXTREME v1 > + * vmcoreinfo: address of mem_section root array > + * -x vmlinux: address of mem_section root array > + * > + * SPARSEMEM_EXTREME v2 (with 83e3c48729d9 and a0b1280368d1) 4.15+ > + * vmcoreinfo: address of mem_section root array > + * -x vmlinux: address of pointer to mem_section root array > + */ > static int > get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > unsigned int num_section) > @@ -3710,12 +3726,27 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > strerror(errno)); > return FALSE; > } > + > + /* > + * There was a report that the first validation wrongly returned TRUE > + * with -x vmlinux and SPARSEMEM_EXTREME v2 on s390x, so skip it. > + * Howerver, leave the fallback validation as it is for the -i option. > + */ > + if (is_sparsemem_extreme() && info->name_vmlinux) { > + unsigned long flag = 0; > + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, > + NULL, &flag) > + && !(flag & TYPE_ARRAY)) > + goto skip_1st_validation; > + } > + > ret = validate_mem_section(mem_sec, SYMBOL(mem_section), > mem_section_size, mem_maps, num_section); > > if (!ret && is_sparsemem_extreme()) { > unsigned long mem_section_ptr; > > +skip_1st_validation: > if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, > sizeof(mem_section_ptr))) > goto out; > -- > 2.27.0 > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux 2022-04-24 10:18 ` Pingfan Liu @ 2022-04-25 0:48 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-25 3:29 ` Pingfan Liu 0 siblings, 1 reply; 8+ messages in thread From: HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= @ 2022-04-25 0:48 UTC (permalink / raw) To: kexec Hi Pingfan, -----Original Message----- > On Wed, Apr 20, 2022 at 11:58:29PM +0000, HAGIO KAZUHITO(?? ??) wrote: > > Currently get_mem_section() validates if SYMBOL(mem_section) is the address > > of the mem_section array first. But there was a report that the first > > validation wrongly returned TRUE with -x vmlinux and SPARSEMEM_EXTREME > > (4.15+) on s390x. This leads to crash failing statup with the following > > seek error: > > > > crash: seek error: kernel virtual address: 67fffc2800 type: "memory section root table" > > > > Skip the first validation when satisfying the conditions. > > > > I still prefer to your V1, which is discussed internally. In which, the > logic was made straight forward. And I suggest some slight change to > your V1, which folds "-x vmlinux" logic into is_sparsemem_extreme(). > > What about the following: (not tested yet, if it is good, I can test it) Thanks for your review and suggestion. The purpose of my patch is to distinguish between SPARSEMEM_EXTREME v1 and v2, i.e. whether it has 83e3c48729d9 or not. is_sparsemem_extreme() has to return whether it's SPARSEMEM_EXTREME or SPARSEMEM, so unfortunately it's not suitable to use that logic in the function. Thanks, Kazu > > > diff --git a/makedumpfile.c b/makedumpfile.c > index a2f45c8..3ebe372 100644 > --- a/makedumpfile.c > +++ b/makedumpfile.c > @@ -2240,11 +2240,18 @@ int > is_sparsemem_extreme(void) > { > if ((ARRAY_LENGTH(mem_section) > - == divideup(NR_MEM_SECTIONS(), _SECTIONS_PER_ROOT_EXTREME())) > - || (ARRAY_LENGTH(mem_section) == NOT_FOUND_STRUCTURE)) > - return TRUE; > - else > + == divideup(NR_MEM_SECTIONS(), _SECTIONS_PER_ROOT_EXTREME())) { > + return TRUE; > + } else if (info->name_vmlinux) { > + unsigned long flag = 0; > + > + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, > + NULL, &flag) > + && !(flag & TYPE_ARRAY)) > + return TRUE; > + } else { > return FALSE; > + } > } > > int > @@ -3704,28 +3711,24 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > { > int ret = FALSE; > unsigned long *mem_sec = NULL; > + unsigned long mem_section_ptr = SYMBOL(mem_section); > > if ((mem_sec = malloc(mem_section_size)) == NULL) { > ERRMSG("Can't allocate memory for the mem_section. %s\n", > strerror(errno)); > return FALSE; > } > - ret = validate_mem_section(mem_sec, SYMBOL(mem_section), > - mem_section_size, mem_maps, num_section); > - > - if (!ret && is_sparsemem_extreme()) { > - unsigned long mem_section_ptr; > > + if (is_sparsemem_extreme()) { > if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, > sizeof(mem_section_ptr))) > goto out; > + } > > - ret = validate_mem_section(mem_sec, mem_section_ptr, > - mem_section_size, mem_maps, num_section); > - > - if (!ret) > + ret = validate_mem_section(mem_sec, mem_section_ptr, > + mem_section_size, mem_maps, num_section); > + if (!ret) > ERRMSG("Could not validate mem_section.\n"); > - } > out: > if (mem_sec != NULL) > free(mem_sec); > -- > > Thanks, > > Pingfan > > > Reported-by: Dave Wysochanski <dwysocha@redhat.com> > > Signed-off-by: Kazuhito Hagio <k-hagio-ab@nec.com> > > --- > > makedumpfile.c | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/makedumpfile.c b/makedumpfile.c > > index a2f45c84cee3..65d1c7c2f02c 100644 > > --- a/makedumpfile.c > > +++ b/makedumpfile.c > > @@ -3698,6 +3698,22 @@ validate_mem_section(unsigned long *mem_sec, > > return ret; > > } > > > > +/* > > + * SYMBOL(mem_section) varies with the combination of memory model and > > + * its source: > > + * > > + * SPARSEMEM > > + * vmcoreinfo: address of mem_section root array > > + * -x vmlinux: address of mem_section root array > > + * > > + * SPARSEMEM_EXTREME v1 > > + * vmcoreinfo: address of mem_section root array > > + * -x vmlinux: address of mem_section root array > > + * > > + * SPARSEMEM_EXTREME v2 (with 83e3c48729d9 and a0b1280368d1) 4.15+ > > + * vmcoreinfo: address of mem_section root array > > + * -x vmlinux: address of pointer to mem_section root array > > + */ > > static int > > get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > > unsigned int num_section) > > @@ -3710,12 +3726,27 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > > strerror(errno)); > > return FALSE; > > } > > + > > + /* > > + * There was a report that the first validation wrongly returned TRUE > > + * with -x vmlinux and SPARSEMEM_EXTREME v2 on s390x, so skip it. > > + * Howerver, leave the fallback validation as it is for the -i option. > > + */ > > + if (is_sparsemem_extreme() && info->name_vmlinux) { > > + unsigned long flag = 0; > > + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, > > + NULL, &flag) > > + && !(flag & TYPE_ARRAY)) > > + goto skip_1st_validation; > > + } > > + > > ret = validate_mem_section(mem_sec, SYMBOL(mem_section), > > mem_section_size, mem_maps, num_section); > > > > if (!ret && is_sparsemem_extreme()) { > > unsigned long mem_section_ptr; > > > > +skip_1st_validation: > > if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, > > sizeof(mem_section_ptr))) > > goto out; > > -- > > 2.27.0 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux 2022-04-25 0:48 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= @ 2022-04-25 3:29 ` Pingfan Liu 2022-04-25 7:16 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 0 siblings, 1 reply; 8+ messages in thread From: Pingfan Liu @ 2022-04-25 3:29 UTC (permalink / raw) To: kexec On Mon, Apr 25, 2022 at 8:48 AM HAGIO KAZUHITO(?????) <k-hagio-ab@nec.com> wrote: > > Hi Pingfan, > > -----Original Message----- > > On Wed, Apr 20, 2022 at 11:58:29PM +0000, HAGIO KAZUHITO(?? ??) wrote: > > > Currently get_mem_section() validates if SYMBOL(mem_section) is the address > > > of the mem_section array first. But there was a report that the first > > > validation wrongly returned TRUE with -x vmlinux and SPARSEMEM_EXTREME > > > (4.15+) on s390x. This leads to crash failing statup with the following > > > seek error: > > > > > > crash: seek error: kernel virtual address: 67fffc2800 type: "memory section root table" > > > > > > Skip the first validation when satisfying the conditions. > > > > > > > I still prefer to your V1, which is discussed internally. In which, the > > logic was made straight forward. And I suggest some slight change to > > your V1, which folds "-x vmlinux" logic into is_sparsemem_extreme(). > > > > What about the following: (not tested yet, if it is good, I can test it) > > Thanks for your review and suggestion. > > The purpose of my patch is to distinguish between SPARSEMEM_EXTREME > v1 and v2, i.e. whether it has 83e3c48729d9 or not. > Not sure about dwarf, but is it possible to utilize the array length info in is_sparsemem_extreme()? For SPARSEMEM_EXTREME, #ifdef CONFIG_SPARSEMEM_EXTREME extern struct mem_section *mem_section[NR_SECTION_ROOTS]; #else extern struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]; #endif And if DWARF_INFO_GET_SYMBOL_ARRAY_LENGTH works, then there is a big gap between "NR_SECTION_ROOTS * 8-bytes" and "sizeof(struct mem_section) * NR_SECTION_ROOTS * SECTIONS_PER_ROOT" Thanks, Pingfan > is_sparsemem_extreme() has to return whether it's SPARSEMEM_EXTREME or > SPARSEMEM, so unfortunately it's not suitable to use that logic in > the function. > > Thanks, > Kazu > > > > > > > diff --git a/makedumpfile.c b/makedumpfile.c > > index a2f45c8..3ebe372 100644 > > --- a/makedumpfile.c > > +++ b/makedumpfile.c > > @@ -2240,11 +2240,18 @@ int > > is_sparsemem_extreme(void) > > { > > if ((ARRAY_LENGTH(mem_section) > > - == divideup(NR_MEM_SECTIONS(), _SECTIONS_PER_ROOT_EXTREME())) > > - || (ARRAY_LENGTH(mem_section) == NOT_FOUND_STRUCTURE)) > > - return TRUE; > > - else > > + == divideup(NR_MEM_SECTIONS(), _SECTIONS_PER_ROOT_EXTREME())) { > > + return TRUE; > > + } else if (info->name_vmlinux) { > > + unsigned long flag = 0; > > + > > + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, > > + NULL, &flag) > > + && !(flag & TYPE_ARRAY)) > > + return TRUE; > > + } else { > > return FALSE; > > + } > > } > > > > int > > @@ -3704,28 +3711,24 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > > { > > int ret = FALSE; > > unsigned long *mem_sec = NULL; > > + unsigned long mem_section_ptr = SYMBOL(mem_section); > > > > if ((mem_sec = malloc(mem_section_size)) == NULL) { > > ERRMSG("Can't allocate memory for the mem_section. %s\n", > > strerror(errno)); > > return FALSE; > > } > > - ret = validate_mem_section(mem_sec, SYMBOL(mem_section), > > - mem_section_size, mem_maps, num_section); > > - > > - if (!ret && is_sparsemem_extreme()) { > > - unsigned long mem_section_ptr; > > > > + if (is_sparsemem_extreme()) { > > if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, > > sizeof(mem_section_ptr))) > > goto out; > > + } > > > > - ret = validate_mem_section(mem_sec, mem_section_ptr, > > - mem_section_size, mem_maps, num_section); > > - > > - if (!ret) > > + ret = validate_mem_section(mem_sec, mem_section_ptr, > > + mem_section_size, mem_maps, num_section); > > + if (!ret) > > ERRMSG("Could not validate mem_section.\n"); > > - } > > out: > > if (mem_sec != NULL) > > free(mem_sec); > > -- > > > > Thanks, > > > > Pingfan > > > > > Reported-by: Dave Wysochanski <dwysocha@redhat.com> > > > Signed-off-by: Kazuhito Hagio <k-hagio-ab@nec.com> > > > --- > > > makedumpfile.c | 31 +++++++++++++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > > > > > diff --git a/makedumpfile.c b/makedumpfile.c > > > index a2f45c84cee3..65d1c7c2f02c 100644 > > > --- a/makedumpfile.c > > > +++ b/makedumpfile.c > > > @@ -3698,6 +3698,22 @@ validate_mem_section(unsigned long *mem_sec, > > > return ret; > > > } > > > > > > +/* > > > + * SYMBOL(mem_section) varies with the combination of memory model and > > > + * its source: > > > + * > > > + * SPARSEMEM > > > + * vmcoreinfo: address of mem_section root array > > > + * -x vmlinux: address of mem_section root array > > > + * > > > + * SPARSEMEM_EXTREME v1 > > > + * vmcoreinfo: address of mem_section root array > > > + * -x vmlinux: address of mem_section root array > > > + * > > > + * SPARSEMEM_EXTREME v2 (with 83e3c48729d9 and a0b1280368d1) 4.15+ > > > + * vmcoreinfo: address of mem_section root array > > > + * -x vmlinux: address of pointer to mem_section root array > > > + */ > > > static int > > > get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > > > unsigned int num_section) > > > @@ -3710,12 +3726,27 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > > > strerror(errno)); > > > return FALSE; > > > } > > > + > > > + /* > > > + * There was a report that the first validation wrongly returned TRUE > > > + * with -x vmlinux and SPARSEMEM_EXTREME v2 on s390x, so skip it. > > > + * Howerver, leave the fallback validation as it is for the -i option. > > > + */ > > > + if (is_sparsemem_extreme() && info->name_vmlinux) { > > > + unsigned long flag = 0; > > > + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, > > > + NULL, &flag) > > > + && !(flag & TYPE_ARRAY)) > > > + goto skip_1st_validation; > > > + } > > > + > > > ret = validate_mem_section(mem_sec, SYMBOL(mem_section), > > > mem_section_size, mem_maps, num_section); > > > > > > if (!ret && is_sparsemem_extreme()) { > > > unsigned long mem_section_ptr; > > > > > > +skip_1st_validation: > > > if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, > > > sizeof(mem_section_ptr))) > > > goto out; > > > -- > > > 2.27.0 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux 2022-04-25 3:29 ` Pingfan Liu @ 2022-04-25 7:16 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 0 siblings, 0 replies; 8+ messages in thread From: HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= @ 2022-04-25 7:16 UTC (permalink / raw) To: kexec -----Original Message----- > On Mon, Apr 25, 2022 at 8:48 AM HAGIO KAZUHITO(?????) > <k-hagio-ab@nec.com> wrote: > > > > Hi Pingfan, > > > > -----Original Message----- > > > On Wed, Apr 20, 2022 at 11:58:29PM +0000, HAGIO KAZUHITO(?? ??) wrote: > > > > Currently get_mem_section() validates if SYMBOL(mem_section) is the address > > > > of the mem_section array first. But there was a report that the first > > > > validation wrongly returned TRUE with -x vmlinux and SPARSEMEM_EXTREME > > > > (4.15+) on s390x. This leads to crash failing statup with the following > > > > seek error: > > > > > > > > crash: seek error: kernel virtual address: 67fffc2800 type: "memory section root table" > > > > > > > > Skip the first validation when satisfying the conditions. > > > > > > > > > > I still prefer to your V1, which is discussed internally. In which, the > > > logic was made straight forward. And I suggest some slight change to > > > your V1, which folds "-x vmlinux" logic into is_sparsemem_extreme(). > > > > > > What about the following: (not tested yet, if it is good, I can test it) > > > > Thanks for your review and suggestion. > > > > The purpose of my patch is to distinguish between SPARSEMEM_EXTREME > > v1 and v2, i.e. whether it has 83e3c48729d9 or not. > > > > Not sure about dwarf, but is it possible to utilize the array length > info in is_sparsemem_extreme()? > > For SPARSEMEM_EXTREME, > #ifdef CONFIG_SPARSEMEM_EXTREME > extern struct mem_section *mem_section[NR_SECTION_ROOTS]; > #else > extern struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]; > #endif > > And if DWARF_INFO_GET_SYMBOL_ARRAY_LENGTH works, then there is a big > gap between "NR_SECTION_ROOTS * 8-bytes" and "sizeof(struct > mem_section) * NR_SECTION_ROOTS * SECTIONS_PER_ROOT" hmm, sorry, I haven't got your point, the current is_sparsemem_extreme() already uses that value to determine whether it's SPARSEMEM_EXTREME or not. and it's doing the same thing with vmlinux, too. > > > if ((ARRAY_LENGTH(mem_section) > > > - == divideup(NR_MEM_SECTIONS(), _SECTIONS_PER_ROOT_EXTREME())) > > > - || (ARRAY_LENGTH(mem_section) == NOT_FOUND_STRUCTURE)) > > > - return TRUE; if (SYMBOL(mem_section) != NOT_FOUND_SYMBOL) SYMBOL_ARRAY_LENGTH_INIT(mem_section, "mem_section"); Thanks, Kazu ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux 2022-04-20 23:58 [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-24 10:18 ` Pingfan Liu @ 2022-04-25 9:21 ` Philipp Rudo 2022-04-27 2:01 ` Pingfan Liu 2 siblings, 0 replies; 8+ messages in thread From: Philipp Rudo @ 2022-04-25 9:21 UTC (permalink / raw) To: kexec Hi Kazu, On Wed, 20 Apr 2022 23:58:29 +0000 HAGIO KAZUHITO(?????) <k-hagio-ab@nec.com> wrote: > Currently get_mem_section() validates if SYMBOL(mem_section) is the address > of the mem_section array first. But there was a report that the first > validation wrongly returned TRUE with -x vmlinux and SPARSEMEM_EXTREME > (4.15+) on s390x. This leads to crash failing statup with the following > seek error: > > crash: seek error: kernel virtual address: 67fffc2800 type: "memory section root table" > > Skip the first validation when satisfying the conditions. > > Reported-by: Dave Wysochanski <dwysocha@redhat.com> > Signed-off-by: Kazuhito Hagio <k-hagio-ab@nec.com> for me this patch looks fine and exactly addresses the problem, that mem_section has different types in the vmlinux and vmcoreinfo. So, for what I want Reviewed-and-Tested-by: Philipp Rudo <prudo@redhat.com> > --- > makedumpfile.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/makedumpfile.c b/makedumpfile.c > index a2f45c84cee3..65d1c7c2f02c 100644 > --- a/makedumpfile.c > +++ b/makedumpfile.c > @@ -3698,6 +3698,22 @@ validate_mem_section(unsigned long *mem_sec, > return ret; > } > > +/* > + * SYMBOL(mem_section) varies with the combination of memory model and > + * its source: > + * > + * SPARSEMEM > + * vmcoreinfo: address of mem_section root array > + * -x vmlinux: address of mem_section root array > + * > + * SPARSEMEM_EXTREME v1 > + * vmcoreinfo: address of mem_section root array > + * -x vmlinux: address of mem_section root array > + * > + * SPARSEMEM_EXTREME v2 (with 83e3c48729d9 and a0b1280368d1) 4.15+ > + * vmcoreinfo: address of mem_section root array > + * -x vmlinux: address of pointer to mem_section root array > + */ > static int > get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > unsigned int num_section) > @@ -3710,12 +3726,27 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > strerror(errno)); > return FALSE; > } > + > + /* > + * There was a report that the first validation wrongly returned TRUE > + * with -x vmlinux and SPARSEMEM_EXTREME v2 on s390x, so skip it. > + * Howerver, leave the fallback validation as it is for the -i option. > + */ > + if (is_sparsemem_extreme() && info->name_vmlinux) { > + unsigned long flag = 0; > + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, > + NULL, &flag) > + && !(flag & TYPE_ARRAY)) > + goto skip_1st_validation; > + } > + > ret = validate_mem_section(mem_sec, SYMBOL(mem_section), > mem_section_size, mem_maps, num_section); > > if (!ret && is_sparsemem_extreme()) { > unsigned long mem_section_ptr; > > +skip_1st_validation: > if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, > sizeof(mem_section_ptr))) > goto out; ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux 2022-04-20 23:58 [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-24 10:18 ` Pingfan Liu 2022-04-25 9:21 ` Philipp Rudo @ 2022-04-27 2:01 ` Pingfan Liu 2022-04-27 5:53 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2 siblings, 1 reply; 8+ messages in thread From: Pingfan Liu @ 2022-04-27 2:01 UTC (permalink / raw) To: kexec On Thu, Apr 21, 2022 at 7:58 AM HAGIO KAZUHITO(?????) <k-hagio-ab@nec.com> wrote: > > Currently get_mem_section() validates if SYMBOL(mem_section) is the address > of the mem_section array first. But there was a report that the first > validation wrongly returned TRUE with -x vmlinux and SPARSEMEM_EXTREME > (4.15+) on s390x. This leads to crash failing statup with the following > seek error: > > crash: seek error: kernel virtual address: 67fffc2800 type: "memory section root table" > > Skip the first validation when satisfying the conditions. > > Reported-by: Dave Wysochanski <dwysocha@redhat.com> > Signed-off-by: Kazuhito Hagio <k-hagio-ab@nec.com> > --- > makedumpfile.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/makedumpfile.c b/makedumpfile.c > index a2f45c84cee3..65d1c7c2f02c 100644 > --- a/makedumpfile.c > +++ b/makedumpfile.c > @@ -3698,6 +3698,22 @@ validate_mem_section(unsigned long *mem_sec, > return ret; > } > > +/* > + * SYMBOL(mem_section) varies with the combination of memory model and > + * its source: > + * > + * SPARSEMEM > + * vmcoreinfo: address of mem_section root array > + * -x vmlinux: address of mem_section root array > + * > + * SPARSEMEM_EXTREME v1 > + * vmcoreinfo: address of mem_section root array > + * -x vmlinux: address of mem_section root array > + * > + * SPARSEMEM_EXTREME v2 (with 83e3c48729d9 and a0b1280368d1) 4.15+ > + * vmcoreinfo: address of mem_section root array > + * -x vmlinux: address of pointer to mem_section root array > + */ > static int > get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > unsigned int num_section) > @@ -3710,12 +3726,27 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > strerror(errno)); > return FALSE; > } > + > + /* > + * There was a report that the first validation wrongly returned TRUE > + * with -x vmlinux and SPARSEMEM_EXTREME v2 on s390x, so skip it. > + * Howerver, leave the fallback validation as it is for the -i option. > + */ > + if (is_sparsemem_extreme() && info->name_vmlinux) { > + unsigned long flag = 0; > + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, > + NULL, &flag) > + && !(flag & TYPE_ARRAY)) > + goto skip_1st_validation; > + } > + > ret = validate_mem_section(mem_sec, SYMBOL(mem_section), > mem_section_size, mem_maps, num_section); > > if (!ret && is_sparsemem_extreme()) { > unsigned long mem_section_ptr; > > +skip_1st_validation: > if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, > sizeof(mem_section_ptr))) > goto out; > -- > 2.27.0 > Discussed with Kazu off-list, and with his nice help, I got clear why he drops V1. Hence, Reviewed-by: Pingfan Liu <piliu@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux 2022-04-27 2:01 ` Pingfan Liu @ 2022-04-27 5:53 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 0 siblings, 0 replies; 8+ messages in thread From: HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= @ 2022-04-27 5:53 UTC (permalink / raw) To: kexec Hi Pingfan, Philipp, Thank you for reviewing and testing this, applied. https://github.com/makedumpfile/makedumpfile/commit/6d0d95ecc04a70f8448d562ff0fbbae237f5c929 Kazu -----Original Message----- > On Thu, Apr 21, 2022 at 7:58 AM HAGIO KAZUHITO(?????) > <k-hagio-ab@nec.com> wrote: > > > > Currently get_mem_section() validates if SYMBOL(mem_section) is the address > > of the mem_section array first. But there was a report that the first > > validation wrongly returned TRUE with -x vmlinux and SPARSEMEM_EXTREME > > (4.15+) on s390x. This leads to crash failing statup with the following > > seek error: > > > > crash: seek error: kernel virtual address: 67fffc2800 type: "memory section root table" > > > > Skip the first validation when satisfying the conditions. > > > > Reported-by: Dave Wysochanski <dwysocha@redhat.com> > > Signed-off-by: Kazuhito Hagio <k-hagio-ab@nec.com> > > --- > > makedumpfile.c | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/makedumpfile.c b/makedumpfile.c > > index a2f45c84cee3..65d1c7c2f02c 100644 > > --- a/makedumpfile.c > > +++ b/makedumpfile.c > > @@ -3698,6 +3698,22 @@ validate_mem_section(unsigned long *mem_sec, > > return ret; > > } > > > > +/* > > + * SYMBOL(mem_section) varies with the combination of memory model and > > + * its source: > > + * > > + * SPARSEMEM > > + * vmcoreinfo: address of mem_section root array > > + * -x vmlinux: address of mem_section root array > > + * > > + * SPARSEMEM_EXTREME v1 > > + * vmcoreinfo: address of mem_section root array > > + * -x vmlinux: address of mem_section root array > > + * > > + * SPARSEMEM_EXTREME v2 (with 83e3c48729d9 and a0b1280368d1) 4.15+ > > + * vmcoreinfo: address of mem_section root array > > + * -x vmlinux: address of pointer to mem_section root array > > + */ > > static int > > get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > > unsigned int num_section) > > @@ -3710,12 +3726,27 @@ get_mem_section(unsigned int mem_section_size, unsigned long *mem_maps, > > strerror(errno)); > > return FALSE; > > } > > + > > + /* > > + * There was a report that the first validation wrongly returned TRUE > > + * with -x vmlinux and SPARSEMEM_EXTREME v2 on s390x, so skip it. > > + * Howerver, leave the fallback validation as it is for the -i option. > > + */ > > + if (is_sparsemem_extreme() && info->name_vmlinux) { > > + unsigned long flag = 0; > > + if (get_symbol_type_name("mem_section", DWARF_INFO_GET_SYMBOL_TYPE, > > + NULL, &flag) > > + && !(flag & TYPE_ARRAY)) > > + goto skip_1st_validation; > > + } > > + > > ret = validate_mem_section(mem_sec, SYMBOL(mem_section), > > mem_section_size, mem_maps, num_section); > > > > if (!ret && is_sparsemem_extreme()) { > > unsigned long mem_section_ptr; > > > > +skip_1st_validation: > > if (!readmem(VADDR, SYMBOL(mem_section), &mem_section_ptr, > > sizeof(mem_section_ptr))) > > goto out; > > -- > > 2.27.0 > > > Discussed with Kazu off-list, and with his nice help, I got clear why > he drops V1. > > Hence, > Reviewed-by: Pingfan Liu <piliu@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-27 5:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-20 23:58 [PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-24 10:18 ` Pingfan Liu 2022-04-25 0:48 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-25 3:29 ` Pingfan Liu 2022-04-25 7:16 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?= 2022-04-25 9:21 ` Philipp Rudo 2022-04-27 2:01 ` Pingfan Liu 2022-04-27 5:53 ` HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=
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.