Re: [PATCH v5 1/5] vmcore: Introduce ELF header in new memory feature

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Jun 21, 2013 at 04:17:03PM +0200, Michael Holzheu wrote:
> On Fri, 14 Jun 2013 14:54:02 -0400
> Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > On Fri, Jun 07, 2013 at 06:55:57PM +0200, Michael Holzheu wrote:
> > 
> > [..]
> > > @@ -935,10 +967,17 @@ static int __init vmcore_init(void)
> > >  {
> > >  	int rc = 0;
> > >  
> > > -	/* If elfcorehdr= has been passed in cmdline, then capture the dump.*/
> > > -	if (!(is_vmcore_usable()))
> > > -		return rc;
> > > +	/*
> > > +	 * If elfcorehdr= has not been passed in cmdline, try to get the
> > > +	 * header from 2nd kernel, then capture the dump.
> > > +	 */
> > > +	if (!(is_vmcore_usable())) {
> > > +		rc = elfcorehdr_alloc();
> > > +		if (rc)
> > > +			return rc;
> > > +	}
> > 
> > Hi Michael,
> > 
> > Patch description says that elfcorehdr_alloc() returns the addr and 
> > size of elf headers. But that does not seem to be the case here. Has
> > it been modified in later patches.
> 
> Sorry, that is a relict of one of my previous experiments where I tried
> to implement elfcorehdr_addr() similar to the way as you suggest it now.
> Because elfcorehdr_addr is a global variable, I decided to not pass
> it in the functions. But of course I can change that again if you prefer
> that.
>  
> > Also will it be better if we call elfcorehdr_alloc() always and then
> > check for is_vmcore_usable().
> > 
> > Something like.
> > 
> > elfcorehdr_addr = elfcorehdr_alloc()
> > if (elfcorehdr_addr < )
> > 	return elfcorehdr_addr
> > 
> > if (!(is_vmcore_usable()))
> > 	return error
> 
> Ok, but then I think elfcorehdr_alloc() should also return
> the elfcorehdr_size.
> 
> So what about the following patch:

This patch looks good to me. There are little ugly pieces w.r.t not
setting elfcorehdr_addr null after calling free() as there are other
pieces depdendent on elfcorehdr_addr. I think sometime later we can
have a separate variable to track track whether this is kdump kernel
and remove dependency on elfcorehdr_addr being set.


So I am fine with this patch.

Vivek

> ---
>  fs/proc/vmcore.c           |   65 +++++++++++++++++++++++++++++++++++++--------
>  include/linux/crash_dump.h |    6 ++++
>  2 files changed, 60 insertions(+), 11 deletions(-)
> 
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -123,6 +123,36 @@ static ssize_t read_from_oldmem(char *bu
>  	return read;
>  }
>  
> +/*
> + * Architectures may override this function to allocate ELF header in 2nd kernel
> + */
> +int __weak elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
> +{
> +	return 0;
> +}
> +
> +/*
> + * Architectures may override this function to free header
> + */
> +void __weak elfcorehdr_free(unsigned long long addr)
> +{}
> +
> +/*
> + * Architectures may override this function to read from ELF header
> + */
> +ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
> +{
> +	return read_from_oldmem(buf, count, ppos, 0);
> +}
> +
> +/*
> + * Architectures may override this function to read from notes sections
> + */
> +ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
> +{
> +	return read_from_oldmem(buf, count, ppos, 0);
> +}
> +
>  /* Read from the ELF header and then the crash dump. On error, negative value is
>   * returned otherwise number of bytes read are returned.
>   */
> @@ -322,7 +352,7 @@ static int __init update_note_header_siz
>  		notes_section = kmalloc(max_sz, GFP_KERNEL);
>  		if (!notes_section)
>  			return -ENOMEM;
> -		rc = read_from_oldmem(notes_section, max_sz, &offset, 0);
> +		rc = elfcorehdr_read_notes(notes_section, max_sz, &offset);
>  		if (rc < 0) {
>  			kfree(notes_section);
>  			return rc;
> @@ -409,7 +439,8 @@ static int __init copy_notes_elf64(const
>  		if (phdr_ptr->p_type != PT_NOTE)
>  			continue;
>  		offset = phdr_ptr->p_offset;
> -		rc = read_from_oldmem(notes_buf, phdr_ptr->p_memsz, &offset, 0);
> +		rc = elfcorehdr_read_notes(notes_buf, phdr_ptr->p_memsz,
> +					   &offset);
>  		if (rc < 0)
>  			return rc;
>  		notes_buf += phdr_ptr->p_memsz;
> @@ -510,7 +541,7 @@ static int __init update_note_header_siz
>  		notes_section = kmalloc(max_sz, GFP_KERNEL);
>  		if (!notes_section)
>  			return -ENOMEM;
> -		rc = read_from_oldmem(notes_section, max_sz, &offset, 0);
> +		rc = elfcorehdr_read_notes(notes_section, max_sz, &offset);
>  		if (rc < 0) {
>  			kfree(notes_section);
>  			return rc;
> @@ -597,7 +628,8 @@ static int __init copy_notes_elf32(const
>  		if (phdr_ptr->p_type != PT_NOTE)
>  			continue;
>  		offset = phdr_ptr->p_offset;
> -		rc = read_from_oldmem(notes_buf, phdr_ptr->p_memsz, &offset, 0);
> +		rc = elfcorehdr_read_notes(notes_buf, phdr_ptr->p_memsz,
> +					   &offset);
>  		if (rc < 0)
>  			return rc;
>  		notes_buf += phdr_ptr->p_memsz;
> @@ -793,7 +825,7 @@ static int __init parse_crash_elf64_head
>  	addr = elfcorehdr_addr;
>  
>  	/* Read Elf header */
> -	rc = read_from_oldmem((char*)&ehdr, sizeof(Elf64_Ehdr), &addr, 0);
> +	rc = elfcorehdr_read((char *)&ehdr, sizeof(Elf64_Ehdr), &addr);
>  	if (rc < 0)
>  		return rc;
>  
> @@ -820,7 +852,7 @@ static int __init parse_crash_elf64_head
>  	if (!elfcorebuf)
>  		return -ENOMEM;
>  	addr = elfcorehdr_addr;
> -	rc = read_from_oldmem(elfcorebuf, elfcorebuf_sz_orig, &addr, 0);
> +	rc = elfcorehdr_read(elfcorebuf, elfcorebuf_sz_orig, &addr);
>  	if (rc < 0)
>  		goto fail;
>  
> @@ -849,7 +881,7 @@ static int __init parse_crash_elf32_head
>  	addr = elfcorehdr_addr;
>  
>  	/* Read Elf header */
> -	rc = read_from_oldmem((char*)&ehdr, sizeof(Elf32_Ehdr), &addr, 0);
> +	rc = elfcorehdr_read((char *)&ehdr, sizeof(Elf32_Ehdr), &addr);
>  	if (rc < 0)
>  		return rc;
>  
> @@ -875,7 +907,7 @@ static int __init parse_crash_elf32_head
>  	if (!elfcorebuf)
>  		return -ENOMEM;
>  	addr = elfcorehdr_addr;
> -	rc = read_from_oldmem(elfcorebuf, elfcorebuf_sz_orig, &addr, 0);
> +	rc = elfcorehdr_read(elfcorebuf, elfcorebuf_sz_orig, &addr);
>  	if (rc < 0)
>  		goto fail;
>  
> @@ -902,7 +934,7 @@ static int __init parse_crash_elf_header
>  	int rc=0;
>  
>  	addr = elfcorehdr_addr;
> -	rc = read_from_oldmem(e_ident, EI_NIDENT, &addr, 0);
> +	rc = elfcorehdr_read(e_ident, EI_NIDENT, &addr);
>  	if (rc < 0)
>  		return rc;
>  	if (memcmp(e_ident, ELFMAG, SELFMAG) != 0) {
> @@ -935,7 +967,14 @@ static int __init vmcore_init(void)
>  {
>  	int rc = 0;
>  
> -	/* If elfcorehdr= has been passed in cmdline, then capture the dump.*/
> +	/* Allow architectures to allocate ELF header in 2nd kernel */
> +	rc = elfcorehdr_alloc(&elfcorehdr_addr, &elfcorehdr_size);
> +	if (rc)
> +		return rc;
> +	/*
> +	 * If elfcorehdr= has been passed in cmdline or created in 2nd kernel,
> +	 * then capture the dump.
> +	 */
>  	if (!(is_vmcore_usable()))
>  		return rc;
>  	rc = parse_crash_elf_headers();
> @@ -943,7 +982,11 @@ static int __init vmcore_init(void)
>  		pr_warn("Kdump: vmcore not initialized\n");
>  		return rc;
>  	}
> -
> +	elfcorehdr_free(elfcorehdr_addr);
> +	/*
> +	 * elfcorehdr_addr must not be set to NULL here to keep
> +	 * is_kdump_kernel() working.
> +	 */
>  	proc_vmcore = proc_create("vmcore", S_IRUSR, NULL, &proc_vmcore_operations);
>  	if (proc_vmcore)
>  		proc_vmcore->size = vmcore_size;
> --- a/include/linux/crash_dump.h
> +++ b/include/linux/crash_dump.h
> @@ -12,6 +12,12 @@
>  extern unsigned long long elfcorehdr_addr;
>  extern unsigned long long elfcorehdr_size;
>  
> +extern int __weak elfcorehdr_alloc(unsigned long long *addr,
> +				   unsigned long long *size);
> +extern void __weak elfcorehdr_free(unsigned long long addr);
> +extern ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos);
> +extern ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos);
> +
>  extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
>  						unsigned long, int);
>