Re: [PATCH] kernel/fork: stop playing lockless games for exe_file replacement

[David Hildenbrand]

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