linux-kernel - Re: [PATCH] s390/crash: Fix KEXEC_NOTE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170609074529.GA9485@dhcp-128-65.nay.redhat.com>
Date:   Fri, 9 Jun 2017 15:45:29 +0800
From:   Dave Young <dyoung@...hat.com>
To:     Xunlei Pang <xlpang@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kexec@...ts.infradead.org,
        akpm@...ux-foundation.org, Eric Biederman <ebiederm@...ssion.com>,
        Baoquan He <bhe@...hat.com>,
        Michael Holzheu <holzheu@...ux.vnet.ibm.com>,
        linux-s390@...r.kernel.org, Dave Anderson <anderson@...hat.com>,
        Hari Bathini <hbathini@...ux.vnet.ibm.com>,
        Gustavo Luiz Duarte <gustavold@...ux.vnet.ibm.com>
Subject: Re: [PATCH] s390/crash: Fix KEXEC_NOTE_BYTES definition

On 06/09/17 at 10:29am, Dave Young wrote:
> On 06/09/17 at 10:17am, Xunlei Pang wrote:
> > S390 KEXEC_NOTE_BYTES is not used by note_buf_t as before, which
> > is now defined as follows:
> >     typedef u32 note_buf_t[CRASH_CORE_NOTE_BYTES/4];
> > It was changed by the CONFIG_CRASH_CORE feature.
> > 
> > This patch gets rid of all the old KEXEC_NOTE_BYTES stuff, and
> > renames KEXEC_NOTE_BYTES to CRASH_CORE_NOTE_BYTES for S390.
> > 
> > Fixes: 692f66f26a4c ("crash: move crashkernel parsing and vmcore related code under CONFIG_CRASH_CORE")
> > Cc: Dave Young <dyoung@...hat.com>
> > Cc: Dave Anderson <anderson@...hat.com>
> > Cc: Hari Bathini <hbathini@...ux.vnet.ibm.com>
> > Cc: Gustavo Luiz Duarte <gustavold@...ux.vnet.ibm.com>
> > Signed-off-by: Xunlei Pang <xlpang@...hat.com>
> > ---
> >  arch/s390/include/asm/kexec.h |  2 +-
> >  include/linux/crash_core.h    |  7 +++++++
> >  include/linux/kexec.h         | 11 +----------
> >  3 files changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/kexec.h b/arch/s390/include/asm/kexec.h
> > index 2f924bc..352deb8 100644
> > --- a/arch/s390/include/asm/kexec.h
> > +++ b/arch/s390/include/asm/kexec.h
> > @@ -47,7 +47,7 @@
> >   * Seven notes plus zero note at the end: prstatus, fpregset, timer,
> >   * tod_cmp, tod_reg, control regs, and prefix
> >   */
> > -#define KEXEC_NOTE_BYTES \
> > +#define CRASH_CORE_NOTE_BYTES \
> >  	(ALIGN(sizeof(struct elf_note), 4) * 8 + \
> >  	 ALIGN(sizeof("CORE"), 4) * 7 + \
> >  	 ALIGN(sizeof(struct elf_prstatus), 4) + \

I found that in mainline since below commit, above define should be
useless, but if distribution with older kernel does need your fix, so in
mainline the right fix should be dropping the s390 part about these
macros usage.

Anyway this need a comment from Michael.

commit 8a07dd02d7615d91d65d6235f7232e3f9b5d347f
Author: Martin Schwidefsky <schwidefsky@...ibm.com>
Date:   Wed Oct 14 15:53:06 2015 +0200

    s390/kdump: remove code to create ELF notes in the crashed system
    
    The s390 architecture can store the CPU registers of the crashed
system
    after the kdump kernel has been started and this is the preferred
way.
    Remove the remaining code fragments that deal with storing CPU
registers
    while the crashed system is still active.
    
    Acked-by: Michael Holzheu <holzheu@...ux.vnet.ibm.com>
    Signed-off-by: Martin Schwidefsky <schwidefsky@...ibm.com>


> > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> > index e9de6b4..dbc6e5c 100644
> > --- a/include/linux/crash_core.h
> > +++ b/include/linux/crash_core.h
> > @@ -10,9 +10,16 @@
> >  #define CRASH_CORE_NOTE_NAME_BYTES ALIGN(sizeof(CRASH_CORE_NOTE_NAME), 4)
> >  #define CRASH_CORE_NOTE_DESC_BYTES ALIGN(sizeof(struct elf_prstatus), 4)
> >  
> > +/*
> > + * The per-cpu notes area is a list of notes terminated by a "NULL"
> > + * note header.  For kdump, the code in vmcore.c runs in the context
> > + * of the second kernel to combine them into one note.
> > + */
> > +#ifndef CRASH_CORE_NOTE_BYTES
> >  #define CRASH_CORE_NOTE_BYTES	   ((CRASH_CORE_NOTE_HEAD_BYTES * 2) +	\
> >  				     CRASH_CORE_NOTE_NAME_BYTES +	\
> >  				     CRASH_CORE_NOTE_DESC_BYTES)
> > +#endif
> >  
> >  #define VMCOREINFO_BYTES	   PAGE_SIZE
> >  #define VMCOREINFO_NOTE_NAME	   "VMCOREINFO"
> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > index 3ea8275..133df03 100644
> > --- a/include/linux/kexec.h
> > +++ b/include/linux/kexec.h
> > @@ -14,7 +14,6 @@
> >  
> >  #if !defined(__ASSEMBLY__)
> >  
> > -#include <linux/crash_core.h>
> >  #include <asm/io.h>
> >  
> >  #include <uapi/linux/kexec.h>
> > @@ -25,6 +24,7 @@
> >  #include <linux/ioport.h>
> >  #include <linux/module.h>
> >  #include <asm/kexec.h>
> > +#include <linux/crash_core.h>
> >  
> >  /* Verify architecture specific macros are defined */
> >  
> > @@ -63,15 +63,6 @@
> >  #define KEXEC_CORE_NOTE_NAME	CRASH_CORE_NOTE_NAME
> >  
> >  /*
> > - * The per-cpu notes area is a list of notes terminated by a "NULL"
> > - * note header.  For kdump, the code in vmcore.c runs in the context
> > - * of the second kernel to combine them into one note.
> > - */
> > -#ifndef KEXEC_NOTE_BYTES
> > -#define KEXEC_NOTE_BYTES	CRASH_CORE_NOTE_BYTES
> > -#endif
> 
> It is still not clear how does s390 use the crash_notes except this macro.
> But from code point of view we do need to update this as well after the
> crash_core splitting.
> 
> Acked-by: Dave Young <dyoung@...hat.com>

Hold on the ack because of the new findings, wait for Michael's
feedback.

Thanks
Dave