LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrea Arcangeli <aarcange@redhat.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Rafael Aquini <aquini@redhat.com>,
	Mark Salter <msalter@redhat.com>,
	Jon Masters <jcm@jonmasters.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linux-arm-kernel@lists.infradead.org,
	Michal Hocko <mhocko@kernel.org>, QI Fuli <qi.fuli@fujitsu.com>
Subject: Re: [PATCH 3/3] arm64: tlb: skip tlbi broadcast
Date: Tue, 31 Mar 2020 20:32:03 -0400	[thread overview]
Message-ID: <20200401003203.GA12536@redhat.com> (raw)
In-Reply-To: <20200331094034.GA1131@C02TD0UTHF1T.local>

Hello Mark,

On Tue, Mar 31, 2020 at 10:45:11AM +0100, Mark Rutland wrote:
> Hi Andrea,
> 
> On Mon, Mar 16, 2020 at 02:09:07PM +0000, Mark Rutland wrote:
> > AFAICT, this series relies on:
> > 
> > * An ISB completing prior page table walks when updating TTBR. I don't
> >   believe this is necessarily the case, given how things work for an
> >   EL1->EL2 transition where there can be ongoing EL1 walks.
> 
> I've had confirmation that a DSB is necessary (after the MSR and ISB) to
> complete any ongoing translation table walks for the stale context.
> 
> Without a DSB, those walks can observe subsequent stores and encounter
> the usual set of CONSTRAINED UNPREDICTABLE behaviours (e.g. walking into
> MMIO with side-effects, continuing from amalgamted entries, etc). Those
> issues are purely to do with the walk, and apply regardless of whether
> the resulting translations are architecturally consumed.

Ok, sorry I didn't get it earlier... I attempted a quick fix below.

From ab30d8082be62fe24a97eceec5dbfeea8e278511 Mon Sep 17 00:00:00 2001
From: Andrea Arcangeli <aarcange@redhat.com>
Date: Tue, 31 Mar 2020 20:03:43 -0400
Subject: [PATCH 1/1] arm64: tlb: skip tlbi broadcast, fix speculative tlb
 lookups

Without DSB in between "MSR; ISB" and "atomic_dec(&nr_active_mm)"
there's the risk a speculative pagecache lookup may still be walking
pagetables of the unloaded asid after nr_active_mm has been
decreased. In such case the remote CPU could free the pagetables and
reuse the memory without first issuing a tlbi broadcast, while the
speculative tlb lookup still runs on the unloaded asid. For this
reason the speculative pagetable walks needs to be flushed before
decreasing nr_active_mm.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 arch/arm64/include/asm/mmu_context.h | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index 9c66fe317e2f..d821ea3ce839 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -210,8 +210,18 @@ enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
 	if (per_cpu(cpu_not_lazy_tlb, cpu) &&
 	    is_idle_task(tsk)) {
 		per_cpu(cpu_not_lazy_tlb, cpu) = false;
-		if (!system_uses_ttbr0_pan())
+		if (!system_uses_ttbr0_pan()) {
 			cpu_set_reserved_ttbr0();
+			/*
+			 * DSB will flush the speculative pagetable
+			 * walks on the old asid. It's required before
+			 * decreasing nr_active_mm because after
+			 * decreasing nr_active_mm the tlbi broadcast
+			 * may not happen on the unloaded asid before
+			 * the pagetables are freed.
+			 */
+			dsb(ish);
+		}
 		atomic_dec(&mm->context.nr_active_mm);
 	}
 	VM_WARN_ON(atomic_read(&mm->context.nr_active_mm) < 0);
@@ -249,6 +259,14 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 	} else if (prev != next) {
 		atomic_inc(&next->context.nr_active_mm);
 		__switch_mm(next, cpu);
+		/*
+		 * DSB will flush the speculative pagetable walks on the old
+		 * asid. It's required before decreasing nr_active_mm because
+		 * after decreasing nr_active_mm the tlbi broadcast may not
+		 * happen on the unloaded asid before the pagetables are
+		 * freed.
+		 */
+		dsb(ish);
 		atomic_dec(&prev->context.nr_active_mm);
 	}
 	VM_WARN_ON(!atomic_read(&next->context.nr_active_mm));


I didn't test it yet, because this being a theoretical issue it is
better reviewed in the source.

> > * Walks never being initiated for `inactive` contexts within the current
> >   translation regime. e.g. while ASID x is installed, never starting a
> >   walk for ASID y. I can imagine that the architecture may permit a form
> >   of this starting with intermediate walk entries in the TLBs.
> 
> I'm still chasing this point.

Appreciated! I'll cross fingers you don't find the speculative lookups
can randomly start on unloaded ASID. That would also imply that it
would be impossible on arm64 to use different asid on different CPUs
as it is normally done on other arches.

Thanks,
Andrea


  reply	other threads:[~2020-04-01 10:37 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-23 19:25 [PATCH 0/3] arm64: tlb: skip tlbi broadcast v2 Andrea Arcangeli
2020-02-23 19:25 ` [PATCH 1/3] mm: use_mm: fix for arches checking mm_users to optimize TLB flushes Andrea Arcangeli
2020-03-03  4:28   ` Rafael Aquini
2020-02-23 19:25 ` [PATCH 2/3] arm64: select CPUMASK_OFFSTACK if NUMA Andrea Arcangeli
2020-03-03  4:31   ` Rafael Aquini
2020-02-23 19:25 ` [PATCH 3/3] arm64: tlb: skip tlbi broadcast Andrea Arcangeli
2020-03-02 15:24   ` Rafael Aquini
2020-03-04  4:19     ` Rafael Aquini
2020-03-09 11:22   ` Catalin Marinas
2020-03-14  3:16     ` Andrea Arcangeli
2020-03-16 14:09       ` Mark Rutland
2020-03-31  9:45         ` Mark Rutland
2020-04-01  0:32           ` Andrea Arcangeli [this message]
2020-03-18  8:53 ` [PATCH 0/3] arm64: tlb: skip tlbi broadcast v2 qi.fuli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200401003203.GA12536@redhat.com \
    --to=aarcange@redhat.com \
    --cc=aquini@redhat.com \
    --cc=catalin.marinas@arm.com \
    --cc=jcm@jonmasters.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mark.rutland@arm.com \
    --cc=mhocko@kernel.org \
    --cc=msalter@redhat.com \
    --cc=qi.fuli@fujitsu.com \
    --cc=will@kernel.org \
    --subject='Re: [PATCH 3/3] arm64: tlb: skip tlbi broadcast' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).