[makedumpfile PATCH] Always use bigger SECTION_MAP_MASK

[makedumpfile PATCH] Always use bigger SECTION_MAP_MASK

[Petr Tesarik]

It is not necessary to distinguish between different kernel
versions. Kernel commit 2d070eab2e82 merely reuses a previously
unused bit (see clarification in kernel commit def9b71ee651a). The
bit was always zero even before that commit, so it is safe to mask
it off even for kernel versions before 4.13, removing some of the
complexity.

More importantly, makedumpfile fails on kernels which backport the
patch on top of an older version (for example SLES15).

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 makedumpfile.c | 5 +----
 makedumpfile.h | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index ed138d3..b180eb3 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -3337,10 +3337,7 @@ section_mem_map_addr(unsigned long addr)
 		return NOT_KV_ADDR;
 	}
 	map = ULONG(mem_section + OFFSET(mem_section.section_mem_map));
-	if (info->kernel_version < KERNEL_VERSION(4, 13, 0))
-		map &= SECTION_MAP_MASK_4_12;
-	else
-		map &= SECTION_MAP_MASK;
+	map &= SECTION_MAP_MASK;
 	free(mem_section);
 
 	return map;
diff --git a/makedumpfile.h b/makedumpfile.h
index 01eece2..548aab0 100644
--- a/makedumpfile.h
+++ b/makedumpfile.h
@@ -186,7 +186,6 @@ isAnon(unsigned long mapping)
 #define SECTION_NR_TO_ROOT(sec)	((sec) / SECTIONS_PER_ROOT())
 #define SECTION_IS_ONLINE	(1UL<<2)
 #define SECTION_MAP_LAST_BIT	(1UL<<3)
-#define SECTION_MAP_MASK_4_12	(~(SECTION_IS_ONLINE-1))
 #define SECTION_MAP_MASK	(~(SECTION_MAP_LAST_BIT-1))
 #define NR_SECTION_ROOTS()	divideup(num_section, SECTIONS_PER_ROOT())
 #define SECTION_NR_TO_PFN(sec)	((sec) << PFN_SECTION_SHIFT())
-- 
2.13.6

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [makedumpfile PATCH] Always use bigger SECTION_MAP_MASK

[Masaki Tachibana]

Hi Petr,

Sorry for the late reply.

> -----Original Message-----
> From: kexec [mailto:kexec-bounces@lists.infradead.org] On Behalf Of Petr Tesarik
> Sent: Friday, February 02, 2018 5:19 PM
> To: kexec mailing list <kexec@lists.infradead.org>; Tachibana Masaki() <mas-tachibana@vf.jp.nec.com>; Nakayama
> Takuya() <tak-nakayama@tg.jp.nec.com>; Nishimura Daisuke() <dai-nishimura@rc.jp.nec.com>
> Subject: [makedumpfile PATCH] Always use bigger SECTION_MAP_MASK
> 
> It is not necessary to distinguish between different kernel
> versions. Kernel commit 2d070eab2e82 merely reuses a previously
> unused bit (see clarification in kernel commit def9b71ee651a). The
> bit was always zero even before that commit, so it is safe to mask
> it off even for kernel versions before 4.13, removing some of the
> complexity.
> 
> More importantly, makedumpfile fails on kernels which backport the
> patch on top of an older version (for example SLES15).

I understand circumstances of SLES.
However I am worried;
- Is it good that we correct the following specified code for the 
  particular distribution?
- Is it good that makedumpfile ignores the bit because the kernel 
  isn't using it?
Everyone, any comments?

Thanks
Tachibana


> 
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
>  makedumpfile.c | 5 +----
>  makedumpfile.h | 1 -
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index ed138d3..b180eb3 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -3337,10 +3337,7 @@ section_mem_map_addr(unsigned long addr)
>  		return NOT_KV_ADDR;
>  	}
>  	map = ULONG(mem_section + OFFSET(mem_section.section_mem_map));
> -	if (info->kernel_version < KERNEL_VERSION(4, 13, 0))
> -		map &= SECTION_MAP_MASK_4_12;
> -	else
> -		map &= SECTION_MAP_MASK;
> +	map &= SECTION_MAP_MASK;
>  	free(mem_section);
> 
>  	return map;
> diff --git a/makedumpfile.h b/makedumpfile.h
> index 01eece2..548aab0 100644
> --- a/makedumpfile.h
> +++ b/makedumpfile.h
> @@ -186,7 +186,6 @@ isAnon(unsigned long mapping)
>  #define SECTION_NR_TO_ROOT(sec)	((sec) / SECTIONS_PER_ROOT())
>  #define SECTION_IS_ONLINE	(1UL<<2)
>  #define SECTION_MAP_LAST_BIT	(1UL<<3)
> -#define SECTION_MAP_MASK_4_12	(~(SECTION_IS_ONLINE-1))
>  #define SECTION_MAP_MASK	(~(SECTION_MAP_LAST_BIT-1))
>  #define NR_SECTION_ROOTS()	divideup(num_section, SECTIONS_PER_ROOT())
>  #define SECTION_NR_TO_PFN(sec)	((sec) << PFN_SECTION_SHIFT())
> --
> 2.13.6
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [makedumpfile PATCH] Always use bigger SECTION_MAP_MASK

[Petr Tesarik]

Hi,

On Tue, 13 Mar 2018 09:18:54 +0000
Masaki Tachibana <mas-tachibana@vf.jp.nec.com> wrote:

> Hi Petr,
> 
> Sorry for the late reply.

Never mind. I was on vacation, anyway. ;-)

> > -----Original Message-----
> > From: kexec [mailto:kexec-bounces@lists.infradead.org] On Behalf Of Petr Tesarik
> > Sent: Friday, February 02, 2018 5:19 PM
> > To: kexec mailing list <kexec@lists.infradead.org>; Tachibana Masaki() <mas-tachibana@vf.jp.nec.com>; Nakayama
> > Takuya() <tak-nakayama@tg.jp.nec.com>; Nishimura Daisuke() <dai-nishimura@rc.jp.nec.com>
> > Subject: [makedumpfile PATCH] Always use bigger SECTION_MAP_MASK
> > 
> > It is not necessary to distinguish between different kernel
> > versions. Kernel commit 2d070eab2e82 merely reuses a previously
> > unused bit (see clarification in kernel commit def9b71ee651a). The
> > bit was always zero even before that commit, so it is safe to mask
> > it off even for kernel versions before 4.13, removing some of the
> > complexity.
> > 
> > More importantly, makedumpfile fails on kernels which backport the
> > patch on top of an older version (for example SLES15).  
> 
> I understand circumstances of SLES.
> However I am worried;
> - Is it good that we correct the following specified code for the 
>   particular distribution?

No, absolutely not. That's why I'm not trying to fix a particular
distribution, but rather make it work for all distributions (including
my own).

> - Is it good that makedumpfile ignores the bit because the kernel 
>   isn't using it?

I've given it a serious thought before writing my patch. Let me explain.

First, what happens if the bit is set?

  A. If the kernel version is at least 4.13.0, then my patch does not
     change makedumpfile's behaviour.

  B. If the kernel version is older than 4.13.0, then I have verified
     that the bit must not be set. If it is set, then this is a bug in
     the kernel or random memory corruption. The kernel would then use
     an incorrect memmap address, probably crashing in short time.
     Without my patch, makedumpfile will most likely fail to produce
     any output. With my patch, it may produce some output.

Second, my patch simplifies makedumpfile code, which is good in itself,
because it reduces the maintenance burden going forward. A few years
from now everybody will be too afraid of regressions to do such cleanup.

The only downside is that in case B above, makedumpfile may silently
produce incorrect output instead of failing mysteriously with an
error message such as: "Can't convert a virtual address
(f000000001000000) to physical address".

Just my two cents,
Petr Tesarik

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [makedumpfile PATCH] Always use bigger SECTION_MAP_MASK

[Masaki Tachibana]

Hi Petr,

Thank you for your explanation.
I understand.
I think we have to show it's not a bug for the future people.
Would you insert comments before definition of SECTION_MAP_LAST_BIT
and resend the patch ?
For example. (Better English is welcome.)

#define SECTION_IS_ONLINE       (1UL<<2)
/*
 * In case of upstream kernels before 4.13;
 * 1. SECTION_MAP_LAST_BIT must be (1UL<<2), but it's dependent on distribution
 *    whether the bit shift should be 2 or 3.
 * 2. It has been verified that (1UL<<2) bit is not set.
 * So for both old kernels and new ones, we use 3.
 */
#define SECTION_MAP_LAST_BIT    (1UL<<3)

Thanks
Tachibana

> -----Original Message-----
> From: Petr Tesarik [mailto:ptesarik@suse.com]
> Sent: Tuesday, March 13, 2018 7:00 PM
> To: Tachibana Masaki() <mas-tachibana@vf.jp.nec.com>
> Cc: kexec mailing list <kexec@lists.infradead.org>; Hayashi Masahiko() <mas-hayashi@tg.jp.nec.com>
> Subject: Re: [makedumpfile PATCH] Always use bigger SECTION_MAP_MASK
> 
> Hi,
> 
> On Tue, 13 Mar 2018 09:18:54 +0000
> Masaki Tachibana <mas-tachibana@vf.jp.nec.com> wrote:
> 
> > Hi Petr,
> >
> > Sorry for the late reply.
> 
> Never mind. I was on vacation, anyway. ;-)
> 
> > > -----Original Message-----
> > > From: kexec [mailto:kexec-bounces@lists.infradead.org] On Behalf Of Petr Tesarik
> > > Sent: Friday, February 02, 2018 5:19 PM
> > > To: kexec mailing list <kexec@lists.infradead.org>; Tachibana Masaki() <mas-tachibana@vf.jp.nec.com>; Nakayama
> > > Takuya() <tak-nakayama@tg.jp.nec.com>; Nishimura Daisuke() <dai-nishimura@rc.jp.nec.com>
> > > Subject: [makedumpfile PATCH] Always use bigger SECTION_MAP_MASK
> > >
> > > It is not necessary to distinguish between different kernel
> > > versions. Kernel commit 2d070eab2e82 merely reuses a previously
> > > unused bit (see clarification in kernel commit def9b71ee651a). The
> > > bit was always zero even before that commit, so it is safe to mask
> > > it off even for kernel versions before 4.13, removing some of the
> > > complexity.
> > >
> > > More importantly, makedumpfile fails on kernels which backport the
> > > patch on top of an older version (for example SLES15).
> >
> > I understand circumstances of SLES.
> > However I am worried;
> > - Is it good that we correct the following specified code for the
> >   particular distribution?
> 
> No, absolutely not. That's why I'm not trying to fix a particular
> distribution, but rather make it work for all distributions (including
> my own).
> 
> > - Is it good that makedumpfile ignores the bit because the kernel
> >   isn't using it?
> 
> I've given it a serious thought before writing my patch. Let me explain.
> 
> First, what happens if the bit is set?
> 
>   A. If the kernel version is at least 4.13.0, then my patch does not
>      change makedumpfile's behaviour.
> 
>   B. If the kernel version is older than 4.13.0, then I have verified
>      that the bit must not be set. If it is set, then this is a bug in
>      the kernel or random memory corruption. The kernel would then use
>      an incorrect memmap address, probably crashing in short time.
>      Without my patch, makedumpfile will most likely fail to produce
>      any output. With my patch, it may produce some output.
> 
> Second, my patch simplifies makedumpfile code, which is good in itself,
> because it reduces the maintenance burden going forward. A few years
> from now everybody will be too afraid of regressions to do such cleanup.
> 
> The only downside is that in case B above, makedumpfile may silently
> produce incorrect output instead of failing mysteriously with an
> error message such as: "Can't convert a virtual address
> (f000000001000000) to physical address".
> 
> Just my two cents,
> Petr Tesarik



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec