linux-kernel - Re: [RFC PATCH] mm, oom_reaper: skip mm structs with mmu notifiers

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]

Message-ID: <20170830174904.GF13559@redhat.com>
Date:   Wed, 30 Aug 2017 19:49:04 +0200
From:   Andrea Arcangeli <aarcange@...hat.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        LKML <linux-kernel@...r.kernel.org>,
        Michal Hocko <mhocko@...e.com>
Subject: Re: [RFC PATCH] mm, oom_reaper: skip mm structs with mmu notifiers

Hello Michal,

On Wed, Aug 30, 2017 at 10:46:00AM +0200, Michal Hocko wrote:
> +	 * TODO: we really want to get rid of this ugly hack and make sure that
> +	 * notifiers cannot block for unbounded amount of time and add
> +	 * mmu_notifier_invalidate_range_{start,end} around unmap_page_range

KVM already should be ok in that respect. However the major reason to
prefer mmu_notifier_invalidate_range_start/end is those can block and
schedule waiting for stuff happening behind the PCI bus easily. So I'm
not sure if the TODO is good idea to keep.

> +	 */
> +	if (mm_has_notifiers(mm)) {
> +		schedule_timeout_idle(HZ);

Why the schedule_timeout? What's the difference with the OOM
reaper going to sleep again in the main loop instead?

> +		goto unlock_oom;
> +	}

mm_has_notifiers stops changing after obtaining the mmap_sem for
reading. See the do_mmu_notifier_register. So it's better to put the
mm_has_notifiers check immediately after the below:

>  	if (!down_read_trylock(&mm->mmap_sem)) {
>  		ret = false;
>  		trace_skip_task_reaping(tsk->pid);

If we succeed taking the mmap_sem for reading then we read a stable
value out of mm_has_notifiers and be sure it won't be set from under
us.

Otherwise the patch looks fine including the incremental comment about
why the mmu_notifier_invalidate_range in MMU gather wasn't enough.

Thanks!
Andrea