* Ingo Molnar <mi...@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 <mi...@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);
