Now that exec no longer needs to return the unshared files to their
previous value there is no reason to return displaced.
Instead when unshare_fd creates a copy of the file table, call
put_files_struct before returning from unshare_files.
Acked-by: Christian Brauner <[email protected]>
v1: https://lkml.kernel.org/r/[email protected]
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/coredump.c | 5 +----
fs/exec.c | 5 +----
include/linux/fdtable.h | 2 +-
kernel/fork.c | 12 ++++++------
4 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index 0cd9056d79cc..abf807235262 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -585,7 +585,6 @@ void do_coredump(const kernel_siginfo_t *siginfo)
int ispipe;
size_t *argv = NULL;
int argc = 0;
- struct files_struct *displaced;
/* require nonrelative corefile path and be extra careful */
bool need_suid_safe = false;
bool core_dumped = false;
@@ -791,11 +790,9 @@ void do_coredump(const kernel_siginfo_t *siginfo)
}
/* get us an unshared descriptor table; almost always a no-op */
- retval = unshare_files(&displaced);
+ retval = unshare_files();
if (retval)
goto close_fail;
- if (displaced)
- put_files_struct(displaced);
if (!dump_interrupted()) {
/*
* umh disabled with CONFIG_STATIC_USERMODEHELPER_PATH="" would
diff --git a/fs/exec.c b/fs/exec.c
index fa788988efe9..14fae2ec1c9d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1238,7 +1238,6 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
int begin_new_exec(struct linux_binprm * bprm)
{
struct task_struct *me = current;
- struct files_struct *displaced;
int retval;
/* Once we are committed compute the creds */
@@ -1259,11 +1258,9 @@ int begin_new_exec(struct linux_binprm * bprm)
goto out;
/* Ensure the files table is not shared. */
- retval = unshare_files(&displaced);
+ retval = unshare_files();
if (retval)
goto out;
- if (displaced)
- put_files_struct(displaced);
/*
* Must be called _before_ exec_mmap() as bprm->mm is
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index a32bf47c593e..f46a084b60b2 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -109,7 +109,7 @@ struct task_struct;
struct files_struct *get_files_struct(struct task_struct *);
void put_files_struct(struct files_struct *fs);
void reset_files_struct(struct files_struct *);
-int unshare_files(struct files_struct **);
+int unshare_files(void);
struct files_struct *dup_fd(struct files_struct *, unsigned, int *) __latent_entropy;
void do_close_on_exec(struct files_struct *);
int iterate_fd(struct files_struct *, unsigned,
diff --git a/kernel/fork.c b/kernel/fork.c
index 32083db7a2a2..837b546528c8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -3023,21 +3023,21 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
* the exec layer of the kernel.
*/
-int unshare_files(struct files_struct **displaced)
+int unshare_files(void)
{
struct task_struct *task = current;
- struct files_struct *copy = NULL;
+ struct files_struct *old, *copy = NULL;
int error;
error = unshare_fd(CLONE_FILES, NR_OPEN_MAX, ©);
- if (error || !copy) {
- *displaced = NULL;
+ if (error || !copy)
return error;
- }
- *displaced = task->files;
+
+ old = task->files;
task_lock(task);
task->files = copy;
task_unlock(task);
+ put_files_struct(old);
return 0;
}
--
2.25.0
I'll try to actually read this series tomorrow but I see nothing wrong
after the quick glance.
Just one off-topic question,
On 11/20, Eric W. Biederman wrote:
>
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -585,7 +585,6 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> int ispipe;
> size_t *argv = NULL;
> int argc = 0;
> - struct files_struct *displaced;
> /* require nonrelative corefile path and be extra careful */
> bool need_suid_safe = false;
> bool core_dumped = false;
> @@ -791,11 +790,9 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> }
>
> /* get us an unshared descriptor table; almost always a no-op */
> - retval = unshare_files(&displaced);
> + retval = unshare_files();
Can anyone explain why does do_coredump() need unshare_files() at all?
Oleg.
On Mon, Nov 23, 2020 at 9:52 AM Oleg Nesterov <[email protected]> wrote:
>
> Can anyone explain why does do_coredump() need unshare_files() at all?
Hmm. It goes back to 2012, and it's placed just before calling
"->core_dump()", so I assume some core dumping function messed with
the file table back when..
I can't see anything like that currently.
The alternative is that core-dumping just keeps the file table around
for a long while, and thus files don't actually close in a timely
manner. So it might not be a "correctness" issue as much as a latency
issue.
Linus
On Tue, Nov 24, 2020 at 8:58 PM Linus Torvalds
<[email protected]> wrote:
>
> On Tue, Nov 24, 2020 at 11:55 AM Eric W. Biederman
> <[email protected]> wrote:
> >
> > If cell happens to be dead we can remove a fair amount of generic kernel
> > code that only exists to support cell.
>
> Even if some people might still use cell (which sounds unlikely), I
> think we can remove the spu core dumping code.
The Cell blade hardware (arch/powerpc/platforms/cell/) that I'm listed
as a maintainer for is very much dead, but there is apparently still some
activity on the Playstation 3 that Geoff Levand maintains.
Eric correctly points out that the PS3 firmware no longer boots
Linux (OtherOS), but AFAIK there are both users with old firmware
and those that use a firmware exploit to run homebrew code including
Linux.
I would assume they still use the SPU and might also use the core
dump code in particular. Let's see what Geoff thinks.
Arnd
On Tue, Nov 24, 2020 at 11:55 AM Eric W. Biederman
<[email protected]> wrote:
>
> If cell happens to be dead we can remove a fair amount of generic kernel
> code that only exists to support cell.
Even if some people might still use cell (which sounds unlikely), I
think we can remove the spu core dumping code.
Linus
On 11/24/20 12:14 PM, Arnd Bergmann wrote:
> On Tue, Nov 24, 2020 at 8:58 PM Linus Torvalds
> <[email protected]> wrote:
>>
>> On Tue, Nov 24, 2020 at 11:55 AM Eric W. Biederman
>> <[email protected]> wrote:
>>>
>>> If cell happens to be dead we can remove a fair amount of generic kernel
>>> code that only exists to support cell.
>>
>> Even if some people might still use cell (which sounds unlikely), I
>> think we can remove the spu core dumping code.
>
> The Cell blade hardware (arch/powerpc/platforms/cell/) that I'm listed
> as a maintainer for is very much dead, but there is apparently still some
> activity on the Playstation 3 that Geoff Levand maintains.
>
> Eric correctly points out that the PS3 firmware no longer boots
> Linux (OtherOS), but AFAIK there are both users with old firmware
> and those that use a firmware exploit to run homebrew code including
> Linux.
>
> I would assume they still use the SPU and might also use the core
> dump code in particular. Let's see what Geoff thinks.
There are still PS3-Linux users out there. They use 'Homebrew' firmware
released through 'Hacker' forums that allow them to run Linux on
non-supported systems. They are generally hobbies who don't post to
Linux kernel mailing lists. I get direct inquiries regularly asking
about how to update to a recent kernel. One of the things that attract
them to the PS3 is the Cell processor and either using or programming
the SPUs.
It is difficult to judge how much use the SPU core dump support gets,
but if it is not a cause of major problems I feel we should consider
keeping it.
-Geoff
Geoff Levand <[email protected]> writes:
> On 11/24/20 12:14 PM, Arnd Bergmann wrote:
>> On Tue, Nov 24, 2020 at 8:58 PM Linus Torvalds
>> <[email protected]> wrote:
>>>
>>> On Tue, Nov 24, 2020 at 11:55 AM Eric W. Biederman
>>> <[email protected]> wrote:
>>>>
>>>> If cell happens to be dead we can remove a fair amount of generic kernel
>>>> code that only exists to support cell.
>>>
>>> Even if some people might still use cell (which sounds unlikely), I
>>> think we can remove the spu core dumping code.
>>
>> The Cell blade hardware (arch/powerpc/platforms/cell/) that I'm listed
>> as a maintainer for is very much dead, but there is apparently still some
>> activity on the Playstation 3 that Geoff Levand maintains.
>>
>> Eric correctly points out that the PS3 firmware no longer boots
>> Linux (OtherOS), but AFAIK there are both users with old firmware
>> and those that use a firmware exploit to run homebrew code including
>> Linux.
>>
>> I would assume they still use the SPU and might also use the core
>> dump code in particular. Let's see what Geoff thinks.
>
> There are still PS3-Linux users out there. They use 'Homebrew' firmware
> released through 'Hacker' forums that allow them to run Linux on
> non-supported systems. They are generally hobbies who don't post to
> Linux kernel mailing lists. I get direct inquiries regularly asking
> about how to update to a recent kernel. One of the things that attract
> them to the PS3 is the Cell processor and either using or programming
> the SPUs.
>
> It is difficult to judge how much use the SPU core dump support gets,
> but if it is not a cause of major problems I feel we should consider
> keeping it.
I just took a quick look to get a sense how much tool support there is.
In the gdb tree I found this 2019 commit abf516c6931a ("Remove Cell
Broadband Engine debugging support"). Which basically removes the code
in gdb that made sense of the spu coredumps.
I would not say the coredump support is a source major problems, but it
is a challenge to understand. One of the pieces of code in there that
is necessary to make the coredump support work reliable, a call to
unshare_files, Oleg whole essentially maintains the ptrace and coredump
support did not know why it was there, and it was not at all obvious
when I looked at the code.
So we are certainly in maintainers loosing hours of time figuring out
what is going on and spending time fixing fuzzer bugs related to the
code.
At the minimum I will add a few more comments so people reading the code
can realize why it is there. Perhaps putting the relevant code behind
a Kconfig so it is only built into the kernel when spufs is present.
I think we are at a point we we can start planning on removing the
coredump support. The tools to read it are going away. None of what is
there is bad, but it is definitely a special case, and it definitely has
a maintenance cost.
Eric
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/[email protected]
[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 <[email protected]>
---
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(+)
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;
--
2.25.0
Eric
On Wed, Nov 25, 2020 at 2:16 AM Eric W. Biederman <[email protected]> wrote:
> > On 11/24/20 12:14 PM, Arnd Bergmann wrote:
> >
> > There are still PS3-Linux users out there. They use 'Homebrew' firmware
> > released through 'Hacker' forums that allow them to run Linux on
> > non-supported systems. They are generally hobbies who don't post to
> > Linux kernel mailing lists. I get direct inquiries regularly asking
> > about how to update to a recent kernel. One of the things that attract
> > them to the PS3 is the Cell processor and either using or programming
> > the SPUs.
> >
> > It is difficult to judge how much use the SPU core dump support gets,
> > but if it is not a cause of major problems I feel we should consider
> > keeping it.
>
> I just took a quick look to get a sense how much tool support there is.
>
> In the gdb tree I found this 2019 commit abf516c6931a ("Remove Cell
> Broadband Engine debugging support"). Which basically removes the code
> in gdb that made sense of the spu coredumps.
Ah, I had not realized this was gone already. The code in gdb for
seamlessly debugging programs across CPU and SPU was clearly
more complex than the kernel portion for the coredump, so it makes
sense this was removed eventually.
> I would not say the coredump support is a source major problems, but it
> is a challenge to understand. One of the pieces of code in there that
> is necessary to make the coredump support work reliable, a call to
> unshare_files, Oleg whole essentially maintains the ptrace and coredump
> support did not know why it was there, and it was not at all obvious
> when I looked at the code.
>
> So we are certainly in maintainers loosing hours of time figuring out
> what is going on and spending time fixing fuzzer bugs related to the
> code.
I also spent some amount of time on this code earlier this year Christoph
did some refactoring, and we could both have used that time better.
> At the minimum I will add a few more comments so people reading the code
> can realize why it is there. Perhaps putting the relevant code behind
> a Kconfig so it is only built into the kernel when spufs is present.
>
> I think we are at a point we we can start planning on removing the
> coredump support. The tools to read it are going away. None of what is
> there is bad, but it is definitely a special case, and it definitely has
> a maintenance cost.
How about adding a comment in the coredump code so it can get
removed the next time someone comes across it during refactoring,
or when they find a bug that can't easily be worked around?
That way there is still a chance of using it where needed, but
hopefully it won't waste anyone's time when it gets in the way.
If there are no objections, I can also send a patch to remove
CONFIG_PPC_CELL_NATIVE, PPC_IBM_CELL_BLADE and
everything that depends on those symbols, leaving only the
bits needed by ps3 in the arch/powerpc/platforms/cell directory.
Arnd
Arnd Bergmann <[email protected]> writes:
> On Wed, Nov 25, 2020 at 2:16 AM Eric W. Biederman <[email protected]> wrote:
>> > On 11/24/20 12:14 PM, Arnd Bergmann wrote:
>> >
>> > There are still PS3-Linux users out there. They use 'Homebrew' firmware
>> > released through 'Hacker' forums that allow them to run Linux on
>> > non-supported systems. They are generally hobbies who don't post to
>> > Linux kernel mailing lists. I get direct inquiries regularly asking
>> > about how to update to a recent kernel. One of the things that attract
>> > them to the PS3 is the Cell processor and either using or programming
>> > the SPUs.
>> >
>> > It is difficult to judge how much use the SPU core dump support gets,
>> > but if it is not a cause of major problems I feel we should consider
>> > keeping it.
>>
>> I just took a quick look to get a sense how much tool support there is.
>>
>> In the gdb tree I found this 2019 commit abf516c6931a ("Remove Cell
>> Broadband Engine debugging support"). Which basically removes the code
>> in gdb that made sense of the spu coredumps.
>
> Ah, I had not realized this was gone already. The code in gdb for
> seamlessly debugging programs across CPU and SPU was clearly
> more complex than the kernel portion for the coredump, so it makes
> sense this was removed eventually.
>
>> I would not say the coredump support is a source major problems, but it
>> is a challenge to understand. One of the pieces of code in there that
>> is necessary to make the coredump support work reliable, a call to
>> unshare_files, Oleg whole essentially maintains the ptrace and coredump
>> support did not know why it was there, and it was not at all obvious
>> when I looked at the code.
>>
>> So we are certainly in maintainers loosing hours of time figuring out
>> what is going on and spending time fixing fuzzer bugs related to the
>> code.
>
> I also spent some amount of time on this code earlier this year Christoph
> did some refactoring, and we could both have used that time better.
>
>> At the minimum I will add a few more comments so people reading the code
>> can realize why it is there. Perhaps putting the relevant code behind
>> a Kconfig so it is only built into the kernel when spufs is present.
>>
>> I think we are at a point we we can start planning on removing the
>> coredump support. The tools to read it are going away. None of what is
>> there is bad, but it is definitely a special case, and it definitely has
>> a maintenance cost.
>
> How about adding a comment in the coredump code so it can get
> removed the next time someone comes across it during refactoring,
> or when they find a bug that can't easily be worked around?
Did my proposed patch look ok?
> That way there is still a chance of using it where needed, but
> hopefully it won't waste anyone's time when it gets in the way.
Sounds good to me.
> If there are no objections, I can also send a patch to remove
> CONFIG_PPC_CELL_NATIVE, PPC_IBM_CELL_BLADE and
> everything that depends on those symbols, leaving only the
> bits needed by ps3 in the arch/powerpc/platforms/cell directory.
That also seems reasonable. My read of the history suggests that
code has been out of commission for a decade or so, and not having it to
trip over (just present in the history) seems very reasonable.
Eric
Arnd Bergmann <[email protected]> writes:
...
>
> If there are no objections, I can also send a patch to remove
> CONFIG_PPC_CELL_NATIVE, PPC_IBM_CELL_BLADE and
> everything that depends on those symbols, leaving only the
> bits needed by ps3 in the arch/powerpc/platforms/cell directory.
I'm not sure I'd merge it.
The only way I am able to (easily) test Cell code is by using one of
those blades, a QS22 to be precise.
So if the blade support is removed then the rest of the Cell code is
likely to bitrot quickly. Which may be the goal.
I'd be more inclined to support removal of the core dump code. That
seems highly unlikely to be in active use, I don't have the impression
there are many folks doing active development on Cell.
cheers
[email protected] (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/[email protected]
> [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 <[email protected]>
> ---
>
> 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 <[email protected]> wrote:
>
> [email protected] (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/[email protected]
> > [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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
On Mon, Nov 23, 2020 at 10:25:13AM -0800, Linus Torvalds wrote:
> On Mon, Nov 23, 2020 at 9:52 AM Oleg Nesterov <[email protected]> wrote:
> >
> > Can anyone explain why does do_coredump() need unshare_files() at all?
>
> Hmm. It goes back to 2012, and it's placed just before calling
> "->core_dump()", so I assume some core dumping function messed with
> the file table back when..
>
> I can't see anything like that currently.
>
> The alternative is that core-dumping just keeps the file table around
> for a long while, and thus files don't actually close in a timely
> manner. So it might not be a "correctness" issue as much as a latency
> issue.
IIRC, it was "weird architecture hooks might be playing silly buggers
with some per-descriptor information they want in coredumps, better
make sure it can't change under them"; it doesn't cost much and
it reduced the analysis surface nicely.
Had been a while ago, so the memories might be faulty... Anyway, that
reasoning seems to be applicable right now - rather than keeping an
eye on coredump logics on random architectures that might be looking
at descriptor table in unsafe way, just make sure they have a stable
private table and be done with that.
How much is simplified by not doing it there, anyway?