From: Ingo Molnar <mingo@kernel.org>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
linux-kernel@vger.kernel.org, x86@kernel.org
Subject: [PATCH v2] fs/coredump/elf: Clean up fill_thread_core_info()
Date: Thu, 28 May 2020 09:40:55 +0200 [thread overview]
Message-ID: <20200528074055.GA111020@gmail.com> (raw)
In-Reply-To: <20200528072934.GA3663052@gmail.com>
* Ingo Molnar <mingo@kernel.org> wrote:
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 13f25e241ac4..25d489bc9453 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -1733,7 +1733,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
> > (!regset->active || regset->active(t->task, regset) > 0)) {
> > int ret;
> > size_t size = regset_size(t->task, regset);
> > - void *data = kmalloc(size, GFP_KERNEL);
> > + void *data = kzalloc(size, GFP_KERNEL);
> > if (unlikely(!data))
> > return 0;
> > ret = regset->get(t->task, regset,
>
> The clean-up patch below on top of the zeroing patch above makes
> fill_thread_core_info() readable for me:
>
> - Use a proper iterator pattern and merge the special case '0' into
> the 1..n-1 iterator.
>
> - Clean up the flow of logic in the iterator to more standard
> patterns, to see the progress of work versus a mix of uncommon
> failure causes with the typical branch.
>
> - Add a WARN_ON_ONCE() for a silent assumption about NT_PRSTATUS
> semantics.
>
> - Get rid of a copious amount of col80 line breaks created by
> copy & paste of overly verbose repetitive patterns.
>
> - Clean up small details like 10 year old "fill the reset" typos in
> comments, unbalanced curly braces, etc.
>
> Now that the compiler can see what we are doing the code likely got a
> tiny bit faster as well, because the code shrunk a bit if we discount
> the extra WARN_ON_ONCE():
>
> # fs/binfmt_elf.o:
>
> text data bss dec hex filename
> 14410 108 0 14518 38b6 binfmt_elf.o.before
> 14381 108 0 14489 3899 binfmt_elf.o.after
>
> (Assuming it's not due to a bug - this is untested.)
>
> Thanks,
>
> Ingo
>
> Signed-off-by-if-you-first-test-it: Ingo Molnar <mingo@kernel.org>
> + SET_PR_FPVALID(&t->prstatus, 1, regset0_size);
Meh, I broke the x86-32 build with this, in part because on 64-bit
SET_PR_FPVALID() silently ignores the third argument.
The patch below, folded into the cleanup patch, does the following:
- fixes the bug I introduced.
- makes SET_PR_FPVALID() use all three arguments on 64-bit systems
too, to keep dorks like me from breaking the code.
- fixes a minor macro assumption in arch/x86/include/asm/compat.h
Still an overall win, if we compare it without the WARN_ON():
# fs/binfmt_elf.o:
text data bss dec hex filename
14410 108 0 14518 38b6 binfmt_elf.o.before
14381 108 0 14489 3899 binfmt_elf.o.after
Thanks,
Ingo
---
arch/x86/include/asm/compat.h | 2 +-
fs/binfmt_elf.c | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
index 52e9f3480f69..2f5ff3c3416b 100644
--- a/arch/x86/include/asm/compat.h
+++ b/arch/x86/include/asm/compat.h
@@ -169,7 +169,7 @@ typedef struct user_regs_struct compat_elf_gregset_t;
/* Full regset -- prstatus on x32, otherwise on ia32 */
#define PRSTATUS_SIZE(S, R) (R != sizeof(S.pr_reg) ? 144 : 296)
#define SET_PR_FPVALID(S, V, R) \
- do { *(int *) (((void *) &((S)->pr_reg)) + R) = (V); } \
+ do { *(int *) (((void *) &((S)->pr_reg)) + (R)) = (V); } \
while (0)
#ifdef CONFIG_X86_X32_ABI
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 3f9f299169fd..bc347179df0f 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1695,7 +1695,7 @@ static void do_thread_regset_writeback(struct task_struct *task,
#endif
#ifndef SET_PR_FPVALID
-#define SET_PR_FPVALID(S, V, R) ((S)->pr_fpvalid = (V))
+#define SET_PR_FPVALID(S, V, R) ((void)(R), (S)->pr_fpvalid = (V))
#endif
static int fill_thread_core_info(struct elf_thread_core_info *t,
@@ -1705,7 +1705,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
unsigned int i;
const struct user_regset *regset = &view->regsets[0];
struct memelfnote *note = &t->notes[0];
- unsigned int size = regset_size(t->task, regset);
+ unsigned int size, size0 = regset_size(t->task, regset);
int ret;
/*
@@ -1715,11 +1715,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
* We assume that regset 0 is NT_PRSTATUS.
*/
fill_prstatus(&t->prstatus, t->task, signr);
- ret = regset->get(t->task, regset, 0, size, &t->prstatus.pr_reg, NULL);
+ ret = regset->get(t->task, regset, 0, size0, &t->prstatus.pr_reg, NULL);
/* NT_PRSTATUS is not supposed to fail: */
WARN_ON_ONCE(ret);
- fill_note(note, "CORE", NT_PRSTATUS, PRSTATUS_SIZE(t->prstatus, size), &t->prstatus);
+ fill_note(note, "CORE", NT_PRSTATUS, PRSTATUS_SIZE(t->prstatus, size0), &t->prstatus);
*total += notesize(note);
do_thread_regset_writeback(t->task, &view->regsets[0]);
@@ -1754,7 +1754,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
if (regset->core_note_type != NT_PRFPREG) {
fill_note(note, "LINUX", regset->core_note_type, size, data);
} else {
- SET_PR_FPVALID(&t->prstatus, 1, regset0_size);
+ SET_PR_FPVALID(&t->prstatus, 1, size0);
fill_note(note, "CORE", NT_PRFPREG, size, data);
}
*total += notesize(note);
next prev parent reply other threads:[~2020-05-28 7:41 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-27 21:34 [git pull] coredump infoleak fix Al Viro
2020-05-28 7:02 ` Ingo Molnar
2020-05-28 7:05 ` Al Viro
2020-05-28 7:44 ` Ingo Molnar
2020-05-28 12:50 ` Al Viro
2020-05-29 9:35 ` Ingo Molnar
2020-05-28 7:29 ` [PATCH] fs/coredump/elf: Clean up fill_thread_core_info() Ingo Molnar
2020-05-28 7:40 ` Ingo Molnar [this message]
2020-05-28 18:34 ` [git pull] coredump infoleak fix Linus Torvalds
2020-05-28 19:05 ` Al Viro
2020-05-28 19:09 ` Linus Torvalds
2020-05-28 19:17 ` Al Viro
2020-05-28 19:19 ` Linus Torvalds
2020-05-28 19:28 ` Al Viro
2020-05-29 9:39 ` Ingo Molnar
2020-05-31 18:05 ` pr-tracker-bot
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=20200528074055.GA111020@gmail.com \
--to=mingo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=viro@zeniv.linux.org.uk \
--cc=x86@kernel.org \
/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).