Messages in this thread | | | Date | Mon, 14 Aug 2023 17:38:55 +0200 | From | David Hildenbrand <> | Subject | Re: [PATCH] kernel/fork: stop playing lockless games for exe_file replacement |
| |
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.
-- Cheers,
David / dhildenb
| |