Messages in this thread Patch in this message | | | Date | Thu, 28 May 2020 09:40:55 +0200 | From | Ingo Molnar <> | Subject | [PATCH v2] fs/coredump/elf: Clean up fill_thread_core_info() |
| |
* 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);
| |