Messages in this thread | | | From | David Hildenbrand <> | Subject | Re: [PATCH v2 2/7] kernel/fork: factor out replacing the current MM exe_file | Date | Fri, 20 Aug 2021 10:46:45 +0200 |
| |
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@redhat.com> 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@openvz.org> 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
| |