[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Will Deacon]

In 52af9c6c ("ARM: 6943/1: mm: use TTBR1 instead of reserved context ID")
I updated the ASID rollover code to use only the kernel page tables
whilst updating the ASID.

Unfortunately, the code to restore the user page tables was part of a
later patch which isn't yet in mainline, so this leaves the code
quite broken.

This patch fixes the issue by calling cpu_switch_mm to change the ASID
which has the side-effect of setting up TTBR0 to point to the user
tables.

Reported-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

Russell - I've reposted this to the list because it somehow got lost in
the archive and you've expressed some concerns over the code via the
patch system. I think the only opportunity for a race is when a CPU
doing switch_mm is interrupted by a rollover event occurring on another
core, but this is something that exists in the current code anyway and
is not affected by this patch.

 arch/arm/mm/context.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/context.c b/arch/arm/mm/context.c
index 8bfae96..2352395 100644
--- a/arch/arm/mm/context.c
+++ b/arch/arm/mm/context.c
@@ -100,8 +100,7 @@ static void reset_context(void *info)
 	set_mm_context(mm, asid);
 
 	/* set the new ASID */
-	asm("mcr	p15, 0, %0, c13, c0, 1\n" : : "r" (mm->context.id));
-	isb();
+	cpu_switch_mm(mm->pgd, mm);
 }
 
 #else
-- 
1.7.0.4

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Catalin Marinas]

On 7 June 2011 11:38, Will Deacon <will.deacon@arm.com> wrote:
> In 52af9c6c ("ARM: 6943/1: mm: use TTBR1 instead of reserved context ID")
> I updated the ASID rollover code to use only the kernel page tables
> whilst updating the ASID.
>
> Unfortunately, the code to restore the user page tables was part of a
> later patch which isn't yet in mainline, so this leaves the code
> quite broken.

IOW, after an ASID roll-over on SMP, the cross-called reset_context()
function sets TTBR0 to swapper_pg_dir but never sets it back to the
one of the currently running process. So interrupted user space
processes would fault when returning from an ASID roll-over event
happening on a different CPU.

> This patch fixes the issue by calling cpu_switch_mm to change the ASID
> which has the side-effect of setting up TTBR0 to point to the user
> tables.
>
> Reported-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Russell King - ARM Linux]

On Tue, Jun 07, 2011 at 11:38:38AM +0100, Will Deacon wrote:
> Russell - I've reposted this to the list because it somehow got lost in
> the archive and you've expressed some concerns over the code via the
> patch system. I think the only opportunity for a race is when a CPU
> doing switch_mm is interrupted by a rollover event occurring on another
> core, but this is something that exists in the current code anyway and
> is not affected by this patch.

However, these patches are introducing a brand new race between the
switch_mm code and the reset_context code.

With the new switch_mm() code, we switch TTBR0 to be the same as TTBR1.
If we then receive an IPI for reset_context(), we will change TTBR0
to point at a set of page tables which don't contain just global mappings.

After returning from reset_context(), we will resume switch_mm(), and
change the ASID value with the page tables pointing to non-global
mappings, violating the whole reason for the switch_mm() change.

The only way around this is to make reset_context() preserve the TTBR0
value across itself, by reading it initially and then restoring before
returning.

So, even though the current code is broken, I'm not applying this patch
as it isn't anywhere near right - and we can do right quite easily here.

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Will Deacon]

Hi Russell,

Thanks for replying to this.

On Wed, Jun 08, 2011 at 09:01:06PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 07, 2011 at 11:38:38AM +0100, Will Deacon wrote:
> > Russell - I've reposted this to the list because it somehow got lost in
> > the archive and you've expressed some concerns over the code via the
> > patch system. I think the only opportunity for a race is when a CPU
> > doing switch_mm is interrupted by a rollover event occurring on another
> > core, but this is something that exists in the current code anyway and
> > is not affected by this patch.
> 
> However, these patches are introducing a brand new race between the
> switch_mm code and the reset_context code.
> 
> With the new switch_mm() code, we switch TTBR0 to be the same as TTBR1.
> If we then receive an IPI for reset_context(), we will change TTBR0
> to point at a set of page tables which don't contain just global mappings.
> 
> After returning from reset_context(), we will resume switch_mm(), and
> change the ASID value with the page tables pointing to non-global
> mappings, violating the whole reason for the switch_mm() change.

Whilst this is a new race condition, it is analagous to the one we have already
and could be fixed at the same time.

> The only way around this is to make reset_context() preserve the TTBR0
> value across itself, by reading it initially and then restoring before
> returning.

I don't think this is safe. The reset_context() broadcast could interrupt us
at a time where current_mm has been updated during context switch, but TTBR0
still contains the page tables of the previous mm. If we blindly save and
restore the value from the hardware, we could end up setting the wrong ASID and
then we're back to square one.

> So, even though the current code is broken, I'm not applying this patch
> as it isn't anywhere near right - and we can do right quite easily here.

I think the easiest fix is to go with my original proposal of disabling
interrupts during switch_mm. Then this patch will work as intended and we'll
eliminate the original race too. Since the interrupts need only be disabled for
SMP systems, it won't affect any platforms with VIVT D-caches, where interupts
should be left enabled while the cache is flushed.

Any ideas?

Will

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Russell King - ARM Linux]

On Wed, Jun 08, 2011 at 09:23:23PM +0100, Will Deacon wrote:
> On Wed, Jun 08, 2011 at 09:01:06PM +0100, Russell King - ARM Linux wrote:
> > However, these patches are introducing a brand new race between the
> > switch_mm code and the reset_context code.
> > 
> > With the new switch_mm() code, we switch TTBR0 to be the same as TTBR1.
> > If we then receive an IPI for reset_context(), we will change TTBR0
> > to point at a set of page tables which don't contain just global mappings.
> > 
> > After returning from reset_context(), we will resume switch_mm(), and
> > change the ASID value with the page tables pointing to non-global
> > mappings, violating the whole reason for the switch_mm() change.
> 
> Whilst this is a new race condition, it is analagous to the one we have
> already and could be fixed at the same time.

Ok, I think we should revert the original patches then.  They were rushed
in during the merge window, and as can be seen, rushing in patches because
we _think_ they're right is never the correct thing to do - we've ended
up with a completely broken situation as stuff now stands.

Let's take out these changes and sort it out properly - not only do we
need to sort out these problems but we should also get rid of the
__ARCH_WANT_INTERRUPTS_ON_CTXSW thing completely.  I have a patch which
I've only tested on SA-1110 which does this so far, but it needs a little
more work to clean up some stuff.

So, we're weren't - and still aren't - ready for any of these changes.

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Will Deacon]

On Wed, Jun 08, 2011 at 09:36:15PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 08, 2011 at 09:23:23PM +0100, Will Deacon wrote:
> > On Wed, Jun 08, 2011 at 09:01:06PM +0100, Russell King - ARM Linux wrote:
> > > However, these patches are introducing a brand new race between the
> > > switch_mm code and the reset_context code.
> > > 
> > > With the new switch_mm() code, we switch TTBR0 to be the same as TTBR1.
> > > If we then receive an IPI for reset_context(), we will change TTBR0
> > > to point at a set of page tables which don't contain just global mappings.
> > > 
> > > After returning from reset_context(), we will resume switch_mm(), and
> > > change the ASID value with the page tables pointing to non-global
> > > mappings, violating the whole reason for the switch_mm() change.
> > 
> > Whilst this is a new race condition, it is analagous to the one we have
> > already and could be fixed at the same time.
> 
> Ok, I think we should revert the original patches then.  They were rushed
> in during the merge window, and as can be seen, rushing in patches because
> we _think_ they're right is never the correct thing to do - we've ended
> up with a completely broken situation as stuff now stands.

Seems a shame given that disabling interrupts during switch_mm would fix this,
but yes, reverting them is certainly better than having a broken kernel. I
think you just need to revert 6944/1 and 6943/1; the other two from that series
are fine to be left in.

> Let's take out these changes and sort it out properly - not only do we
> need to sort out these problems but we should also get rid of the
> __ARCH_WANT_INTERRUPTS_ON_CTXSW thing completely.  I have a patch which
> I've only tested on SA-1110 which does this so far, but it needs a little
> more work to clean up some stuff.

Could you post that code please? I can then rebase these patches against it for
future inclusion in mainline. Does the removal of
__ARCH_WANT_INTERRUPTS_ON_CTXSW mean that switch_mm will run with interrupts
disabled by default?

> So, we're weren't - and still aren't - ready for any of these changes.

Once the interrupt stuff is sorted out I can rebase the ASID stuff on top of it
and we should be ready to go.

Cheers,

Will

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Russell King - ARM Linux]

On Wed, Jun 08, 2011 at 09:49:49PM +0100, Will Deacon wrote:
> Seems a shame given that disabling interrupts during switch_mm would fix
> this, but yes, reverting them is certainly better than having a broken
> kernel. I think you just need to revert 6944/1 and 6943/1; the other two
> from that series are fine to be left in.

Ok.

> > Let's take out these changes and sort it out properly - not only do we
> > need to sort out these problems but we should also get rid of the
> > __ARCH_WANT_INTERRUPTS_ON_CTXSW thing completely.  I have a patch which
> > I've only tested on SA-1110 which does this so far, but it needs a little
> > more work to clean up some stuff.
> 
> Could you post that code please? I can then rebase these patches against
> it for future inclusion in mainline.

I'd like to finish sorting it out properly first, so that it's all sane.

> Does the removal of
> __ARCH_WANT_INTERRUPTS_ON_CTXSW mean that switch_mm will run with interrupts
> disabled by default?

Yes, just like all but one other arch in the kernel.  The scheduler
folk would like to see this symbol die...

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Stephen Boyd]

Sorry to dig up an old thread....

On 06/08/11 13:55, Russell King - ARM Linux wrote:
> On Wed, Jun 08, 2011 at 09:49:49PM +0100, Will Deacon wrote:
>>> Let's take out these changes and sort it out properly - not only do we
>>> need to sort out these problems but we should also get rid of the
>>> __ARCH_WANT_INTERRUPTS_ON_CTXSW thing completely.  I have a patch which
>>> I've only tested on SA-1110 which does this so far, but it needs a little
>>> more work to clean up some stuff.
>> Could you post that code please? I can then rebase these patches against
>> it for future inclusion in mainline.
> I'd like to finish sorting it out properly first, so that it's all sane.
>
>> Does the removal of
>> __ARCH_WANT_INTERRUPTS_ON_CTXSW mean that switch_mm will run with interrupts
>> disabled by default?
> Yes, just like all but one other arch in the kernel.  The scheduler
> folk would like to see this symbol die...

Was this patch ever posted? This looks like a particularly annoying
problem to debug if it actually occurs (plus ARM is the final holdout
for this symbol now).

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Catalin Marinas]

On 8 June 2011 21:36, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Wed, Jun 08, 2011 at 09:23:23PM +0100, Will Deacon wrote:
>> On Wed, Jun 08, 2011 at 09:01:06PM +0100, Russell King - ARM Linux wrote:
>> > However, these patches are introducing a brand new race between the
>> > switch_mm code and the reset_context code.
>> >
>> > With the new switch_mm() code, we switch TTBR0 to be the same as TTBR1.
>> > If we then receive an IPI for reset_context(), we will change TTBR0
>> > to point at a set of page tables which don't contain just global mappings.
>> >
>> > After returning from reset_context(), we will resume switch_mm(), and
>> > change the ASID value with the page tables pointing to non-global
>> > mappings, violating the whole reason for the switch_mm() change.
>>
>> Whilst this is a new race condition, it is analagous to the one we have
>> already and could be fixed at the same time.
>
> Ok, I think we should revert the original patches then. ?They were rushed
> in during the merge window, and as can be seen, rushing in patches because
> we _think_ they're right is never the correct thing to do - we've ended
> up with a completely broken situation as stuff now stands.

We rushed a series of patches fixing this but you didn't like the
patch disabling interrupts around cpu_switch_mm(). This turned out to
be essential for avoiding the race condition.

Please note that the old switch_mm code with reserved ASID is broken
on A15 (and not just in theory), hence the need to use reserved TTBR0.

> Let's take out these changes and sort it out properly - not only do we
> need to sort out these problems but we should also get rid of the
> __ARCH_WANT_INTERRUPTS_ON_CTXSW thing completely. ?I have a patch which
> I've only tested on SA-1110 which does this so far, but it needs a little
> more work to clean up some stuff.

Even if you get rid of __ARCH_WANT_INTERRUPTS_ON_CTXSW, I would much
prefer to use the new switch_mm code as a base rather than going back
to the reserved ASID. The simplest way to fix the race condition you
mentioned is to also integrate the other patch from Will which
disables the interrupts around cpu_switch_mm(). After that we have
more time to review the __ARCH_WANT_INTERRUPTS_ON_CTXSW patch.

-- 
Catalin

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Russell King - ARM Linux]

On Wed, Jun 08, 2011 at 10:12:23PM +0100, Catalin Marinas wrote:
> On 8 June 2011 21:36, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > On Wed, Jun 08, 2011 at 09:23:23PM +0100, Will Deacon wrote:
> >> On Wed, Jun 08, 2011 at 09:01:06PM +0100, Russell King - ARM Linux wrote:
> >> > However, these patches are introducing a brand new race between the
> >> > switch_mm code and the reset_context code.
> >> >
> >> > With the new switch_mm() code, we switch TTBR0 to be the same as TTBR1.
> >> > If we then receive an IPI for reset_context(), we will change TTBR0
> >> > to point at a set of page tables which don't contain just global mappings.
> >> >
> >> > After returning from reset_context(), we will resume switch_mm(), and
> >> > change the ASID value with the page tables pointing to non-global
> >> > mappings, violating the whole reason for the switch_mm() change.
> >>
> >> Whilst this is a new race condition, it is analagous to the one we have
> >> already and could be fixed at the same time.
> >
> > Ok, I think we should revert the original patches then. ?They were rushed
> > in during the merge window, and as can be seen, rushing in patches because
> > we _think_ they're right is never the correct thing to do - we've ended
> > up with a completely broken situation as stuff now stands.
> 
> We rushed a series of patches fixing this but you didn't like the
> patch disabling interrupts around cpu_switch_mm().

No.  We rushed the entire thing as can be seen due to the "forgotten"
patch and the submission of it _during_ the merge window.  Clearly
no one had thought enough about the inter-relationship between the
patches when deciding that only some of the patches should go in.

That's why there's a big reason to avoid merging any new patches during
the merge window - it gives time for such things to be found before it
becomes a problem.

> Please note that the old switch_mm code with reserved ASID is broken
> on A15 (and not just in theory), hence the need to use reserved TTBR0.

But A15 is not actively supported in mainline yet so that's not a
decision relevant to what we do with mainline.

> > Let's take out these changes and sort it out properly - not only do we
> > need to sort out these problems but we should also get rid of the
> > __ARCH_WANT_INTERRUPTS_ON_CTXSW thing completely. ?I have a patch which
> > I've only tested on SA-1110 which does this so far, but it needs a little
> > more work to clean up some stuff.
> 
> Even if you get rid of __ARCH_WANT_INTERRUPTS_ON_CTXSW, I would much
> prefer to use the new switch_mm code as a base rather than going back
> to the reserved ASID. The simplest way to fix the race condition you
> mentioned is to also integrate the other patch from Will which
> disables the interrupts around cpu_switch_mm(). After that we have
> more time to review the __ARCH_WANT_INTERRUPTS_ON_CTXSW patch.

Once we have the context switching situation sorted for VIVT we can then
reconsider these patches - their pre-requisits will have been solved by
way of the VIVT situation being sorted.

[PATCH] ARM: mm: ensure TTBR0 is restored when changing ASID on rollover

[Will Deacon]

In 52af9c6c ("ARM: 6943/1: mm: use TTBR1 instead of reserved context ID")
I updated the ASID rollover code to use only the kernel page tables
whilst updating the ASID.

Unfortunately, the code to restore the user page tables was part of a
later patch which isn't yet in mainline, so this leaves the code
quite broken.

This patch fixes the issue by calling cpu_switch_mm to change the ASID
which has the side-effect of setting up TTBR0 to point to the user
tables.

Reported-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/mm/context.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/context.c b/arch/arm/mm/context.c
index 8bfae96..2352395 100644
--- a/arch/arm/mm/context.c
+++ b/arch/arm/mm/context.c
@@ -100,8 +100,7 @@ static void reset_context(void *info)
 	set_mm_context(mm, asid);
 
 	/* set the new ASID */
-	asm("mcr	p15, 0, %0, c13, c0, 1\n" : : "r" (mm->context.id));
-	isb();
+	cpu_switch_mm(mm->pgd, mm);
 }
 
 #else
-- 
1.7.0.4