linux-kernel - Re: [PATCH v2] mm/mmu_notifiers: Esnure range_end() is paired with range

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] [day] [month] [year] [list]

Message-ID: <20210311232847.GA2710221@ziepe.ca>
Date:   Thu, 11 Mar 2021 19:28:47 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        David Rientjes <rientjes@...gle.com>,
        Ben Gardon <bgardon@...gle.com>,
        Michal Hocko <mhocko@...e.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Dimitri Sivanich <dimitri.sivanich@....com>
Subject: Re: [PATCH v2] mm/mmu_notifiers: Esnure range_end() is paired with
 range_start()

On Thu, Mar 11, 2021 at 10:00:57AM -0800, Sean Christopherson wrote:
> If one or more notifiers fails .invalidate_range_start(), invoke
> .invalidate_range_end() for "all" notifiers.  If there are multiple
> notifiers, those that did not fail are expecting _start() and _end() to
> be paired, e.g. KVM's mmu_notifier_count would become imbalanced.
> Disallow notifiers that can fail _start() from implementing _end() so
> that it's unnecessary to either track which notifiers rejected _start(),
> or had already succeeded prior to a failed _start().
> 
> Note, the existing behavior of calling _start() on all notifiers even
> after a previous notifier failed _start() was an unintented "feature".
> Make it canon now that the behavior is depended on for correctness.
> 
> As of today, the bug is likely benign:
> 
>   1. The only caller of the non-blocking notifier is OOM kill.
>   2. The only notifiers that can fail _start() are the i915 and Nouveau
>      drivers.
>   3. The only notifiers that utilize _end() are the SGI UV GRU driver
>      and KVM.
>   4. The GRU driver will never coincide with the i195/Nouveau drivers.
>   5. An imbalanced kvm->mmu_notifier_count only causes soft lockup in the
>      _guest_, and the guest is already doomed due to being an OOM victim.
> 
> Fix the bug now to play nice with future usage, e.g. KVM has a potential
> use case for blocking memslot updates in KVM while an invalidation is
> in-progress, and failure to unblock would result in said updates being
> blocked indefinitely and hanging.
> 
> Found by inspection.  Verified by adding a second notifier in KVM that
> periodically returns -EAGAIN on non-blockable ranges, triggering OOM,
> and observing that KVM exits with an elevated notifier count.
> 
> Fixes: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
> Suggested-by: Jason Gunthorpe <jgg@...pe.ca>
> Cc: stable@...r.kernel.org
> Cc: David Rientjes <rientjes@...gle.com>
> Cc: Ben Gardon <bgardon@...gle.com>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: "Jérôme Glisse" <jglisse@...hat.com>
> Cc: Andrea Arcangeli <aarcange@...hat.com>
> Cc: Johannes Weiner <hannes@...xchg.org>
> Cc: Dimitri Sivanich <dimitri.sivanich@....com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> 
> v2: Reimplemented as suggested by Jason.  Only functional change relative
>     to Jason's suggestion is to check invalidate_range_end before calling to
>     avoid a NULL pointer dereference.  I also added more comments, hopefully
>     they're helpful...
> 
> v1: https://lkml.kernel.org/r/20210310213117.1444147-1-seanjc@google.com

Looks fine, thanks. I think you need some commit message remark to
discourage backporting, the added WARN_ON will trigger on older
kernels that have many more things implementing
invalidate_range_end(). It should not be backported to anything that
has more invalidate_range_ends()'s than today's kernel.

Reviewed-by: Jason Gunthorpe <jgg@...dia.com>

Jason