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

[David Hildenbrand]

On 14.08.23 10:21, Mateusz Guzik wrote:
> On 8/14/23, David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxxxxx/
> >
> > 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.

--
Cheers,

David / dhildenb