some likely bugs in IOMMUv2 (in tlb_finish_mmu() nested flush and mremap())

some likely bugs in IOMMUv2 (in tlb_finish_mmu() nested flush and mremap())

[Jann Horn]

Hi!

I looked through some of the code related to IOMMUv2 (the thing where
the IOMMU walks the normal userspace page tables and TLB shootdowns
are replicated to the IOMMU through
mmu_notifier_ops::invalidate_range).

I think there's a bug in the interaction between tlb_finish_mmu() and
mmu_notifier_ops::invalidate_range: In the mm_tlb_flush_nested() case,
__tlb_reset_range() sets tlb->start and tlb->end *both* to ~0.
Afterwards, tlb_finish_mmu() calls
tlb_flush_mmu()->tlb_flush_mmu_tlbonly()->mmu_notifier_invalidate_range(),
which will pass those tlb->start and tlb->end values to
mmu_notifier_ops::invalidate_range callbacks. But those callbacks
don't know about this special case and then basically only flush
virtual address ~0, making the flush useless. (However, pretty much
every place that calls tlb_finish_mmu() first calls
mmu_notifier_invalidate_range_end() even though the appropriate thing
would probably be mmu_notifier_invalidate_range_only_end(); and I
think those two things probably cancel each other out?)

Also, from what I can tell, the mremap() code, in move_page_tables(),
only invokes mmu_notifier_ops::invalidate_range via the
mmu_notifier_invalidate_range_end() at the very end, long after TLB
flushes must have happened - sort of like the bug we had years ago
where mremap() was flushing the normal TLBs too late
(https://bugs.chromium.org/p/project-zero/issues/detail?id=1695).

Re: some likely bugs in IOMMUv2 (in tlb_finish_mmu() nested flush and mremap())

[Jason Gunthorpe]

On Fri, Sep 23, 2022 at 05:38:12PM +0200, Jann Horn wrote:
> Hi!
> 
> I looked through some of the code related to IOMMUv2 (the thing where
> the IOMMU walks the normal userspace page tables and TLB shootdowns
> are replicated to the IOMMU through
> mmu_notifier_ops::invalidate_range).
> 
> I think there's a bug in the interaction between tlb_finish_mmu() and
> mmu_notifier_ops::invalidate_range: In the mm_tlb_flush_nested() case,
> __tlb_reset_range() sets tlb->start and tlb->end *both* to ~0.
> Afterwards, tlb_finish_mmu() calls
> tlb_flush_mmu()->tlb_flush_mmu_tlbonly()->mmu_notifier_invalidate_range(),
> which will pass those tlb->start and tlb->end values to
> mmu_notifier_ops::invalidate_range callbacks. But those callbacks
> don't know about this special case and then basically only flush
> virtual address ~0, making the flush useless. 

Yeah, that looks wrong to me, and it extends more than just the iommu
drivers kvm_arch_mmu_notifier_invalidate_range() also does not handle
this coding.

Most likely tlb_flush_mmu_tlbonly() need to change it back to 0 to ~0?
I wonder why it uses such an odd coding in the first place?

Actually, maybe having mm_tlb_flush_nested() call __tlb_reset_range()
to generate a 'flush all' request is just a bad idea, as we already
had another bug in 7a30df49f63ad92 related to reset_range doing the
wrong thing for a flush all action.

> (However, pretty much every place that calls tlb_finish_mmu() first
> calls mmu_notifier_invalidate_range_end() even though the
> appropriate thing would probably be
> mmu_notifier_invalidate_range_only_end(); and I think those two
> things probably cancel each other out?)

That does sound like double flushing to me, though as you note below,
the invalidate_range() triggered by range_end() after the TLB
flush/page freeing is functionally incorrect, so we cannot rely on it.

> Also, from what I can tell, the mremap() code, in move_page_tables(),
> only invokes mmu_notifier_ops::invalidate_range via the
> mmu_notifier_invalidate_range_end() at the very end, long after TLB
> flushes must have happened - sort of like the bug we had years ago
> where mremap() was flushing the normal TLBs too late
> (https://bugs.chromium.org/p/project-zero/issues/detail?id=1695).

Based on the description of eb66ae03082960 I would say that yes the
invalidate_range op is missing here for the same reasons the CPU flush
was missing.

AFAIK if we are flushing the CPU tlb then we really must also flush
the CPU tlb that KVM controls, and that is primarily what
invalidate_range() is used for.

Which makes me wonder if the invalidate_range() hidden inside
invalidate_end() is a bad idea in general - when is this need and
would be correct? Isn't it better to put the invalidates near the TLB
invalidates and leave start/end as purely a bracketing API, which by
definition, cannot have an end that is 'too late'?

Jason

Re: some likely bugs in IOMMUv2 (in tlb_finish_mmu() nested flush and mremap())

[Sean Christopherson]

On Fri, Sep 23, 2022, Jason Gunthorpe wrote:
> On Fri, Sep 23, 2022 at 05:38:12PM +0200, Jann Horn wrote:
> > Hi!
> > 
> > I looked through some of the code related to IOMMUv2 (the thing where
> > the IOMMU walks the normal userspace page tables and TLB shootdowns
> > are replicated to the IOMMU through
> > mmu_notifier_ops::invalidate_range).
> > 
> > I think there's a bug in the interaction between tlb_finish_mmu() and
> > mmu_notifier_ops::invalidate_range: In the mm_tlb_flush_nested() case,
> > __tlb_reset_range() sets tlb->start and tlb->end *both* to ~0.
> > Afterwards, tlb_finish_mmu() calls
> > tlb_flush_mmu()->tlb_flush_mmu_tlbonly()->mmu_notifier_invalidate_range(),
> > which will pass those tlb->start and tlb->end values to
> > mmu_notifier_ops::invalidate_range callbacks. But those callbacks
> > don't know about this special case and then basically only flush
> > virtual address ~0, making the flush useless. 
> 
> Yeah, that looks wrong to me, and it extends more than just the iommu
> drivers kvm_arch_mmu_notifier_invalidate_range() also does not handle
> this coding.

FWIW, the bug is likely benign for KVM.  KVM does almost all of its TLB flushing
via invalidate_range_{start,end}(), the invalidate_range() hook is used only by
x86/VMX to react to a specific KVM-allocated page being migrated (the page is only
ever unmapped when the VM is dying).

> Most likely tlb_flush_mmu_tlbonly() need to change it back to 0 to ~0?
> I wonder why it uses such an odd coding in the first place?
> 
> Actually, maybe having mm_tlb_flush_nested() call __tlb_reset_range()
> to generate a 'flush all' request is just a bad idea, as we already
> had another bug in 7a30df49f63ad92 related to reset_range doing the
> wrong thing for a flush all action.
> 
> > (However, pretty much every place that calls tlb_finish_mmu() first
> > calls mmu_notifier_invalidate_range_end() even though the
> > appropriate thing would probably be
> > mmu_notifier_invalidate_range_only_end(); and I think those two
> > things probably cancel each other out?)
> 
> That does sound like double flushing to me, though as you note below,
> the invalidate_range() triggered by range_end() after the TLB
> flush/page freeing is functionally incorrect, so we cannot rely on it.
> 
> > Also, from what I can tell, the mremap() code, in move_page_tables(),
> > only invokes mmu_notifier_ops::invalidate_range via the
> > mmu_notifier_invalidate_range_end() at the very end, long after TLB
> > flushes must have happened - sort of like the bug we had years ago
> > where mremap() was flushing the normal TLBs too late
> > (https://bugs.chromium.org/p/project-zero/issues/detail?id=1695).
> 
> Based on the description of eb66ae03082960 I would say that yes the
> invalidate_range op is missing here for the same reasons the CPU flush
> was missing.
> 
> AFAIK if we are flushing the CPU tlb then we really must also flush
> the CPU tlb that KVM controls, and that is primarily what
> invalidate_range() is used for.

As above, for its actual secondary MMU, KVM invalidates and flushes at
invalidate_range_start(), and then prevents vCPUs from creating new entries for
the range until invalidate_range_start_end().

The VMX use case is for a physical address that is consumed by hardware without
going through the secondary page tables; using the start/end hooks would be slightly
annoying due to the need to stall the vCPU until end, and so KVM uses invalidate_range()
for that one specific case.

> Which makes me wonder if the invalidate_range() hidden inside
> invalidate_end() is a bad idea in general - when is this need and
> would be correct? Isn't it better to put the invalidates near the TLB
> invalidates and leave start/end as purely a bracketing API, which by
> definition, cannot have an end that is 'too late'?

Documentation/mm/mmu_notifier.rst explains this, although even that is quite subtle.
The argument is that if the change is purely to downgrade protections, then
deferring invalidate_range() is ok because the only requirement is that secondary
MMUs invalidate before the "end" of the sequence.

  When changing a pte to write protect or to point to a new write protected page  
  with same content (KSM) it is fine to delay the mmu_notifier_invalidate_range   
  call to mmu_notifier_invalidate_range_end() outside the page table lock. This   
  is true even if the thread doing the page table update is preempted right after 
  releasing page table lock but before call mmu_notifier_invalidate_range_end().

That said, I also dislike hiding invalidate_range() inside end(), I constantly
forget about that behavior.  To address that, what about renaming
mmu_notifier_invalidate_range_end() to make it more explicit, e.g.
mmu_notifier_invalidate_range_and_end().

Re: some likely bugs in IOMMUv2 (in tlb_finish_mmu() nested flush and mremap())

[Jason Gunthorpe]

On Mon, Sep 26, 2022 at 08:13:00PM +0000, Sean Christopherson wrote:

> > AFAIK if we are flushing the CPU tlb then we really must also flush
> > the CPU tlb that KVM controls, and that is primarily what
> > invalidate_range() is used for.
> 
> As above, for its actual secondary MMU, KVM invalidates and flushes at
> invalidate_range_start(), and then prevents vCPUs from creating new entries for
> the range until invalidate_range_start_end().

Was it always like this? Why did we add this invalidate_range thing if
nothing really needed it?

That means iommu is really the only place using it as a proper
synchronous shadow TLB flush.
 
> > Which makes me wonder if the invalidate_range() hidden inside
> > invalidate_end() is a bad idea in general - when is this need and
> > would be correct? Isn't it better to put the invalidates near the TLB
> > invalidates and leave start/end as purely a bracketing API, which by
> > definition, cannot have an end that is 'too late'?
> 
> Documentation/mm/mmu_notifier.rst explains this, although even that is quite subtle.
> The argument is that if the change is purely to downgrade protections, then
> deferring invalidate_range() is ok because the only requirement is that secondary
> MMUs invalidate before the "end" of the sequence.
> 
>   When changing a pte to write protect or to point to a new write protected page  
>   with same content (KSM) it is fine to delay the mmu_notifier_invalidate_range   
>   call to mmu_notifier_invalidate_range_end() outside the page table lock. This   

And then if KVM never needed it why on earth did we micro-optimize it
in such an obscure and opaque way?

>   is true even if the thread doing the page table update is preempted right after 
>   releasing page table lock but before call mmu_notifier_invalidate_range_end().

That feels like it is getting dangerously close to the CVE Jan pointed
at.. We have a write protected page, installed in the PTEs, PTLs
unlocked and other things can sense the PTE and see that it is write
protected - is it really true nothing acts on that - especially now
that DavidH has gone and changed all that logic?

IMHO if we had logic that required the CPU TLB to be flushed under a
certain lock I find it to be a very, very, difficult conceptual leap
that a shadow TLB is OK to flush later.  If the shadow TLB is OK then
lets move the CPU TLB out of the lock as well :)

> That said, I also dislike hiding invalidate_range() inside end(), I constantly
> forget about that behavior.  To address that, what about renaming
> mmu_notifier_invalidate_range_end() to make it more explicit, e.g.
> mmu_notifier_invalidate_range_and_end().

The name for the special case should really capture that hidden point
above 'invalidate_range_delayed_write_protect_end' or something else
long and horrible. Because it really is special, it is really is only
allowed in that one special case (assuming the logic still holds) and
every other possible case should catch the invalidate through the tlb
flusher.

Jason

Re: some likely bugs in IOMMUv2 (in tlb_finish_mmu() nested flush and mremap())

[Sean Christopherson]

On Mon, Sep 26, 2022, Jason Gunthorpe wrote:
> On Mon, Sep 26, 2022 at 08:13:00PM +0000, Sean Christopherson wrote:
> 
> > > AFAIK if we are flushing the CPU tlb then we really must also flush
> > > the CPU tlb that KVM controls, and that is primarily what
> > > invalidate_range() is used for.
> > 
> > As above, for its actual secondary MMU, KVM invalidates and flushes at
> > invalidate_range_start(), and then prevents vCPUs from creating new entries for
> > the range until invalidate_range_start_end().
> 
> Was it always like this? Why did we add this invalidate_range thing if
> nothing really needed it?

No, the invalidate_range() hook was added by commit 1897bdc4d331 ("mmu_notifier:
add mmu_notifier_invalidate_range()") for IOMMUs.  From that changelog, the key
issue is stalling hardware while the start+end pair is ongoing runs afoul of GPUs
that don't play nice with re-faulting "indefinitely.

    The page-fault handler in the AMD IOMMUv2 driver doesn't handle the fault
    if an invalidate_range_start/end pair is active, it just reports back
    SUCCESS to the device and let it refault the page.  But existing hardware
    (newer Radeon GPUs) that makes use of this feature don't re-fault
    indefinitly, after a certain number of faults for the same address the
    device enters a failure state and needs to be resetted.
    
    To avoid the GPUs entering a failure state we need to get rid of the
    empty-page-table workaround and use the mmu_notifier_invalidate_range()
    function introduced with this patch.

> That means iommu is really the only place using it as a proper
> synchronous shadow TLB flush.

More or less. There's also an "OpenCAPI coherent accelerator support" driver,
drivers/misc/ocxl, that appears use invalidate_range() the same way the IOMMU does.
No idea how relevant that is these days.

I much prefer KVM's (and the old IOMMU's) approach of re-faulting in hardware until
the entire sequence completes.   It _might_ be less performant, but I find it so
much easier to reason about.  I actually had typed out a "can we just kill off
mmu_notifier_invalidate_range() and force users to refault hardware" question
before seeing the above changelog.

> > > Which makes me wonder if the invalidate_range() hidden inside
> > > invalidate_end() is a bad idea in general - when is this need and
> > > would be correct? Isn't it better to put the invalidates near the TLB
> > > invalidates and leave start/end as purely a bracketing API, which by
> > > definition, cannot have an end that is 'too late'?
> > 
> > Documentation/mm/mmu_notifier.rst explains this, although even that is quite subtle.
> > The argument is that if the change is purely to downgrade protections, then
> > deferring invalidate_range() is ok because the only requirement is that secondary
> > MMUs invalidate before the "end" of the sequence.
> > 
> >   When changing a pte to write protect or to point to a new write protected page  
> >   with same content (KSM) it is fine to delay the mmu_notifier_invalidate_range   
> >   call to mmu_notifier_invalidate_range_end() outside the page table lock. This   
> 
> And then if KVM never needed it why on earth did we micro-optimize it
> in such an obscure and opaque way?

I don't know.  I found the series that introduced the behavior[*], but there are
no numbers provided and I haven't been able to dredge up why this was even looked
into in the first place.  From the cover letter:

  It should improve performances but i am lacking hardware and benchmarks
  which might show an improvement. Maybe folks in cc can help here.

[*] https://lore.kernel.org/all/20171017031003.7481-1-jglisse@redhat.com

> >   is true even if the thread doing the page table update is preempted right after 
> >   releasing page table lock but before call mmu_notifier_invalidate_range_end().
> 
> That feels like it is getting dangerously close to the CVE Jan pointed
> at.. We have a write protected page, installed in the PTEs, PTLs
> unlocked and other things can sense the PTE and see that it is write
> protected - is it really true nothing acts on that - especially now
> that DavidH has gone and changed all that logic?
> 
> IMHO if we had logic that required the CPU TLB to be flushed under a
> certain lock I find it to be a very, very, difficult conceptual leap
> that a shadow TLB is OK to flush later.  If the shadow TLB is OK then
> lets move the CPU TLB out of the lock as well :)
> 
> > That said, I also dislike hiding invalidate_range() inside end(), I constantly
> > forget about that behavior.  To address that, what about renaming
> > mmu_notifier_invalidate_range_end() to make it more explicit, e.g.
> > mmu_notifier_invalidate_range_and_end().
> 
> The name for the special case should really capture that hidden point
> above 'invalidate_range_delayed_write_protect_end' or something else
> long and horrible. Because it really is special, it is really is only
> allowed in that one special case (assuming the logic still holds) and
> every other possible case should catch the invalidate through the tlb
> flusher.

If I had a vote to cast, I would vote to always do invalidate_range() at the same
time the primary TLBs are flushed.  That seems completely logical and much harder
to screw up.  I might be a little biased though since KVM doesn't benefit from the
current shenanigans :-)

Re: some likely bugs in IOMMUv2 (in tlb_finish_mmu() nested flush and mremap())

[Jason Gunthorpe]

On Tue, Sep 27, 2022 at 12:24:41AM +0000, Sean Christopherson wrote:
> On Mon, Sep 26, 2022, Jason Gunthorpe wrote:
> > On Mon, Sep 26, 2022 at 08:13:00PM +0000, Sean Christopherson wrote:
> > 
> > > > AFAIK if we are flushing the CPU tlb then we really must also flush
> > > > the CPU tlb that KVM controls, and that is primarily what
> > > > invalidate_range() is used for.
> > > 
> > > As above, for its actual secondary MMU, KVM invalidates and flushes at
> > > invalidate_range_start(), and then prevents vCPUs from creating new entries for
> > > the range until invalidate_range_start_end().
> > 
> > Was it always like this? Why did we add this invalidate_range thing if
> > nothing really needed it?
> 
> No, the invalidate_range() hook was added by commit 1897bdc4d331 ("mmu_notifier:
> add mmu_notifier_invalidate_range()") for IOMMUs. 

Ah, right OK. This is specifically because the iommu is sharing the
*exact* page table of the CPU so the trick KVM/etc uses where 'start'
makes the shadow PTE non-present and then delays the fault until end
completes cannot work here.

>     The page-fault handler in the AMD IOMMUv2 driver doesn't handle the fault
>     if an invalidate_range_start/end pair is active, it just reports back
>     SUCCESS to the device and let it refault the page.  

Yah, this algorithm just doesn't really work, IMHO.. So it makes sense
we have invalidate_range as Joerg originally created it. Though the
GPU is still busted IMHO, there is no guarantee of forward progress
after some number of iterations, it is just much more likely if the
non-present is as narrow as possible.

So, then we can see where the end_only thing came from, commit
0f10851ea475 ("mm/mmu_notifier: avoid double notification when it is
useless") and that long winded message explains why some of the cases
must be ordered in the same place as the CPU flush, but doesn't
explain very much why it is OK to push it after beyond saying "ksm is
OK"

Looking at some of the places where 0f10851ea475 removed the notifies
they seem pretty pointless.

- fs/dax.c
  This never needed notify in the first place, it is populating a
  non-present PTE because it just faulted.

- __split_huge_zero_page_pmd()
  Sure, maybe, but who cares? The real fix here was changing
  __split_huge_pmd() to use only_end() because all paths already
  call invalidate_range

- copy_hugetlb_page_range()
  Sure, there is no CPU tlb flush.

  The CPU tlb flush on this path is in flush_tlb_mm() called by
  dup_mmap().

  The right thing to do is to ensure flush_tlb_mm() calls
  invalidate_range and skip it here. But the reasoning is not some
  "we are downgrading protections blah blah", the logic is that the
  CPU TLB flush can be delayed/consolidated so we can delay the
  shadow TLB flush too.

  (And why does copy_hugetlb_page_range use MMU_NOTIFY_CLEAR but 
   copy_p4d_range is bounded by MMU_NOTIFY_PROTECTION_PAGE ??)

- hugetlb_change_protection()
  Again I feel like the sensible thing here is to trigger the shadow
  flush in flush_hugetlb_tlb_range() always and use end_only

.. and so on ..

So, IMHO, we need to rewrite what 0f10851ea475 was trying to do in
order to fix the bug Jann noticed :\  That is bigger than I can knock
off while I write this email though ..

> > That means iommu is really the only place using it as a proper
> > synchronous shadow TLB flush.
> 
> More or less. There's also an "OpenCAPI coherent accelerator
> support" driver, drivers/misc/ocxl, that appears use
> invalidate_range() the same way the IOMMU does.  No idea how
> relevant that is these days.

Yeah, OpenCAPI is the same stuff as the IOMMU. Just PPC got away with
building all their IOMMU layer in its own arch specific subsystem :|

> I much prefer KVM's (and the old IOMMU's) approach of re-faulting in hardware until
> the entire sequence completes.   It _might_ be less performant, but I find it so
> much easier to reason about.  I actually had typed out a "can we just kill off
> mmu_notifier_invalidate_range() and force users to refault hardware" question
> before seeing the above changelog.

The key thing this requires is the ability to put the hardware into
fault mode (non-present), for the range under invalidation. If you
can't do that, then you can't use it.

> I don't know.  I found the series that introduced the behavior[*], but there are
> no numbers provided and I haven't been able to dredge up why this was even looked
> into in the first place.  From the cover letter:

It looks like a 'by inspection' project..

> If I had a vote to cast, I would vote to always do invalidate_range() at the same
> time the primary TLBs are flushed.  That seems completely logical and much harder
> to screw up.  I might be a little biased though since KVM doesn't benefit from the
> current shenanigans :-)

Me too.

Jason

Re: some likely bugs in IOMMUv2 (in tlb_finish_mmu() nested flush and mremap())

[John Hubbard]

On 9/28/22 11:12, Jason Gunthorpe wrote:
> On Tue, Sep 27, 2022 at 12:24:41AM +0000, Sean Christopherson wrote:
>> On Mon, Sep 26, 2022, Jason Gunthorpe wrote:
>>> On Mon, Sep 26, 2022 at 08:13:00PM +0000, Sean Christopherson wrote:
>>>
>>>>> AFAIK if we are flushing the CPU tlb then we really must also flush
>>>>> the CPU tlb that KVM controls, and that is primarily what
>>>>> invalidate_range() is used for.
>>>>
>>>> As above, for its actual secondary MMU, KVM invalidates and flushes at
>>>> invalidate_range_start(), and then prevents vCPUs from creating new entries for
>>>> the range until invalidate_range_start_end().
>>>
>>> Was it always like this? Why did we add this invalidate_range thing if
>>> nothing really needed it?
>>
>> No, the invalidate_range() hook was added by commit 1897bdc4d331 ("mmu_notifier:
>> add mmu_notifier_invalidate_range()") for IOMMUs.
> 
> Ah, right OK. This is specifically because the iommu is sharing the
> *exact* page table of the CPU so the trick KVM/etc uses where 'start'
> makes the shadow PTE non-present and then delays the fault until end
> completes cannot work here.

ohhh, is this trick something I should read more about, if I'm about to
jump in here?

> 
>>      The page-fault handler in the AMD IOMMUv2 driver doesn't handle the fault
>>      if an invalidate_range_start/end pair is active, it just reports back
>>      SUCCESS to the device and let it refault the page.
> 
> Yah, this algorithm just doesn't really work, IMHO.. So it makes sense
> we have invalidate_range as Joerg originally created it. Though the
> GPU is still busted IMHO, there is no guarantee of forward progress
> after some number of iterations, it is just much more likely if the
> non-present is as narrow as possible.
> 
> So, then we can see where the end_only thing came from, commit
> 0f10851ea475 ("mm/mmu_notifier: avoid double notification when it is
> useless") and that long winded message explains why some of the cases

I seem to recall that there was a performance drop involving GPUs, due
to the double notification. Just to fill in a little bit of history as
to why Jerome was trying to deduplicate the notifier callbacks.

> must be ordered in the same place as the CPU flush, but doesn't
> explain very much why it is OK to push it after beyond saying "ksm is
> OK"
> 
> Looking at some of the places where 0f10851ea475 removed the notifies
> they seem pretty pointless.
> 
> - fs/dax.c
>    This never needed notify in the first place, it is populating a
>    non-present PTE because it just faulted.
> 
> - __split_huge_zero_page_pmd()
>    Sure, maybe, but who cares? The real fix here was changing
>    __split_huge_pmd() to use only_end() because all paths already
>    call invalidate_range
> 
> - copy_hugetlb_page_range()
>    Sure, there is no CPU tlb flush.
> 
>    The CPU tlb flush on this path is in flush_tlb_mm() called by
>    dup_mmap().
> 
>    The right thing to do is to ensure flush_tlb_mm() calls
>    invalidate_range and skip it here. But the reasoning is not some
>    "we are downgrading protections blah blah", the logic is that the
>    CPU TLB flush can be delayed/consolidated so we can delay the
>    shadow TLB flush too.
> 
>    (And why does copy_hugetlb_page_range use MMU_NOTIFY_CLEAR but
>     copy_p4d_range is bounded by MMU_NOTIFY_PROTECTION_PAGE ??)
> 
> - hugetlb_change_protection()
>    Again I feel like the sensible thing here is to trigger the shadow
>    flush in flush_hugetlb_tlb_range() always and use end_only
> 
> .. and so on ..
> 
> So, IMHO, we need to rewrite what 0f10851ea475 was trying to do in
> order to fix the bug Jann noticed :\  That is bigger than I can knock
> off while I write this email though ..

After an initial pass through this, with perhaps 80% understanding
of the story, I'm reading that as:

     Audit all the sites (which you initially did quickly, above)
     that 0f10851ea475 touched, and any other related ones, and
     change things so that invalidate_range() and primary TLB
     flushing happen at the same point(s).

Yes? Anything else?

thanks,
-- 
John Hubbard
NVIDIA

> 
>>> That means iommu is really the only place using it as a proper
>>> synchronous shadow TLB flush.
>>
>> More or less. There's also an "OpenCAPI coherent accelerator
>> support" driver, drivers/misc/ocxl, that appears use
>> invalidate_range() the same way the IOMMU does.  No idea how
>> relevant that is these days.
> 
> Yeah, OpenCAPI is the same stuff as the IOMMU. Just PPC got away with
> building all their IOMMU layer in its own arch specific subsystem :|
> 
>> I much prefer KVM's (and the old IOMMU's) approach of re-faulting in hardware until
>> the entire sequence completes.   It _might_ be less performant, but I find it so
>> much easier to reason about.  I actually had typed out a "can we just kill off
>> mmu_notifier_invalidate_range() and force users to refault hardware" question
>> before seeing the above changelog.
> 
> The key thing this requires is the ability to put the hardware into
> fault mode (non-present), for the range under invalidation. If you
> can't do that, then you can't use it.
> 
>> I don't know.  I found the series that introduced the behavior[*], but there are
>> no numbers provided and I haven't been able to dredge up why this was even looked
>> into in the first place.  From the cover letter:
> 
> It looks like a 'by inspection' project..
> 
>> If I had a vote to cast, I would vote to always do invalidate_range() at the same
>> time the primary TLBs are flushed.  That seems completely logical and much harder
>> to screw up.  I might be a little biased though since KVM doesn't benefit from the
>> current shenanigans :-)
> 
> Me too.
> 
> Jason
> 

Re: some likely bugs in IOMMUv2 (in tlb_finish_mmu() nested flush and mremap())

[Jason Gunthorpe]

On Thu, Sep 29, 2022 at 07:31:03PM -0700, John Hubbard wrote:

> > Ah, right OK. This is specifically because the iommu is sharing the
> > *exact* page table of the CPU so the trick KVM/etc uses where 'start'
> > makes the shadow PTE non-present and then delays the fault until end
> > completes cannot work here.
> 
> ohhh, is this trick something I should read more about, if I'm about to
> jump in here?

All the invalidate_start implementations have a parallel page table
structure that they copy from the main page table into, eg using
hmm_range_fault() or whatever kvm does. Thus we can create a situation
where a sPTE is non-present while a PTE is present.

The iommu/etc point the HW at exactly the same page table as the mm,
so we cannot create a situation where a sPTE is non-present while the
PTE is present.

Thus, IMHO, the only correct answer is to flush the shadow TLB at
exactly the same moments we flush the CPU TLB because the two TLBs are
processing exactly the same data structure. If there is logic that the
TLB flush can be delayed to optimize it then that logic applies
equally to both TLBs.

> > So, then we can see where the end_only thing came from, commit
> > 0f10851ea475 ("mm/mmu_notifier: avoid double notification when it is
> > useless") and that long winded message explains why some of the cases
> 
> I seem to recall that there was a performance drop involving GPUs, due
> to the double notification. Just to fill in a little bit of history as
> to why Jerome was trying to deduplicate the notifier callbacks.

Sure, the double notification is clearly undesired, but the right way
to fix that was to remove the call to invalidate_range() from
mn_hlist_invalidate_end() which means adding any missing calls to
invalidate_range() near the CPU TLB

However, as Sean suggested, GPU drivers should not use
invalidate_range() because they are not directly sharing the CPU page
table in the first place. They should use start/end. Maybe the docs
need clarification on this point as well.

> After an initial pass through this, with perhaps 80% understanding
> of the story, I'm reading that as:
> 
>     Audit all the sites (which you initially did quickly, above)
>     that 0f10851ea475 touched, and any other related ones, and
>     change things so that invalidate_range() and primary TLB
>     flushing happen at the same point(s).
> 
> Yes? Anything else?

I would structure it as

1) Make a patch fixing the documentation around all of the 
   mmu_notifier_invalidate_range_only_end() to explain where the
   invalidate_range() call is (eg where the CPU TLB flush is) or why
   the CPU TLB flush is not actually needed. 0f10851ea475 is a good
   guide where to touch

   Remove all the confusing documentation about write protect and
   'promotion'. The new logic is we call invalidate_range when we
   flush the CPU TLB and we might do that outside the start/end
   block.

2) Make patch(es) converting all the places calling
   mmu_notifier_invalidate_range_end() to only_end() by identify where
   the CPU TLB flush is and ensuring the invalidate_range is present
   at the CPU TLB flush point.

   eg all the range_end() calls on the fork() path are switched to
   only_end() and an invalidate_range() put outside the
   start/end/block in dup_mmap() near the TLB flush. This is even more
   optimal because we batch flushing the entire shadown TLB in one
   shot, instead of trying to invalidate every VMA range.

3) Fix the TLB flusher to not send -1 -1 in the corner Jann noticed in
   the first mail

Jason