linux-kernel - Re: [PATCH 2/2] arm64: Notify on pte permission upgrades

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: <ZHZuSDp6ioPqI272@google.com>
Date:   Tue, 30 May 2023 14:44:40 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Robin Murphy <robin.murphy@....com>,
        Alistair Popple <apopple@...dia.com>,
        Andrew Morton <akpm@...ux-foundation.org>, will@...nel.org,
        catalin.marinas@....com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, nicolinc@...dia.com,
        linux-arm-kernel@...ts.infradead.org, kvm@...r.kernel.org,
        John Hubbard <jhubbard@...dia.com>, zhi.wang.linux@...il.com
Subject: Re: [PATCH 2/2] arm64: Notify on pte permission upgrades

On Tue, May 30, 2023, Jason Gunthorpe wrote:
> IMHO I think we messed this up at some point..
> 
> Joerg added invalidate_range just for the iommu to use, so having it
> be arch specific could make some sense.
> 
> However, KVM later co-opted it to do this:
> 
> commit e649b3f0188f8fd34dd0dde8d43fd3312b902fb2
> Author: Eiichi Tsukata <eiichi.tsukata@...anix.com>
> Date:   Sat Jun 6 13:26:27 2020 +0900
> 
>     KVM: x86: Fix APIC page invalidation race

...

>     The fix therefore is to reload the APIC-access page field in the VMCS
>     from kvm_mmu_notifier_invalidate_range() instead of ..._range_start().
> 
> Which I think is a hacky fix.

Agreed, though as you note below, the invalidate_range() description does make it
seem like a somewhat reasonable thing to do. 

> KVM already has locking for invalidate_start/end - it has to check
> mmu_notifier_retry_cache() with the sequence numbers/etc around when
> it does does hva_to_pfn()
> 
> The bug is that the kvm_vcpu_reload_apic_access_page() path is
> ignoring this locking so it ignores in-progress range
> invalidations. It should spin until the invalidation clears like other
> places in KVM.
> 
> The comment is kind of misleading because drivers shouldn't be abusing
> the iommu centric invalidate_range() thing to fix missing locking in
> start/end users. :\
> 
> So if KVM could be fixed up we could make invalidate_range defined to
> be an arch specific callback to synchronize the iommu TLB.

And maybe rename invalidate_range() and/or invalidate_range_{start,end}() to make
it super obvious that they are intended for two different purposes?  E.g. instead
of invalidate_range(), something like invalidate_secondary_tlbs().

FWIW, PPC's OpenCAPI support (drivers/misc/ocxl/link.c) also uses invalidate_range().
Though IIUC, the use case is the same as a "traditional" IOMMU, where a device can
share the CPU's page tables, so maybe the devices can be considered IOMMUs in practice,
if not in name?

  It allows an accelerator (which could be an FPGA, ASICs, ...) to access
  the host memory coherently, using virtual addresses. An OpenCAPI
  device can also host its own memory, that can be accessed from the
  host.

> Sean?

It's doable, though definitely not 6.4 material.  

I have patches coded up.  Assuming testing goes well, I'll post them regardless
of the OCXL side of things.  I've disliked KVM's one-off use of invalidate_range()
for a long time, this is a good excuse to get rid of it before KVM gains more usage.