| Message ID | 20200414070142.288696-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] powerpc/spufs: simplify spufs core dumping | expand |
On Tue, Apr 14, 2020 at 9:02 AM Christoph Hellwig <hch@lst.de> wrote: > > Instead of messing with the address limit just open code the trivial > memcpy + memset logic for the native version, and a call to > to_compat_siginfo for the compat version. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Nice! > */ > #define user_long_t compat_long_t > #define user_siginfo_t compat_siginfo_t > -#define copy_siginfo_to_user copy_siginfo_to_user32 > +#define fill_siginfo_note(note, csigdata, siginfo) \ > +do { \ > + to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \ > + fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata); \ > +} while (0) I don't think you are changing the behavior here, but I still wonder if it is in fact correct for x32: is in_x32_syscall() true here when dumping an x32 compat elf process, or should this rather be set according to which binfmt_elf copy is being used? Arnd
Christoph Hellwig <hch@lst.de> writes: > Instead of messing with the address limit just open code the trivial > memcpy + memset logic for the native version, and a call to > to_compat_siginfo for the compat version. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/binfmt_elf.c | 9 +++++---- > fs/compat_binfmt_elf.c | 6 +++++- > 2 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 13f25e241ac4..607c5a5f855e 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -1553,15 +1553,16 @@ static void fill_auxv_note(struct memelfnote *note, struct mm_struct *mm) > fill_note(note, "CORE", NT_AUXV, i * sizeof(elf_addr_t), auxv); > } > > +#ifndef fill_siginfo_note > static void fill_siginfo_note(struct memelfnote *note, user_siginfo_t *csigdata, > const kernel_siginfo_t *siginfo) > { > - mm_segment_t old_fs = get_fs(); > - set_fs(KERNEL_DS); > - copy_siginfo_to_user((user_siginfo_t __user *) csigdata, siginfo); > - set_fs(old_fs); > + memcpy(csigdata, siginfo, sizeof(struct kernel_siginfo)); > + memset((char *)csigdata + sizeof(struct kernel_siginfo), 0, > + SI_EXPANSION_SIZE); > fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata); > } > +#endif > > #define MAX_FILE_NOTE_SIZE (4*1024*1024) > /* > diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c > index aaad4ca1217e..ab84e095618b 100644 > --- a/fs/compat_binfmt_elf.c > +++ b/fs/compat_binfmt_elf.c > @@ -39,7 +39,11 @@ > */ > #define user_long_t compat_long_t > #define user_siginfo_t compat_siginfo_t > -#define copy_siginfo_to_user copy_siginfo_to_user32 > +#define fill_siginfo_note(note, csigdata, siginfo) \ > +do { \ > + to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \ > + fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata); \ > +} while (0) This doesn't build on ppc (cell_defconfig): ../fs/binfmt_elf.c: In function 'fill_note_info': ../fs/compat_binfmt_elf.c:44:39: error: implicit declaration of function 'compat_siginfo_flags'; did you mean 'to_compat_siginfo'? [-Werror=implicit-function-d eclaration] to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \ ^~~~~~~~~~~~~~~~~~~~ ../fs/binfmt_elf.c:1846:2: note: in expansion of macro 'fill_siginfo_note' fill_siginfo_note(&info->signote, &info->csigdata, siginfo); ^~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors make[2]: *** [../scripts/Makefile.build:266: fs/compat_binfmt_elf.o] Error 1 I guess the empty version from kernel/signal.c needs to move into a header somewhere. cheers
On Wed, Apr 15, 2020 at 01:01:59PM +1000, Michael Ellerman wrote: > > + to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \ > > + fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata); \ > > +} while (0) > > This doesn't build on ppc (cell_defconfig): > > ../fs/binfmt_elf.c: In function 'fill_note_info': > ../fs/compat_binfmt_elf.c:44:39: error: implicit declaration of function 'compat_siginfo_flags'; did you mean 'to_compat_siginfo'? [-Werror=implicit-function-d > eclaration] > to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \ > ^~~~~~~~~~~~~~~~~~~~ > ../fs/binfmt_elf.c:1846:2: note: in expansion of macro 'fill_siginfo_note' > fill_siginfo_note(&info->signote, &info->csigdata, siginfo); > ^~~~~~~~~~~~~~~~~ > cc1: some warnings being treated as errors > make[2]: *** [../scripts/Makefile.build:266: fs/compat_binfmt_elf.o] Error 1 > > > I guess the empty version from kernel/signal.c needs to move into a > header somewhere. Yes, it should. Odd that the buildbut hasn't complained yet so far..
On Tue, Apr 14, 2020 at 03:15:09PM +0200, Arnd Bergmann wrote: > I don't think you are changing the behavior here, but I still wonder if it > is in fact correct for x32: is in_x32_syscall() true here when dumping an > x32 compat elf process, or should this rather be set according to which > binfmt_elf copy is being used? The infrastructure cold enable that, although it would require more arch hooks I think. I'd rather keep it out of this series and to an interested party. Then again x32 doesn't seem to have a whole lot of interested parties..
On Wed, Apr 15, 2020 at 9:45 AM Christoph Hellwig <hch@lst.de> wrote: > > On Tue, Apr 14, 2020 at 03:15:09PM +0200, Arnd Bergmann wrote: > > I don't think you are changing the behavior here, but I still wonder if it > > is in fact correct for x32: is in_x32_syscall() true here when dumping an > > x32 compat elf process, or should this rather be set according to which > > binfmt_elf copy is being used? > > The infrastructure could enable that, although it would require more > arch hooks I think. I was more interested in whether you can tell if it's currently broken or not. If my feeling is right that the current code does the wrong thing here, it would be good to at least put a FIXME comment in there. > I'd rather keep it out of this series and to > an interested party. Then again x32 doesn't seem to have a whole lot > of interested parties.. Fine with me. It's on my mental list of things that we want to kill off eventually as soon as the remaining users stop replying to questions about it. In fact I should really turn that into a properly maintained list in Documentation/... that contains any options that someone has asked about removing in the past, along with the reasons for keeping it around and a time at which we should ask about it again. Arnd
On Wed, Apr 15, 2020 at 10:20:11AM +0200, Arnd Bergmann wrote: > > I'd rather keep it out of this series and to > > an interested party. Then again x32 doesn't seem to have a whole lot > > of interested parties.. > > Fine with me. It's on my mental list of things that we want to kill off > eventually as soon as the remaining users stop replying to questions > about it. > > In fact I should really turn that into a properly maintained list in > Documentation/... that contains any options that someone has > asked about removing in the past, along with the reasons for keeping > it around and a time at which we should ask about it again. To the newly added x86 maintainers: Arnd brought up the point that elf_core_dump writes the ABI siginfo format into the core dump. That format differs for i386 vs x32. Is there any good way to find out which is the right format when are not in a syscall? As far a I can tell x32 vs i386 just seems to be based around what syscall table was used for the current syscall, but core dumps aren't always in syscall context.
Christoph Hellwig <hch@lst.de> writes: > On Wed, Apr 15, 2020 at 10:20:11AM +0200, Arnd Bergmann wrote: >> > I'd rather keep it out of this series and to >> > an interested party. Then again x32 doesn't seem to have a whole lot >> > of interested parties.. >> >> Fine with me. It's on my mental list of things that we want to kill off >> eventually as soon as the remaining users stop replying to questions >> about it. >> >> In fact I should really turn that into a properly maintained list in >> Documentation/... that contains any options that someone has >> asked about removing in the past, along with the reasons for keeping >> it around and a time at which we should ask about it again. > > To the newly added x86 maintainers: Arnd brought up the point that > elf_core_dump writes the ABI siginfo format into the core dump. That > format differs for i386 vs x32. Is there any good way to find out > which is the right format when are not in a syscall? > > As far a I can tell x32 vs i386 just seems to be based around what > syscall table was used for the current syscall, but core dumps aren't > always in syscall context. I don't think this matters. The i386 and x32 signal structures only differ for SIGCHLD. The SIGCHLD signal does cause coredumps. So as long as we get the 32bit vs 64bit distinct correct all should be well. Eric
On Fri, Apr 17, 2020 at 8:13 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Christoph Hellwig <hch@lst.de> writes: > > > On Wed, Apr 15, 2020 at 10:20:11AM +0200, Arnd Bergmann wrote: > >> > I'd rather keep it out of this series and to > >> > an interested party. Then again x32 doesn't seem to have a whole lot > >> > of interested parties.. > >> > >> Fine with me. It's on my mental list of things that we want to kill off > >> eventually as soon as the remaining users stop replying to questions > >> about it. > >> > >> In fact I should really turn that into a properly maintained list in > >> Documentation/... that contains any options that someone has > >> asked about removing in the past, along with the reasons for keeping > >> it around and a time at which we should ask about it again. > > > > To the newly added x86 maintainers: Arnd brought up the point that > > elf_core_dump writes the ABI siginfo format into the core dump. That > > format differs for i386 vs x32. Is there any good way to find out > > which is the right format when are not in a syscall? > > > > As far a I can tell x32 vs i386 just seems to be based around what > > syscall table was used for the current syscall, but core dumps aren't > > always in syscall context. > > I don't think this matters. The i386 and x32 signal structures > only differ for SIGCHLD. The SIGCHLD signal does cause coredumps. > So as long as we get the 32bit vs 64bit distinct correct all should be > well. Ok, makes sense. Thanks for taking a look into this! Arnd
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 13f25e241ac4..607c5a5f855e 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1553,15 +1553,16 @@ static void fill_auxv_note(struct memelfnote *note, struct mm_struct *mm) fill_note(note, "CORE", NT_AUXV, i * sizeof(elf_addr_t), auxv); } +#ifndef fill_siginfo_note static void fill_siginfo_note(struct memelfnote *note, user_siginfo_t *csigdata, const kernel_siginfo_t *siginfo) { - mm_segment_t old_fs = get_fs(); - set_fs(KERNEL_DS); - copy_siginfo_to_user((user_siginfo_t __user *) csigdata, siginfo); - set_fs(old_fs); + memcpy(csigdata, siginfo, sizeof(struct kernel_siginfo)); + memset((char *)csigdata + sizeof(struct kernel_siginfo), 0, + SI_EXPANSION_SIZE); fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata); } +#endif #define MAX_FILE_NOTE_SIZE (4*1024*1024) /* diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c index aaad4ca1217e..ab84e095618b 100644 --- a/fs/compat_binfmt_elf.c +++ b/fs/compat_binfmt_elf.c @@ -39,7 +39,11 @@ */ #define user_long_t compat_long_t #define user_siginfo_t compat_siginfo_t -#define copy_siginfo_to_user copy_siginfo_to_user32 +#define fill_siginfo_note(note, csigdata, siginfo) \ +do { \ + to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \ + fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata); \ +} while (0) /* * The machine-dependent core note format types are defined in elfcore-compat.h,
Instead of messing with the address limit just open code the trivial memcpy + memset logic for the native version, and a call to to_compat_siginfo for the compat version. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/binfmt_elf.c | 9 +++++---- fs/compat_binfmt_elf.c | 6 +++++- 2 files changed, 10 insertions(+), 5 deletions(-)