LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
Cc: netdev@vger.kernel.org, kexec@lists.infradead.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	davem@davemloft.net, viro@zeniv.linux.org.uk,
	stephen@networkplumber.org, akpm@linux-foundation.org,
	torvalds@linux-foundation.org, ganeshgr@chelsio.com,
	nirranjan@chelsio.com, indranil@chelsio.com
Subject: Re: [PATCH net-next v5 1/3] vmcore: add API to collect hardware dump in second kernel
Date: Thu, 26 Apr 2018 20:30:58 -0500	[thread overview]
Message-ID: <871sf1a10t.fsf@xmission.com> (raw)
In-Reply-To: <1393d2fce1a2c2a0f78431845deae26bc28c16ef.1524329561.git.rahul.lakkireddy@chelsio.com> (Rahul Lakkireddy's message of "Sat, 21 Apr 2018 22:35:53 +0530")


While looking this over I found a bug in the way elf notes are being composed.

Rahul Lakkireddy <rahul.lakkireddy@chelsio.com> writes:
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index a45f0af22a60..7395462d2f86 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -1145,6 +1150,132 @@ static int __init parse_crash_elf_headers(void)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PROC_VMCORE_DEVICE_DUMP
> +/**
> + * vmcoredd_get_note_size - Get size of the note that will be inserted at
> + * beginning of the dump's buffer.
> + * @name: Note's name
> + *
> + * Gets the overall size of the note that will be inserted at the beginning
> + * of the dump's buffer.  It also adds padding, if necessary to meet
> + * alignment requirements.
> + */
> +static inline size_t vmcoredd_get_note_size(const char *name)
> +{
> +	return CRASH_CORE_NOTE_HEAD_BYTES +
> +	       ALIGN(VMCOREDD_NOTE_NAME_BYTES + strlen(name), sizeof(Elf_Word));
> +}
> +
> +/**
> + * vmcoredd_write_note - Write note at the beginning of the dump's buffer
> + * @name: Dump's name
> + * @buf: Output buffer where the note is written
> + * @size: Size of the dump
> + *
> + * Fills beginning of the dump's data with elf note.
> + */
> +static void vmcoredd_write_note(const char *name, void *buf, size_t size)
> +{
> +	struct elf_note *note = (struct elf_note *)buf;
> +	Elf_Word *word = (Elf_Word *)note;
> +
> +	note->n_namesz = ALIGN(VMCOREDD_NOTE_NAME_BYTES + strlen(name),
> +			       sizeof(Elf_Word));
> +	note->n_descsz = size;
> +	note->n_type = NT_VMCOREDD;
> +	word += DIV_ROUND_UP(sizeof(*note), sizeof(Elf_Word));
> +	snprintf((char *)word, note->n_namesz, "%s_%s", VMCOREDD_NOTE_NAME,
> +		 name);

I hate to do this to you but as this is ABI I am going to pick on
this bit of code.

First namesz needs to include the '\0' of the name string.
Second you did not count the length of "_" namesz.
Third name needs to be a vendor identifier.  So "LINUX\0\0\0" in our case.

Which means the device name needs to be in the body of the note.
Perhaps just reserve 32 bytes for the device name?
Perhaps prefix the device name with a length?

The exact layout is whatever you want NT_VMCOREDD to mean.

> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index e2535d6dcec7..4e12c423b9fe 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -421,6 +421,7 @@ typedef struct elf64_shdr {
>  #define NT_ARM_SYSTEM_CALL	0x404	/* ARM system call number */
>  #define NT_ARM_SVE	0x405		/* ARM Scalable Vector Extension registers */
>  #define NT_ARC_V2	0x600		/* ARCv2 accumulator/extra registers */
> +#define NT_VMCOREDD	0x700		/* Vmcore Device Dump Note */
>  
>  /* Note header in a PT_NOTE section */
>  typedef struct elf32_note {


Eric

  reply	other threads:[~2018-04-27  1:31 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-21 17:05 [PATCH net-next v5 0/3] kernel: add support to collect hardware logs in crash recovery kernel Rahul Lakkireddy
2018-04-21 17:05 ` [PATCH net-next v5 1/3] vmcore: add API to collect hardware dump in second kernel Rahul Lakkireddy
2018-04-27  1:30   ` Eric W. Biederman [this message]
2018-04-21 17:05 ` [PATCH net-next v5 2/3] vmcore: append device dumps to vmcore as elf notes Rahul Lakkireddy
2018-04-21 17:05 ` [PATCH net-next v5 3/3] cxgb4: collect hardware dump in second kernel Rahul Lakkireddy
2018-04-24 19:54 ` [PATCH net-next v5 0/3] kernel: add support to collect hardware logs in crash recovery kernel David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871sf1a10t.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=akpm@linux-foundation.org \
    --cc=davem@davemloft.net \
    --cc=ganeshgr@chelsio.com \
    --cc=indranil@chelsio.com \
    --cc=kexec@lists.infradead.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nirranjan@chelsio.com \
    --cc=rahul.lakkireddy@chelsio.com \
    --cc=stephen@networkplumber.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH net-next v5 1/3] vmcore: add API to collect hardware dump in second kernel' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).