| Message ID | 20201120231441.29911-2-ebiederm@xmission.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2,01/24] exec: Move unshare_files to fix posix file locking during exec | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 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; }