linux-kernel - Re: [patch v2 1/2] mm, mmu_notifier: annotate mmu notifiers with blockable invalidate callbacks

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:   Fri, 15 Dec 2017 17:25:34 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     David Rientjes <rientjes@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Oded Gabbay <oded.gabbay@...il.com>,
        Alex Deucher <alexander.deucher@....com>,
        Christian König <christian.koenig@....com>,
        David Airlie <airlied@...ux.ie>,
        Joerg Roedel <joro@...tes.org>,
        Doug Ledford <dledford@...hat.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Mike Marciniszyn <mike.marciniszyn@...el.com>,
        Sean Hefty <sean.hefty@...el.com>,
        Dimitri Sivanich <sivanich@....com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [patch v2 1/2] mm, mmu_notifier: annotate mmu notifiers with
 blockable invalidate callbacks

On Thu 14-12-17 13:30:56, David Rientjes wrote:
> Commit 4d4bbd8526a8 ("mm, oom_reaper: skip mm structs with mmu notifiers")
> prevented the oom reaper from unmapping private anonymous memory with the
> oom reaper when the oom victim mm had mmu notifiers registered.
> 
> The rationale is that doing mmu_notifier_invalidate_range_{start,end}()
> around the unmap_page_range(), which is needed, can block and the oom
> killer will stall forever waiting for the victim to exit, which may not
> be possible without reaping.
> 
> That concern is real, but only true for mmu notifiers that have blockable
> invalidate_range_{start,end}() callbacks.  This patch adds a "flags" field
> to mmu notifier ops that can set a bit to indicate that these callbacks do
> not block.
> 
> The implementation is steered toward an expensive slowpath, such as after
> the oom reaper has grabbed mm->mmap_sem of a still alive oom victim.
> 
> Signed-off-by: David Rientjes <rientjes@...gle.com>

Yes, this make sense. I haven't checked all the existing mmu notifiers
but those that you have marked seem to be OK.

I just think that the semantic of the flag should be describe more. See
below

Acked-by: Michal Hocko <mhocko@...e.com>

> ---
>  v2:
>    - specifically exclude mmu_notifiers without invalidate callbacks
>    - move flags to mmu_notifier_ops per Paolo
>    - reverse flag from blockable -> not blockable per Christian
> 
>  drivers/infiniband/hw/hfi1/mmu_rb.c |  1 +
>  drivers/iommu/amd_iommu_v2.c        |  1 +
>  drivers/iommu/intel-svm.c           |  1 +
>  drivers/misc/sgi-gru/grutlbpurge.c  |  1 +
>  include/linux/mmu_notifier.h        | 21 +++++++++++++++++++++
>  mm/mmu_notifier.c                   | 31 +++++++++++++++++++++++++++++++
>  virt/kvm/kvm_main.c                 |  1 +
>  7 files changed, 57 insertions(+)
> 
[...]
> diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
> --- a/include/linux/mmu_notifier.h
> +++ b/include/linux/mmu_notifier.h
> @@ -10,6 +10,9 @@
>  struct mmu_notifier;
>  struct mmu_notifier_ops;
>  
> +/* mmu_notifier_ops flags */
> +#define MMU_INVALIDATE_DOES_NOT_BLOCK	(0x01)
> +
>  #ifdef CONFIG_MMU_NOTIFIER
>  
>  /*
> @@ -26,6 +29,15 @@ struct mmu_notifier_mm {
>  };
>  
>  struct mmu_notifier_ops {
> +	/*
> +	 * Flags to specify behavior of callbacks for this MMU notifier.
> +	 * Used to determine which context an operation may be called.
> +	 *
> +	 * MMU_INVALIDATE_DOES_NOT_BLOCK: invalidate_{start,end} does not
> +	 *				  block
> +	 */
> +	int flags;

This should be more specific IMHO. What do you think about the following
wording?

invalidate_{start,end,range} doesn't block on any locks which depend
directly or indirectly (via lock chain or resources e.g. worker context)
on a memory allocation.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -476,6 +476,7 @@ static void kvm_mmu_notifier_release(struct mmu_notifier *mn,
>  }
>  
>  static const struct mmu_notifier_ops kvm_mmu_notifier_ops = {
> +	.flags			= MMU_INVALIDATE_DOES_NOT_BLOCK,
>  	.invalidate_range_start	= kvm_mmu_notifier_invalidate_range_start,
>  	.invalidate_range_end	= kvm_mmu_notifier_invalidate_range_end,
>  	.clear_flush_young	= kvm_mmu_notifier_clear_flush_young,

-- 
Michal Hocko
SUSE Labs