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
next prev parent 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).