linux-kernel - Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of unmap_page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 19 Dec 2016 15:00:09 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Rik van Riel <riel@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of
 unmap_page_range()

On Mon 19-12-16 20:39:24, Tetsuo Handa wrote:
> On 2016/12/16 23:15, Kirill A. Shutemov wrote:
> > Logic on whether we can reap pages from the VMA should match what we
> > have in madvise_dontneed(). In particular, we should skip, VM_PFNMAP
> > VMAs, but we don't now.
> > 
> > Let's just call madvise_dontneed() from __oom_reap_task_mm(), so we
> > won't need to sync the logic in the future.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> > ---
> >  mm/internal.h |  7 +++----
> >  mm/madvise.c  |  2 +-
> >  mm/memory.c   |  2 +-
> >  mm/oom_kill.c | 15 ++-------------
> >  4 files changed, 7 insertions(+), 19 deletions(-)
> 
> madvise_dontneed() calls zap_page_range().
> zap_page_range() calls mmu_notifier_invalidate_range_start().
> mmu_notifier_invalidate_range_start() calls __mmu_notifier_invalidate_range_start().
> __mmu_notifier_invalidate_range_start() calls srcu_read_lock()/srcu_read_unlock().
> This means that madvise_dontneed() might sleep.
> 
> I don't know what individual notifier will do, but for example
> 
>   static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
>           .invalidate_range_start = i915_gem_userptr_mn_invalidate_range_start,
>   };
> 
> i915_gem_userptr_mn_invalidate_range_start() calls flush_workqueue()
> which means that we can OOM livelock if work item involves memory allocation.
> Some of other notifiers call mutex_lock()/mutex_unlock().
> 
> Even if none of currently in-tree notifier users are blocked on memory
> allocation, I think it is not guaranteed that future changes/users won't be
> blocked on memory allocation.

Yes I agree. The reason I didn't go with zap_page_range was that I
didn't want to rely on any external code path. Moreover I believe that
we even do not have to care about mmu notifiers. The task is dead and
nobody should be watching its address space. If somebody still does then
it would get SEGV anyway. Or am I missing something?

-- 
Michal Hocko
SUSE Labs