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