| Message ID | 20200827114932.3572699-7-jannh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix ELF / FDPIC ELF core dumping, and use mmap_lock properly in there | expand |
On Thu, Aug 27, 2020 at 4:50 AM Jann Horn <jannh@google.com> wrote: > > Properly take the mmap_lock before calling into the GUP code from > get_dump_page(); and play nice, allowing the GUP code to drop the mmap_lock > if it has to sleep. Hmm. Of all the patches in the series, this simple one is now the only one I feel makes for ugly code. Certainly not uglier than it used to be, but also not as pretty as it could be.. I think you're pretty much just re-implementing get_user_pages_unlocked(), aren't you? There are differences - you use mmap_read_lock_killable(), for example. But I think get_user_pages_unlocked() should too. The other difference is that you don't set FOLL_TOUCH. So it's not *exactly* the same thing, but it's close enough that I get the feeling that this should be cleaned up to use a common helper between the two. That said, I suspect that falls under the heading of "future cleanup". I don't think there's any need to re-spin this series for this, it's just the only slightly negative reaction I had for the whole series now. Linus
diff --git a/mm/gup.c b/mm/gup.c index 92519e5a44b3..bd0f7311c5c6 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1552,19 +1552,23 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start, * NULL wherever the ZERO_PAGE, or an anonymous pte_none, has been found - * allowing a hole to be left in the corefile to save diskspace. * - * Called without mmap_lock, but after all other threads have been killed. + * Called without mmap_lock (takes and releases the mmap_lock by itself). */ #ifdef CONFIG_ELF_CORE struct page *get_dump_page(unsigned long addr) { - struct vm_area_struct *vma; + struct mm_struct *mm = current->mm; struct page *page; + int locked = 1; + int ret; - if (__get_user_pages_locked(current->mm, addr, 1, &page, &vma, NULL, - FOLL_FORCE | FOLL_DUMP | FOLL_GET) < 1) + if (mmap_read_lock_killable(mm)) return NULL; - flush_cache_page(vma, addr, page_to_pfn(page)); - return page; + ret = __get_user_pages_locked(mm, addr, 1, &page, NULL, &locked, + FOLL_FORCE | FOLL_DUMP | FOLL_GET); + if (locked) + mmap_read_unlock(mm); + return (ret == 1) ? page : NULL; } #endif /* CONFIG_ELF_CORE */
Properly take the mmap_lock before calling into the GUP code from get_dump_page(); and play nice, allowing the GUP code to drop the mmap_lock if it has to sleep. As Linus pointed out, we don't actually need the VMA because __get_user_pages() will flush the dcache for us if necessary. Signed-off-by: Jann Horn <jannh@google.com> --- mm/gup.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)