On 19.08.21 22:51, Linus Torvalds wrote:
So I like this series.
However, logically, I think this part in replace_mm_exe_file() no
longer makes sense:
On Mon, Aug 16, 2021 at 12:50 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
+ /* Forbid mm->exe_file change if old file still mapped. */
+ old_exe_file = get_mm_exe_file(mm);
+ if (old_exe_file) {
+ mmap_read_lock(mm);
+ for (vma = mm->mmap; vma && !ret; vma = vma->vm_next) {
+ if (!vma->vm_file)
+ continue;
+ if (path_equal(&vma->vm_file->f_path,
+ &old_exe_file->f_path))
+ ret = -EBUSY;
+ }
+ mmap_read_unlock(mm);
+ fput(old_exe_file);
+ if (ret)
+ return ret;
+ }
and should just be removed.
NOTE! I think it makes sense within the context of this patch (where
you just move code around), but that it should then be removed in the
next patch that does that "always deny write access to current MM
exe_file" thing.
I just quoted it in the context of this patch, since the next patch
doesn't actually show this code any more.
In the *old* model - where the ETXTBUSY was about the mmap() of the
file - the above tests make sense.
But in the new model, walking the mappings just doesn't seem to be a
sensible operation any more. The mappings simply aren't what ETXTBUSY
is about in the new world order, and so doing that mapping walk seems
nonsensical.
Hmm?
I think this is somewhat another kind of "stop user space trying
to do stupid things" thingy, not necessarily glued to ETXTBUSY:
don't allow replacing exe_file if that very file is still mapped
and consequently eventually still in use by the application.
I don't think it necessarily has many things to do with ETXTBUSY:
we only check if there is a VMA mapping that file, not that it's
a VM_DENYWRITE mapping.
That code originates from
commit 4229fb1dc6843c49a14bb098719f8a696cdc44f8
Author: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxx>
Date: Wed Jul 11 14:02:11 2012 -0700
c/r: prctl: less paranoid prctl_set_mm_exe_file()
"no other files mapped" requirement from my previous patch (c/r: prctl:
update prctl_set_mm_exe_file() after mm->num_exe_file_vmas removal) is too
paranoid, it forbids operation even if there mapped one shared-anon vma.
Let's check that current mm->exe_file already unmapped, in this case
exe_file symlink already outdated and its changing is reasonable.
The statement "exe_file symlink already outdated and its
changing is reasonable" somewhat makes sense.
Long story short, I think this check somehow makes a bit of sense, but
we wouldn't lose too much if we drop it -- just another sanity check.
Your call :)
--
Thanks,
David / dhildenb
