| Message ID | 87h7pdnlzv.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] coredump: Document coredump code exclusively used by cell spufs | expand |
ebiederm@xmission.com (Eric W. Biederman) writes: > Oleg Nesterov recently asked[1] why is there an unshare_files in > do_coredump. After digging through all of the callers of lookup_fd it > turns out that it is > arch/powerpc/platforms/cell/spufs/coredump.c:coredump_next_context > that needs the unshare_files in do_coredump. > > Looking at the history[2] this code was also the only piece of coredump code > that required the unshare_files when the unshare_files was added. > > Looking at that code it turns out that cell is also the only > architecture that implements elf_coredump_extra_notes_size and > elf_coredump_extra_notes_write. > > I looked at the gdb repo[3] support for cell has been removed[4] in binutils > 2.34. Geoff Levand reports he is still getting questions on how to > run modern kernels on the PS3, from people using 3rd party firmware so > this code is not dead. According to Wikipedia the last PS3 shipped in > Japan sometime in 2017. So it will probably be a little while before > everyone's hardware dies. > > Add some comments briefly documenting the coredump code that exists > only to support cell spufs to make it easier to understand the > coredump code. Eventually the hardware will be dead, or their won't > be userspace tools, or the coredump code will be refactored and it > will be too difficult to update a dead architecture and these comments > make it easy to tell where to pull to remove cell spufs support. > > [1] https://lkml.kernel.org/r/20201123175052.GA20279@redhat.com > [2] 179e037fc137 ("do_coredump(): make sure that descriptor table isn't shared") > [3] git://sourceware.org/git/binutils-gdb.git > [4] abf516c6931a ("Remove Cell Broadband Engine debugging support"). > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > --- > > Does this change look good to people? I think it captures this state of > things and makes things clearer without breaking anything or removing > functionality for anyone. I haven't heard anything except a general ack to the concept of comments. So I am applying this. Eric > > fs/binfmt_elf.c | 2 ++ > fs/coredump.c | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index b6b3d052ca86..c1996f0aeaed 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -2198,6 +2198,7 @@ static int elf_core_dump(struct coredump_params *cprm) > { > size_t sz = get_note_info_size(&info); > > + /* For cell spufs */ > sz += elf_coredump_extra_notes_size(); > > phdr4note = kmalloc(sizeof(*phdr4note), GFP_KERNEL); > @@ -2261,6 +2262,7 @@ static int elf_core_dump(struct coredump_params *cprm) > if (!write_note_info(&info, cprm)) > goto end_coredump; > > + /* For cell spufs */ > if (elf_coredump_extra_notes_write(cprm)) > goto end_coredump; > > diff --git a/fs/coredump.c b/fs/coredump.c > index abf807235262..3ff17eea812e 100644 > --- a/fs/coredump.c > +++ b/fs/coredump.c > @@ -790,6 +790,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) > } > > /* get us an unshared descriptor table; almost always a no-op */ > + /* The cell spufs coredump code reads the file descriptor tables */ > retval = unshare_files(); > if (retval) > goto close_fail;
On Wed, Dec 2, 2020 at 4:20 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > ebiederm@xmission.com (Eric W. Biederman) writes: > > > Oleg Nesterov recently asked[1] why is there an unshare_files in > > do_coredump. After digging through all of the callers of lookup_fd it > > turns out that it is > > arch/powerpc/platforms/cell/spufs/coredump.c:coredump_next_context > > that needs the unshare_files in do_coredump. > > > > Looking at the history[2] this code was also the only piece of coredump code > > that required the unshare_files when the unshare_files was added. > > > > Looking at that code it turns out that cell is also the only > > architecture that implements elf_coredump_extra_notes_size and > > elf_coredump_extra_notes_write. > > > > I looked at the gdb repo[3] support for cell has been removed[4] in binutils > > 2.34. Geoff Levand reports he is still getting questions on how to > > run modern kernels on the PS3, from people using 3rd party firmware so > > this code is not dead. According to Wikipedia the last PS3 shipped in > > Japan sometime in 2017. So it will probably be a little while before > > everyone's hardware dies. > > > > Add some comments briefly documenting the coredump code that exists > > only to support cell spufs to make it easier to understand the > > coredump code. Eventually the hardware will be dead, or their won't > > be userspace tools, or the coredump code will be refactored and it > > will be too difficult to update a dead architecture and these comments > > make it easy to tell where to pull to remove cell spufs support. > > > > [1] https://lkml.kernel.org/r/20201123175052.GA20279@redhat.com > > [2] 179e037fc137 ("do_coredump(): make sure that descriptor table isn't shared") > > [3] git://sourceware.org/git/binutils-gdb.git > > [4] abf516c6931a ("Remove Cell Broadband Engine debugging support"). > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > > --- > > > > Does this change look good to people? I think it captures this state of > > things and makes things clearer without breaking anything or removing > > functionality for anyone. > > I haven't heard anything except a general ack to the concept of > comments. So I am applying this. > Sorry I missed it when you originally sent it. Looks good ot me, Acked-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index b6b3d052ca86..c1996f0aeaed 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -2198,6 +2198,7 @@ static int elf_core_dump(struct coredump_params *cprm) { size_t sz = get_note_info_size(&info); + /* For cell spufs */ sz += elf_coredump_extra_notes_size(); phdr4note = kmalloc(sizeof(*phdr4note), GFP_KERNEL); @@ -2261,6 +2262,7 @@ static int elf_core_dump(struct coredump_params *cprm) if (!write_note_info(&info, cprm)) goto end_coredump; + /* For cell spufs */ if (elf_coredump_extra_notes_write(cprm)) goto end_coredump; diff --git a/fs/coredump.c b/fs/coredump.c index abf807235262..3ff17eea812e 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -790,6 +790,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) } /* get us an unshared descriptor table; almost always a no-op */ + /* The cell spufs coredump code reads the file descriptor tables */ retval = unshare_files(); if (retval) goto close_fail;
Oleg Nesterov recently asked[1] why is there an unshare_files in do_coredump. After digging through all of the callers of lookup_fd it turns out that it is arch/powerpc/platforms/cell/spufs/coredump.c:coredump_next_context that needs the unshare_files in do_coredump. Looking at the history[2] this code was also the only piece of coredump code that required the unshare_files when the unshare_files was added. Looking at that code it turns out that cell is also the only architecture that implements elf_coredump_extra_notes_size and elf_coredump_extra_notes_write. I looked at the gdb repo[3] support for cell has been removed[4] in binutils 2.34. Geoff Levand reports he is still getting questions on how to run modern kernels on the PS3, from people using 3rd party firmware so this code is not dead. According to Wikipedia the last PS3 shipped in Japan sometime in 2017. So it will probably be a little while before everyone's hardware dies. Add some comments briefly documenting the coredump code that exists only to support cell spufs to make it easier to understand the coredump code. Eventually the hardware will be dead, or their won't be userspace tools, or the coredump code will be refactored and it will be too difficult to update a dead architecture and these comments make it easy to tell where to pull to remove cell spufs support. [1] https://lkml.kernel.org/r/20201123175052.GA20279@redhat.com [2] 179e037fc137 ("do_coredump(): make sure that descriptor table isn't shared") [3] git://sourceware.org/git/binutils-gdb.git [4] abf516c6931a ("Remove Cell Broadband Engine debugging support"). Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- Does this change look good to people? I think it captures this state of things and makes things clearer without breaking anything or removing functionality for anyone. fs/binfmt_elf.c | 2 ++ fs/coredump.c | 1 + 2 files changed, 3 insertions(+)