| Message ID | 20230813123333.1705833-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kernel/fork: stop playing lockless games for exe_file replacement | expand |
On 13.08.23 14:33, Mateusz Guzik wrote: > xchg originated in 6e399cd144d8 ("prctl: avoid using mmap_sem for > exe_file serialization"). While the commit message does not explain > *why* the change, clearly the intent was to use mmap_sem less in this > codepath. I found the original submission [1] which ultimately claims it > cleans things up. More details are apparently in v1 of that patch: https://lore.kernel.org/lkml/1424979417.10344.14.camel@stgolabs.net/ Regarding your patch: adding more mmap_write_lock() where avoidable, I'm not so sure. Your patch doesn't look (to me) like it is removing a lot of complexity.
On 8/14/23, David Hildenbrand <david@redhat.com> wrote: > On 13.08.23 14:33, Mateusz Guzik wrote: >> xchg originated in 6e399cd144d8 ("prctl: avoid using mmap_sem for >> exe_file serialization"). While the commit message does not explain >> *why* the change, clearly the intent was to use mmap_sem less in this >> codepath. I found the original submission [1] which ultimately claims it >> cleans things up. > > More details are apparently in v1 of that patch: > > https://lore.kernel.org/lkml/1424979417.10344.14.camel@stgolabs.net/ > > Regarding your patch: adding more mmap_write_lock() where avoidable, I'm > not so sure. > But exe_file access is already synchronized with the semaphore and your own commit added a mmap_read_lock/mmap_read_unlock cycle after the xchg in question to accomodate this requirement. Then mmap_write_lock around the replacement is the obvious thing to do. > Your patch doesn't look (to me) like it is removing a lot of complexity. > The code in the current form makes the reader ask what prompts xchg + read-lock instead of mere write-locking. This is not a hot path either and afaics it can only cause contention if userspace is trying to abuse the interface to break the kernel, messing with a processs hard at work (which would be an extra argument to not play games on kernel side). That said, no, it does not remove "a lot of complexity". It does remove some though at no real downside that I can see. But then again, it is on people who insist on xchg to justify it.
On 14.08.23 10:21, Mateusz Guzik wrote: > On 8/14/23, David Hildenbrand <david@redhat.com> wrote: >> On 13.08.23 14:33, Mateusz Guzik wrote: >>> xchg originated in 6e399cd144d8 ("prctl: avoid using mmap_sem for >>> exe_file serialization"). While the commit message does not explain >>> *why* the change, clearly the intent was to use mmap_sem less in this >>> codepath. I found the original submission [1] which ultimately claims it >>> cleans things up. >> >> More details are apparently in v1 of that patch: >> >> https://lore.kernel.org/lkml/1424979417.10344.14.camel@stgolabs.net/ >> >> Regarding your patch: adding more mmap_write_lock() where avoidable, I'm >> not so sure. >> > > But exe_file access is already synchronized with the semaphore and > your own commit added a mmap_read_lock/mmap_read_unlock cycle after > the xchg in question to accomodate this requirement. Yes, we want to handle concurrent fork() ("Don't race with dup_mmap()"), thus mmap_read_lock. > Then mmap_write_lock around the replacement is the obvious thing to do. Apparently to you. :) mmap_write_lock will block more than fork. For example, concurrent page faults (without new VMA locking), for no apparent reason to me. > >> Your patch doesn't look (to me) like it is removing a lot of complexity. >> > > The code in the current form makes the reader ask what prompts xchg + > read-lock instead of mere write-locking. > > This is not a hot path either and afaics it can only cause contention > if userspace is trying to abuse the interface to break the kernel, > messing with a processs hard at work (which would be an extra argument > to not play games on kernel side). > > That said, no, it does not remove "a lot of complexity". It does > remove some though at no real downside that I can see. > > But then again, it is on people who insist on xchg to justify it. Changing it now needs good justification, why we would want to block any concurrent MM activity. Maybe there is good justification. In any case, this commit would have to update the documentation of replace_mm_exe_file, that spells out existing locking behavior.
On 8/14/23, David Hildenbrand <david@redhat.com> wrote: > On 14.08.23 10:21, Mateusz Guzik wrote: >> On 8/14/23, David Hildenbrand <david@redhat.com> wrote: >>> On 13.08.23 14:33, Mateusz Guzik wrote: >>>> xchg originated in 6e399cd144d8 ("prctl: avoid using mmap_sem for >>>> exe_file serialization"). While the commit message does not explain >>>> *why* the change, clearly the intent was to use mmap_sem less in this >>>> codepath. I found the original submission [1] which ultimately claims >>>> it >>>> cleans things up. >>> >>> More details are apparently in v1 of that patch: >>> >>> https://lore.kernel.org/lkml/1424979417.10344.14.camel@stgolabs.net/ >>> >>> Regarding your patch: adding more mmap_write_lock() where avoidable, I'm >>> not so sure. >>> >> >> But exe_file access is already synchronized with the semaphore and >> your own commit added a mmap_read_lock/mmap_read_unlock cycle after >> the xchg in question to accomodate this requirement. > > Yes, we want to handle concurrent fork() ("Don't race with dup_mmap()"), > thus mmap_read_lock. > >> Then mmap_write_lock around the replacement is the obvious thing to do. > > Apparently to you. :) > > mmap_write_lock will block more than fork. For example, concurrent page > faults (without new VMA locking), for no apparent reason to me. > >> >>> Your patch doesn't look (to me) like it is removing a lot of complexity. >>> >> >> The code in the current form makes the reader ask what prompts xchg + >> read-lock instead of mere write-locking. >> >> This is not a hot path either and afaics it can only cause contention >> if userspace is trying to abuse the interface to break the kernel, >> messing with a processs hard at work (which would be an extra argument >> to not play games on kernel side). >> >> That said, no, it does not remove "a lot of complexity". It does >> remove some though at no real downside that I can see. >> >> But then again, it is on people who insist on xchg to justify it. > > Changing it now needs good justification, why we would want to block any > concurrent MM activity. Maybe there is good justification. > > In any case, this commit would have to update the documentation of > replace_mm_exe_file, that spells out existing locking behavior. > Perhaps it will help if I add that the prctl thingy always had a troubled relationship with locking. Last time I looked at it was in 2016, where I found that it was doing down_read to update arg_start/arg_end and others while a consumer in procfs would read them and assert on their sanity. As only a read-lock was held, 2 threads could be used to transiently produce a bogus state as they race with their updates and trigger the BUG. See this commit (but also excuse weirdly bad english ;)) https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ddf1d398e517e660207e2c807f76a90df543a217 Moreover check out the following in prctl_set_auxv: task_lock(current); memcpy(mm->saved_auxv, user_auxv, len); task_unlock(current); any thread in the process can reach that codepath while sharing the same mm, thus this does not lock any updates. Not only that, but a duplicated memcpy onto the area in prctl_set_mm_map does not even take that lock and the code to read this does not take any locks. [Code duplication and synchronization aside, additional points deducted for saved_auxv storing always-NULL pointers instead of adding them on reads.] The above exhausts my willingness to argue about this change, I'm just a passerby. If it is NAKed, I'm dropping the subject. I am willing to do the comment tidy ups if this can go in though, but not before there is consensus.
On 08/13, Mateusz Guzik wrote: > > fe69d560b5bd ("kernel/fork: always deny write access to current MM > exe_file") added another lock trip to synchronize the state of exe_file > against fork, further defeating the point of xchg. > > As such I think the atomic here only adds complexity for no benefit. > > Just write-lock around the replacement. Well, I tend to agree but can't really comment because I forgot everything about these code paths. But I have to admit that I don't understand the code in replace_mm_exe_file() without this patch... old_exe_file = xchg(&mm->exe_file, new_exe_file); if (old_exe_file) { /* * Don't race with dup_mmap() getting the file and disallowing * write access while someone might open the file writable. */ mmap_read_lock(mm); allow_write_access(old_exe_file); fput(old_exe_file); mmap_read_unlock(mm); } Can someone please explain me which exactly race this mmap_read_lock() tries to avoid and how ? Oleg.
On 08/14, Oleg Nesterov wrote: > > On 08/13, Mateusz Guzik wrote: > > > > fe69d560b5bd ("kernel/fork: always deny write access to current MM > > exe_file") added another lock trip to synchronize the state of exe_file > > against fork, further defeating the point of xchg. > > > > As such I think the atomic here only adds complexity for no benefit. > > > > Just write-lock around the replacement. > > Well, I tend to agree but can't really comment because I forgot everything > about these code paths. > > But I have to admit that I don't understand the code in replace_mm_exe_file() > without this patch... > > old_exe_file = xchg(&mm->exe_file, new_exe_file); > if (old_exe_file) { > /* > * Don't race with dup_mmap() getting the file and disallowing > * write access while someone might open the file writable. > */ > mmap_read_lock(mm); > allow_write_access(old_exe_file); > fput(old_exe_file); > mmap_read_unlock(mm); > } > > Can someone please explain me which exactly race this mmap_read_lock() tries > to avoid and how ? OK, I seem to understand... without mmap_read_lock() it is possible that - dup_mm_exe_file() sees mm->exe_file = old_exe_file - replace_mm_exe_file() does allow_write_access(old_exe_file) - another process does get_write_access(old_exe_file) - dup_mm_exe_file()->deny_write_access() fails Right? Or something else? Well to me Mateusz's patch does make this logic more clear ;) Oleg.
On 14.08.23 17:20, Oleg Nesterov wrote: > On 08/14, Oleg Nesterov wrote: >> >> On 08/13, Mateusz Guzik wrote: >>> >>> fe69d560b5bd ("kernel/fork: always deny write access to current MM >>> exe_file") added another lock trip to synchronize the state of exe_file >>> against fork, further defeating the point of xchg. >>> >>> As such I think the atomic here only adds complexity for no benefit. >>> >>> Just write-lock around the replacement. >> >> Well, I tend to agree but can't really comment because I forgot everything >> about these code paths. >> >> But I have to admit that I don't understand the code in replace_mm_exe_file() >> without this patch... >> >> old_exe_file = xchg(&mm->exe_file, new_exe_file); >> if (old_exe_file) { >> /* >> * Don't race with dup_mmap() getting the file and disallowing >> * write access while someone might open the file writable. >> */ >> mmap_read_lock(mm); >> allow_write_access(old_exe_file); >> fput(old_exe_file); >> mmap_read_unlock(mm); >> } >> >> Can someone please explain me which exactly race this mmap_read_lock() tries >> to avoid and how ? > > OK, I seem to understand... without mmap_read_lock() it is possible that > > - dup_mm_exe_file() sees mm->exe_file = old_exe_file > > - replace_mm_exe_file() does allow_write_access(old_exe_file) > > - another process does get_write_access(old_exe_file) > > - dup_mm_exe_file()->deny_write_access() fails > > Right? From what I recall, yes.
On 14.08.23 10:54, Mateusz Guzik wrote: > On 8/14/23, David Hildenbrand <david@redhat.com> wrote: >> On 14.08.23 10:21, Mateusz Guzik wrote: >>> On 8/14/23, David Hildenbrand <david@redhat.com> wrote: >>>> On 13.08.23 14:33, Mateusz Guzik wrote: >>>>> xchg originated in 6e399cd144d8 ("prctl: avoid using mmap_sem for >>>>> exe_file serialization"). While the commit message does not explain >>>>> *why* the change, clearly the intent was to use mmap_sem less in this >>>>> codepath. I found the original submission [1] which ultimately claims >>>>> it >>>>> cleans things up. >>>> >>>> More details are apparently in v1 of that patch: >>>> >>>> https://lore.kernel.org/lkml/1424979417.10344.14.camel@stgolabs.net/ >>>> >>>> Regarding your patch: adding more mmap_write_lock() where avoidable, I'm >>>> not so sure. >>>> >>> >>> But exe_file access is already synchronized with the semaphore and >>> your own commit added a mmap_read_lock/mmap_read_unlock cycle after >>> the xchg in question to accomodate this requirement. >> >> Yes, we want to handle concurrent fork() ("Don't race with dup_mmap()"), >> thus mmap_read_lock. >> >>> Then mmap_write_lock around the replacement is the obvious thing to do. >> >> Apparently to you. :) >> >> mmap_write_lock will block more than fork. For example, concurrent page >> faults (without new VMA locking), for no apparent reason to me. >> >>> >>>> Your patch doesn't look (to me) like it is removing a lot of complexity. >>>> >>> >>> The code in the current form makes the reader ask what prompts xchg + >>> read-lock instead of mere write-locking. >>> >>> This is not a hot path either and afaics it can only cause contention >>> if userspace is trying to abuse the interface to break the kernel, >>> messing with a processs hard at work (which would be an extra argument >>> to not play games on kernel side). >>> >>> That said, no, it does not remove "a lot of complexity". It does >>> remove some though at no real downside that I can see. >>> >>> But then again, it is on people who insist on xchg to justify it. >> >> Changing it now needs good justification, why we would want to block any >> concurrent MM activity. Maybe there is good justification. >> >> In any case, this commit would have to update the documentation of >> replace_mm_exe_file, that spells out existing locking behavior. >> > > Perhaps it will help if I add that the prctl thingy always had a > troubled relationship with locking. Yes, it's not the first time that I looked at kernel/sys.c and wodnered why some of the toggles don't perform any locking. > > Last time I looked at it was in 2016, where I found that it was doing > down_read to update arg_start/arg_end and others while a consumer in > procfs would read them and assert on their sanity. As only a read-lock > was held, 2 threads could be used to transiently produce a bogus state > as they race with their updates and trigger the BUG. See this commit > (but also excuse weirdly bad english ;)) > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ddf1d398e517e660207e2c807f76a90df543a217 > > Moreover check out the following in prctl_set_auxv: > > task_lock(current); > memcpy(mm->saved_auxv, user_auxv, len); > task_unlock(current); > > any thread in the process can reach that codepath while sharing the > same mm, thus this does not lock any updates. Not only that, but a > duplicated memcpy onto the area in prctl_set_mm_map does not even take > that lock and the code to read this does not take any locks. > > [Code duplication and synchronization aside, additional points > deducted for saved_auxv storing always-NULL pointers instead of adding > them on reads.] > > The above exhausts my willingness to argue about this change, I'm just > a passerby. If it is NAKed, I'm dropping the subject. As long as nobody cares about concurrent MM activity being restricted with your change (I suspect we don't care), we're good.
On 08/14, David Hildenbrand wrote: > > >OK, I seem to understand... without mmap_read_lock() it is possible that > > > > - dup_mm_exe_file() sees mm->exe_file = old_exe_file > > > > - replace_mm_exe_file() does allow_write_access(old_exe_file) > > > > - another process does get_write_access(old_exe_file) > > > > - dup_mm_exe_file()->deny_write_access() fails > > > >Right? > > From what I recall, yes. Thanks! but then... David, this all is subjective, feel free to ignore, but the current code doesn't look good to me, I mean the purpose of mmap_read_lock() is very unclear. To me something like if (old_exe_file) { /* * Ensure that if we race with dup_mm_exe_file() and it sees * mm->exe_file == old_exe_file deny_write_access(old_exe_file) * can't fail after we do allow_write_access() and another task * does get_write_access(old_exe_file). */ mmap_read_lock(mm); mmap_read_unlock(mm); allow_write_access(old_exe_file); fput(old_exe_file); } looks more understandable... But this patch from Mateusz looks even better to me. So, FWIW, Acked-by: Oleg Nesterov <oleg@redhat.com>
On 14.08.23 17:58, Oleg Nesterov wrote: > On 08/14, David Hildenbrand wrote: >> >>> OK, I seem to understand... without mmap_read_lock() it is possible that >>> >>> - dup_mm_exe_file() sees mm->exe_file = old_exe_file >>> >>> - replace_mm_exe_file() does allow_write_access(old_exe_file) >>> >>> - another process does get_write_access(old_exe_file) >>> >>> - dup_mm_exe_file()->deny_write_access() fails >>> >>> Right? >> >> From what I recall, yes. > > Thanks! but then... David, this all is subjective, feel free to ignore, but > the current code doesn't look good to me, I mean the purpose of mmap_read_lock() > is very unclear. To me something like > > if (old_exe_file) { > /* > * Ensure that if we race with dup_mm_exe_file() and it sees > * mm->exe_file == old_exe_file deny_write_access(old_exe_file) > * can't fail after we do allow_write_access() and another task > * does get_write_access(old_exe_file). > */ > mmap_read_lock(mm); > mmap_read_unlock(mm); > > allow_write_access(old_exe_file); > fput(old_exe_file); > } > > looks more understandable... I don't particularly care about that code, and if there are ways to make it clearer, great. As long as we can clarify in the patch description why we decided to go again the other direction (write lock) and not do what we did in 2015, that would be great.
diff --git a/kernel/fork.c b/kernel/fork.c index d2e12b6d2b18..f576ce341e43 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1464,22 +1464,20 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file) return ret; } - /* set the new file, lockless */ ret = deny_write_access(new_exe_file); if (ret) return -EACCES; get_file(new_exe_file); - old_exe_file = xchg(&mm->exe_file, new_exe_file); + /* set the new file */ + mmap_write_lock(mm); + old_exe_file = rcu_dereference_raw(mm->exe_file); + rcu_assign_pointer(mm->exe_file, new_exe_file); + mmap_write_unlock(mm); + if (old_exe_file) { - /* - * Don't race with dup_mmap() getting the file and disallowing - * write access while someone might open the file writable. - */ - mmap_read_lock(mm); allow_write_access(old_exe_file); fput(old_exe_file); - mmap_read_unlock(mm); } return 0; }
xchg originated in 6e399cd144d8 ("prctl: avoid using mmap_sem for exe_file serialization"). While the commit message does not explain *why* the change, clearly the intent was to use mmap_sem less in this codepath. I found the original submission [1] which ultimately claims it cleans things up. However, replacement itself takes it for reading before doing any work and other places associated with it also take it. fe69d560b5bd ("kernel/fork: always deny write access to current MM exe_file") added another lock trip to synchronize the state of exe_file against fork, further defeating the point of xchg. 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] Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- kernel/fork.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)