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.
--
Cheers,
David / dhildenb
