alpha: potential race around hae_cache in RESTORE_ALL

alpha: potential race around hae_cache in RESTORE_ALL

[Al Viro]

	What happens if we get to RESTORE_ALL with interrupts enabled,
find that we want to restore HAE, get to
        stq     $21, HAE_CACHE($19);    \
and get hit by an interrupt right after that assignment?  Note that
*alpha_mv->hae_register is still not updated, but alpha_mv->hae_cache
already is, so if the interrupt calls set_hae() it would get seriously
confused if the value it wants is equal to the value we've put into
->hae_cache.

	Until ~2002 it used to have a couple of swpipl around these
assignments and __set_hae() is still doing those.  I agree that on
many exits we *will* have interrupts disabled when we get to RESTORE_ALL,
but not on all of them.  E.g. any interrupt taken in kernel mode will
happily go to restore_all without bothering with swpipl at all.

	AFAICS, it looks like a race; the change in question had been
introduced in "Update Alpha UP for thread_info and scheduler changes"
(Feb 10 2002, commit 374eeee8a8a50e12278dfa37021df7b6efe506c3 in historical
git tree).

	Comments?

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Linus Torvalds]

On Sat, Sep 25, 2010 at 11:13 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>        What happens if we get to RESTORE_ALL with interrupts enabled,
> find that we want to restore HAE, get to
>        stq     $21, HAE_CACHE($19);    \
> and get hit by an interrupt right after that assignment?

Ok, so it's been absolutely ages since I touched the HAE stuff, but I
think the logic was that interrupts will always restore HAE to the
pre-interrupt state on exit, so reading it was always supposedly
race-free and didn't need any protection from normal code.

But yes, any actual _changes_ to HAE had better protect against
interrupts, so that they don't see the half-done state.

So yes, I think you found a bug.

I also don't see why that crazy RESTORE_ALL code does that reload of
$0 and $1 when it updates HAE. Is that just legacy from it having
_used_ to do the swipl PAL-call and that trashed those registers?

Anyway, I think the fix should be that we really always do have
interrupts disabled in RESTORE_ALL, rather than re-introduce the swipl
into the RESTORE_ALL sequence. A lot of the critical sequences already
do that - notably the normal return-to-user space code sequence that
is the really critical one.

                                          Linus

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Al Viro]

On Sat, Sep 25, 2010 at 11:42:41AM -0700, Linus Torvalds wrote:
> Ok, so it's been absolutely ages since I touched the HAE stuff, but I
> think the logic was that interrupts will always restore HAE to the
> pre-interrupt state on exit, so reading it was always supposedly
> race-free and didn't need any protection from normal code.
> 
> But yes, any actual _changes_ to HAE had better protect against
> interrupts, so that they don't see the half-done state.
> 
> So yes, I think you found a bug.
> 
> I also don't see why that crazy RESTORE_ALL code does that reload of
> $0 and $1 when it updates HAE. Is that just legacy from it having
> _used_ to do the swipl PAL-call and that trashed those registers?

Looks like...  swpipl takes argument in $16 and returns value in $0;
trashes $1, $16, $22--$25.  Out of those, RESTORE_ALL deals with $22--$25
later in the sequence and $16 it handled by rti.

> Anyway, I think the fix should be that we really always do have
> interrupts disabled in RESTORE_ALL, rather than re-introduce the swipl
> into the RESTORE_ALL sequence. A lot of the critical sequences already
> do that - notably the normal return-to-user space code sequence that
> is the really critical one.

Agreed.  There are only two places that need change (return to kernel path
in ret_from_syscall and kernel_thread essentially jumping to the same path),
so I'll just add
ret_to_kernel:
        lda     $16, 7
        call_pal PAL_swpipl
	br	restore_all
and make these two places go to ret_to_kernel instead of restore_all.

BTW, another bug on alpha: coredump puts rdusp() result into regsets for
_all_ threads.  Should be ti->pcb.usp for everything except current, a-la
KSTK_ESP logics...

I'll send fixes shortly

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Al Viro]

On Sat, Sep 25, 2010 at 08:18:36PM +0100, Al Viro wrote:
> Agreed.  There are only two places that need change (return to kernel path
> in ret_from_syscall and kernel_thread essentially jumping to the same path),
> so I'll just add
> ret_to_kernel:
>         lda     $16, 7
>         call_pal PAL_swpipl
> 	br	restore_all
> and make these two places go to ret_to_kernel instead of restore_all.

BTW, am I right assuming that HAE modifications is UP-only thing?  It would
be obviously b0rken on any SMP box, since alpha_mv is not per-CPU thing...

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Linus Torvalds]

Gaah. This got bounced, because that idiotic android gmail client
sends emails out as html even though they are just plain text. And
lkml (correctly) thinks that html emails are likely to be spam and
useless.

Too bad. I like being able to pen some replies on the road, and
android is pretty good at it otherwise.

Btw Al, I just noticed that you only cc'd me and rth. These days, for
alpha stuff, you probably should have added at least Matt Turner to
the cc too. I don't think Richard has been active in the alpha
community for many years now.

Matt - see the whole discussion on lkml (or I can forward you the
emails, if you want to).

                    Linus

On Sat, Sep 25, 2010 at 1:14 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> I think it's not only UP-only, I think it ends up being an issue only on
> really old machines.
>
> The newer alpha's have byte/word operations and thus have enough physical
> address space to just map all the IO in at the same time. It was only the
> old crappy alphas that needed the whole windowing thing due to the whole
> silly "encode operand size in the address" thing.
>
> But I forget exactly where the cut-off point is.
>
>          Linus
>
> On Sep 25, 2010 12:26 PM, "Al Viro" <viro@zeniv.linux.org.uk> wrote:
>>
>> BTW, am I right assuming that HAE modifications is UP-only thing? It would
>> be obviously b0rken on any SMP box, since alpha_mv is not per-CPU thing...
>

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Ivan Kokshaysky]

On Sat, Sep 25, 2010 at 08:25:09PM +0100, Al Viro wrote:
> BTW, am I right assuming that HAE modifications is UP-only thing?  It would
> be obviously b0rken on any SMP box, since alpha_mv is not per-CPU thing...

The only SMP system that does HAE modifications at runtime is T2, so it has
a spinlock protection around set_hae() - see core_t2.h. Others are either
limited to use HAE window 0 only, or do not have HAE hardware at all.

Ivan.

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Al Viro]

On Mon, Sep 27, 2010 at 11:58:28AM +0400, Ivan Kokshaysky wrote:
> On Sat, Sep 25, 2010 at 08:25:09PM +0100, Al Viro wrote:
> > BTW, am I right assuming that HAE modifications is UP-only thing?  It would
> > be obviously b0rken on any SMP box, since alpha_mv is not per-CPU thing...
> 
> The only SMP system that does HAE modifications at runtime is T2, so it has
> a spinlock protection around set_hae() - see core_t2.h. Others are either
> limited to use HAE window 0 only, or do not have HAE hardware at all.

Um?  Pardon me, but that makes no sense; how would a spinlock taken in
e.g. readl() stop another process from leaving a syscall, getting to
RESTORE_ALL and overwriting HAE register while we are halfway through
the spinlock-protected area?

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Al Viro]

On Mon, Sep 27, 2010 at 01:12:28PM +0100, Al Viro wrote:
> On Mon, Sep 27, 2010 at 11:58:28AM +0400, Ivan Kokshaysky wrote:
> > On Sat, Sep 25, 2010 at 08:25:09PM +0100, Al Viro wrote:
> > > BTW, am I right assuming that HAE modifications is UP-only thing?  It would
> > > be obviously b0rken on any SMP box, since alpha_mv is not per-CPU thing...
> > 
> > The only SMP system that does HAE modifications at runtime is T2, so it has
> > a spinlock protection around set_hae() - see core_t2.h. Others are either
> > limited to use HAE window 0 only, or do not have HAE hardware at all.
> 
> Um?  Pardon me, but that makes no sense; how would a spinlock taken in
> e.g. readl() stop another process from leaving a syscall, getting to
> RESTORE_ALL and overwriting HAE register while we are halfway through
> the spinlock-protected area?

AFAICS, we have 3 variants:
	1) alpha_mv.hae_register == &alpha_mv.hae_cache; all that code
becomes a no-op.
	2) UP boxen with hae_register pointing someplace real; we save
HAE in SAVE_ALL, restore it in RESTORE_ALL and disable interrupts around
the updates of hae_cache/*hae_register to keep them in sync.  readl()
et.al. set HAE, then do memory access and rely on not giving CPU up between
these moments.  Since alpha doesn't do PREEMPT, we are OK (otherwise we'd
needed to disable preempt in those places; also not a big deal)
	3) SMP t2 boxen; we protect the entire sequence from setting HAE to
memory access with spinlock and with disabling interrupts.  We don't rely on
interrupts not modifying the damn thing, but we *do* rely on other CPU not
messing with HAE on syscall paths outside of spinlock-protected area.  And
we have RESTORE_ALL hit us on all exits to userland, interrupt, trap and
syscall alike.

	Looks like (3) has always been broken...

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Ivan Kokshaysky]

On Mon, Sep 27, 2010 at 01:46:24PM +0100, Al Viro wrote:
> AFAICS, we have 3 variants:
> 	1) alpha_mv.hae_register == &alpha_mv.hae_cache; all that code
> becomes a no-op.
> 	2) UP boxen with hae_register pointing someplace real; we save
> HAE in SAVE_ALL, restore it in RESTORE_ALL and disable interrupts around
> the updates of hae_cache/*hae_register to keep them in sync.  readl()
> et.al. set HAE, then do memory access and rely on not giving CPU up between
> these moments.  Since alpha doesn't do PREEMPT, we are OK (otherwise we'd
> needed to disable preempt in those places; also not a big deal)
> 	3) SMP t2 boxen; we protect the entire sequence from setting HAE to
> memory access with spinlock and with disabling interrupts.  We don't rely on
> interrupts not modifying the damn thing, but we *do* rely on other CPU not
> messing with HAE on syscall paths outside of spinlock-protected area.  And
> we have RESTORE_ALL hit us on all exits to userland, interrupt, trap and
> syscall alike.
> 
> 	Looks like (3) has always been broken...

Ah, agreed with all of the above.

Looks like we need to drop HAE bits from SAVE_ALL/RESTORE_ALL, which
benefits (1) and automatically fixes (3), and do the entire IO sequences
in (2) with disabled interrupts (if HAE is involved).
The latter includes apecs, lca and jensen.

Ivan.

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Linus Torvalds]

On Mon, Sep 27, 2010 at 9:26 AM, Ivan Kokshaysky
<ink@jurassic.park.msu.ru> wrote:
>
> Looks like we need to drop HAE bits from SAVE_ALL/RESTORE_ALL, which
> benefits (1) and automatically fixes (3), and do the entire IO sequences
> in (2) with disabled interrupts (if HAE is involved).

No can do.

HAE is used in user space too (the X server), and it depends on the
kernel restoring HAE over interrupts and system calls, afaik.

I'm also pretty certain that all SMP machines either don't have HAE at
all, or have a per-CPU HAE in hardware (and then it's possible that we
screw it up in software, of course). Anything else would be too broken
for words. Can somebody find documentation saying otherwise?

                        Linus

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Richard Henderson]

On 09/27/2010 10:10 AM, Linus Torvalds wrote:
> On Mon, Sep 27, 2010 at 9:26 AM, Ivan Kokshaysky
> <ink@jurassic.park.msu.ru> wrote:
>>
>> Looks like we need to drop HAE bits from SAVE_ALL/RESTORE_ALL, which
>> benefits (1) and automatically fixes (3), and do the entire IO sequences
>> in (2) with disabled interrupts (if HAE is involved).
> 
> No can do.
> 
> HAE is used in user space too (the X server), and it depends on the
> kernel restoring HAE over interrupts and system calls, afaik.
> 
> I'm also pretty certain that all SMP machines either don't have HAE at
> all, or have a per-CPU HAE in hardware (and then it's possible that we
> screw it up in software, of course). Anything else would be too broken
> for words. Can somebody find documentation saying otherwise?

I'm pretty sure T2 is too broken for words, and RAWHIDE has
wide enough HAE that we can get away with leaving it at
defaults most of the time.

That said, I'm pretty sure I've never heard of anyone trying
T2 that wasn't at DEC.  I'm pretty sure if we deleted it, no
one would notice.

But all the respectable smp hardware doesn't use the HAE.


r~

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Al Viro]

On Mon, Sep 27, 2010 at 10:10:40AM -0700, Linus Torvalds wrote:
> On Mon, Sep 27, 2010 at 9:26 AM, Ivan Kokshaysky
> <ink@jurassic.park.msu.ru> wrote:
> >
> > Looks like we need to drop HAE bits from SAVE_ALL/RESTORE_ALL, which
> > benefits (1) and automatically fixes (3), and do the entire IO sequences
> > in (2) with disabled interrupts (if HAE is involved).
> 
> No can do.
> 
> HAE is used in user space too (the X server), and it depends on the
> kernel restoring HAE over interrupts and system calls, afaik.
> 
> I'm also pretty certain that all SMP machines either don't have HAE at
> all, or have a per-CPU HAE in hardware (and then it's possible that we
> screw it up in software, of course). Anything else would be too broken
> for words. Can somebody find documentation saying otherwise?

Besides, I'm not particulary happy to force a couple of swpipl on each
iomem access on old UP alpha boxen.

The only flavours that have HAE at all are APECS, LCA, MCPCIA, JENSEN and
T2.  APECS, LCA and JENSEN are UP-only, MCPCIA we build with
MCPCIA_ONE_HAE_WINDOW which blocks HAE switching AFAICS.  So it's really
about T2 and there we have something interesting:

#define T2_HAE_1                (IDENT_ADDR + GAMMA_BIAS + 0x38e0000e0UL)
#define T2_HAE_2                (IDENT_ADDR + GAMMA_BIAS + 0x38e000100UL)
#define T2_HAE_3                (IDENT_ADDR + GAMMA_BIAS + 0x38e000240UL)
#define T2_HAE_4                (IDENT_ADDR + GAMMA_BIAS + 0x38e000260UL)
#define T2_HAE_ADDRESS          T2_HAE_1

And seeing that it appears to be 4-CPU chipset...

Re: alpha: potential race around hae_cache in RESTORE_ALL

[Ivan Kokshaysky]

On Mon, Sep 27, 2010 at 08:01:09PM +0100, Al Viro wrote:
> The only flavours that have HAE at all are APECS, LCA, MCPCIA, JENSEN and
> T2.  APECS, LCA and JENSEN are UP-only, MCPCIA we build with
> MCPCIA_ONE_HAE_WINDOW which blocks HAE switching AFAICS.  So it's really
> about T2 and there we have something interesting:
> 
> #define T2_HAE_1                (IDENT_ADDR + GAMMA_BIAS + 0x38e0000e0UL)
> #define T2_HAE_2                (IDENT_ADDR + GAMMA_BIAS + 0x38e000100UL)
> #define T2_HAE_3                (IDENT_ADDR + GAMMA_BIAS + 0x38e000240UL)
> #define T2_HAE_4                (IDENT_ADDR + GAMMA_BIAS + 0x38e000260UL)
> #define T2_HAE_ADDRESS          T2_HAE_1
> 
> And seeing that it appears to be 4-CPU chipset...

Yeah, looks like per-cpu stuff, but no... From core_t2.c:

	/* Zero HAE.  */                                                        
	*(vulp)T2_HAE_1 = 0; mb(); /* Sparse MEM HAE */                         
	*(vulp)T2_HAE_2 = 0; mb(); /* Sparse I/O HAE */                         
	*(vulp)T2_HAE_3 = 0; mb(); /* Config Space HAE */                       

        /*                                                                      
	* We also now zero out HAE_4, the dense memory HAE, so that            
...

However, a good thing about T2 is that the Sparse HAE window is reasonably
large - 128 Mb, exactly as on MCPCIA. So we can easily do T2_ONE_HAE_WINDOW
and be done with it.

Ivan.

[PATCH] alpha: fix hae_cache race in RESTORE_ALL

[Al Viro]

We want interrupts disabled on all paths leading to RESTORE_ALL;
otherwise, we are risking an IRQ coming between the updates of
alpha_mv->hae_cache and *alpha_mv->hae_register and set_hae()
within the IRQ getting badly confused.

RESTORE_ALL used to play with disabling IRQ itself, but that got
removed back in 2002, without making sure we had them disabled
on all paths.  It's cheaper to make sure we have them disabled than
to revert to original variant...

Remove the detritus left from that commit back in 2002; we used to
need a reload of $0 and $1 since swpipl would change those, but
doing that had become pointless when we stopped doing swpipl in
there...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 arch/alpha/kernel/entry.S |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/alpha/kernel/entry.S b/arch/alpha/kernel/entry.S
index ab1ee0a..c674b92 100644
--- a/arch/alpha/kernel/entry.S
+++ b/arch/alpha/kernel/entry.S
@@ -73,8 +73,6 @@
 	ldq	$20, HAE_REG($19);	\
 	stq	$21, HAE_CACHE($19);	\
 	stq	$21, 0($20);		\
-	ldq	$0, 0($sp);		\
-	ldq	$1, 8($sp);		\
 99:;					\
 	ldq	$19, 72($sp);		\
 	ldq	$20, 80($sp);		\
@@ -316,7 +314,7 @@ ret_from_sys_call:
 	cmovne	$26, 0, $19		/* $19 = 0 => non-restartable */
 	ldq	$0, SP_OFF($sp)
 	and	$0, 8, $0
-	beq	$0, restore_all
+	beq	$0, ret_to_kernel
 ret_to_user:
 	/* Make sure need_resched and sigpending don't change between
 		sampling and the rti.  */
@@ -328,6 +326,10 @@ ret_to_user:
 restore_all:
 	RESTORE_ALL
 	call_pal PAL_rti
+ret_to_kernel:
+	lda	$16, 7
+	call_pal PAL_swpipl
+	br restore_all
 
 	.align 3
 $syscall_error:
@@ -657,7 +659,7 @@ kernel_thread:
 	/* We don't actually care for a3 success widgetry in the kernel.
 	   Not for positive errno values.  */
 	stq	$0, 0($sp)		/* $0 */
-	br	restore_all
+	br	ret_to_kernel
 .end kernel_thread
 
 /*
-- 
1.5.6.5

[PATCH] alpha: fix usp value in multithreaded coredumps

[Al Viro]

rdusp() gives us the right value only for the current thread...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 arch/alpha/kernel/process.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/alpha/kernel/process.c b/arch/alpha/kernel/process.c
index 842dba3..3ec3506 100644
--- a/arch/alpha/kernel/process.c
+++ b/arch/alpha/kernel/process.c
@@ -356,7 +356,7 @@ dump_elf_thread(elf_greg_t *dest, struct pt_regs *pt, struct thread_info *ti)
 	dest[27] = pt->r27;
 	dest[28] = pt->r28;
 	dest[29] = pt->gp;
-	dest[30] = rdusp();
+	dest[30] = ti == current_thread_info() ? rdusp() : ti->pcb.usp;
 	dest[31] = pt->pc;
 
 	/* Once upon a time this was the PS value.  Which is stupid
-- 
1.5.6.5