diff options
| author | Mateusz Guzik <mjguzik@gmail.com> | 2023-08-14 19:21:40 +0200 |
|---|---|---|
| committer | Andrew Morton <akpm@linux-foundation.org> | 2023-08-21 13:46:24 -0700 |
| commit | a7031f145257516ce725dfce38d99da598a3972d (patch) | |
| tree | 1035414f76f6eba63e7c4a7383fe70b80a018f53 /fs/exec.c | |
| parent | 8bd49ef211b61e228f9341e53c381e6c47d47212 (diff) | |
| download | lwn-a7031f145257516ce725dfce38d99da598a3972d.tar.gz lwn-a7031f145257516ce725dfce38d99da598a3972d.zip | |
kernel/fork: stop playing lockless games for exe_file replacement
xchg originated in 6e399cd144d8 ("prctl: avoid using mmap_sem for exe_file
serialization"). While the commit message does not explain *why* the
change, I found the original submission [1] which ultimately claims it
cleans things up by removing dependency of exe_file on the semaphore.
However, fe69d560b5bd ("kernel/fork: always deny write access to current
MM exe_file") added a semaphore up/down cycle to synchronize the state of
exe_file against fork, defeating the point of the original change.
This is on top of semaphore trips already present both in the replacing
function and prctl (the only consumer).
Normally replacing exe_file does not happen for busy processes, thus
write-locking is not an impediment to performance in the intended use
case. If someone keeps invoking the routine for a busy processes they are
trying to play dirty and that's another reason to avoid any trickery.
As such I think the atomic here only adds complexity for no benefit.
Just write-lock around the replacement.
I also note that replacement races against the mapping check loop as
nothing synchronizes actual assignment with with said checks but I am not
addressing it in this patch. (Is the loop of any use to begin with?)
Link: https://lore.kernel.org/linux-mm/1424979417.10344.14.camel@stgolabs.net/ [1]
Link: https://lkml.kernel.org/r/20230814172140.1777161-1-mjguzik@gmail.com
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: David Hildenbrand <david@redhat.com>
Cc: "Christian Brauner (Microsoft)" <brauner@kernel.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Konstantin Khlebnikov <koct9i@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mateusz Guzik <mjguzik@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Diffstat (limited to 'fs/exec.c')
| -rw-r--r-- | fs/exec.c | 4 |
1 files changed, 2 insertions, 2 deletions
diff --git a/fs/exec.c b/fs/exec.c index 1a827d55ba94..dc41180d4e70 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1276,8 +1276,8 @@ int begin_new_exec(struct linux_binprm * bprm) /* * Must be called _before_ exec_mmap() as bprm->mm is - * not visible until then. This also enables the update - * to be lockless. + * not visible until then. Doing it here also ensures + * we don't race against replace_mm_exe_file(). */ retval = set_mm_exe_file(bprm->mm, bprm->file); if (retval) |