Re: [PATCH] Remove struct mm_struct::exe_file et al

[Alexey Dobriyan <adobriyan@gmail.com>]

On Tue, Mar 31, 2009 at 05:14:27PM -0700, Matt Helsley wrote:
> On March 23rd Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > Commit 925d1c401fa6cfd0df5d2e37da8981494ccdec07 aka "procfs task exe symlink".
> > introduced struct mm_struct::exe_file and struct mm_struct::num_exe_file_vmas.
> > 
> > The rationale is weak: unifying MMU and no-MMU version of /proc/*/exe
> > code. For this a) struct mm_struct becomes bigger, b) mmap/munmap/exit become
> > slower, c) patch adds more code than removes in fact. ->exe_file maybe well
> 
> Your patch adds a VMA walk with the mmap semaphore held when openning
> /proc/*/exe. That gives unrelated tasks the opportunity to contend on each
> others mmap semaphores just by doing a readlink on /proc/*/exe.

This is dealt with in proc_fd_access_allowed() with ptrace check.

> Did you have any measurements to report showing an improvement in performance
> with your patch or are you expecting it to be "slower" based purely on code
> inspection? 

The performance is restored to previous levels, because code became the same
as before ->exe_file.

> > defined, but doesn't make sense always. After original executable is unmapped,
> > /proc/*/exe will still report it and, more importantly, pin corresponding
> > struct file. ->num_exe_file_vmas is even worse -- it just counts number of
> 
> ->exe_file does not pin the file. It uses the num_exe_file_vmas count to drop
> the mm->exe_file reference when the last executable VMA is gone. Since
> MAP_EXECUTABLE (and hence VM_EXECUTABLE) is only applied to the first executable
> file opened during exec (stored in ->exe_file) and since userspace can't use it
> (the mmap syscall code masks it out), num_exe_file_vmas counts VMAs related to 
> exe_file.

OK.

> So ->exe_file does not pin the file any more or less than the old VMA does...
> 
> > executable file-backed VMAs even if those VMAs aren't related to ->exe_file.
> 
> and this part of your argument is incorrect as well.
> 
> > After commit 8feae13110d60cc6287afabc2887366b0eb226c2 aka
> > "NOMMU: Make VMAs per MM as for MMU-mode linux" no-MMU kernels also
> > maintain list of VMAs in ->mmap, so we can switch back for MMU version
> > of /proc/*/exe.
> 
> Unless the executable is fully unmapped ->exe_file won't change even if
> something else manages to get mapped at a lower address. In contrast, the MMU
> version is sensitive to changes in the order of the VMAs over the lifetime of
> the task. It has worked in the past because MAP_EXECUTABLE only gets applied
> during exec, MAP_EXECUTABLE can only be applied by the kernel, all binary
> format handlers place the executable low in the mmap, and nothing else
> maps MAP_EXECUTABLE regions. While ->exe_file still relies on
> MAP_EXECUTABLE, it does not care about the mapped address because it
> does not walk the VMAs.

Both VMA walk for /proc/*/exe and ->exe_file are heuristics and rely on
the fact that userspace can't create explicit VM_EXECUTABLE mappings.
Take out that and ->exe_file becomes incorrect by design -- which is
the point of the patch to remove incorectness.