[PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux

[PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

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

[PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux

[Pingfan Liu]

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
> 

[PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

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

[PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux

[Pingfan Liu]

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

[PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

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

[PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux

[Philipp Rudo]

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;

[PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux

[Pingfan Liu]

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>

[PATCH makedumpfile] Avoid false-positive mem_section validation with vmlinux

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

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>