| Message ID | 20201120231441.29911-2-ebiederm@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/24] exec: Move unshare_files to fix posix file locking during exec | expand |
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 <oleg@redhat.com> 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 11:55 AM Eric W. Biederman <ebiederm@xmission.com> 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 Tue, Nov 24, 2020 at 8:58 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Tue, Nov 24, 2020 at 11:55 AM Eric W. Biederman > <ebiederm@xmission.com> 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 11/24/20 12:14 PM, Arnd Bergmann wrote: > On Tue, Nov 24, 2020 at 8:58 PM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> On Tue, Nov 24, 2020 at 11:55 AM Eric W. Biederman >> <ebiederm@xmission.com> 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 <geoff@infradead.org> writes: > On 11/24/20 12:14 PM, Arnd Bergmann wrote: >> On Tue, Nov 24, 2020 at 8:58 PM Linus Torvalds >> <torvalds@linux-foundation.org> wrote: >>> >>> On Tue, Nov 24, 2020 at 11:55 AM Eric W. Biederman >>> <ebiederm@xmission.com> 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
On Wed, Nov 25, 2020 at 2:16 AM Eric W. Biederman <ebiederm@xmission.com> 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 <arnd@kernel.org> writes: > On Wed, Nov 25, 2020 at 2:16 AM Eric W. Biederman <ebiederm@xmission.com> 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 <arnd@kernel.org> 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
On Mon, Nov 23, 2020 at 10:25:13AM -0800, Linus Torvalds wrote: > On Mon, Nov 23, 2020 at 9:52 AM Oleg Nesterov <oleg@redhat.com> 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?
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; }