From: Michael Ellerman <mpe@ellerman•id.au>
To: Gavin Shan <gwshan@linux•vnet.ibm.com>
Cc: linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH] powerpc/kernel: Avoid memory corruption at early stage
Date: Fri, 23 Jan 2015 11:47:02 +1100 [thread overview]
Message-ID: <1421974022.24984.1.camel@ellerman.id.au> (raw)
In-Reply-To: <20150122064032.GA19557@shangw>
On Thu, 2015-01-22 at 17:40 +1100, Gavin Shan wrote:
> On Thu, Jan 22, 2015 at 05:21:22PM +1100, Michael Ellerman wrote:
> >On Thu, 2015-01-08 at 16:40 +1100, Gavin Shan wrote:
> >> When calling to early_setup(), we picks "boot_paca" up for the
> >> master CPU and initialize that with initialise_paca(). At the
> >> point, SLB shadow buffer isn't populated yet. Updating the SLB
> >> shadow buffer should corrupt what we had in physical address 0
> >> where the trap instruction is usually stored.
> >
> >Ouch.
> >
> >Introduced in 6f4441ef7009 ("powerpc: Dynamically allocate slb_shadow from
> >memblock") - December 2013.
> >
> >So it seems it doesn't cause us any harm in general.
> >
> >Did you actually hit a bug with it?
>
> I didn't hit any bugs with it. So I guess it's unnecessary to mark it as
> "stable". I found the issue (not bug) occasionally: starting kernel in
> simulator, dumping the instruction at physical address 0x0 and found
> it's not "nop".
Yeah OK. So we'll merge it but it doesn't need to go to stable.
> >> diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
> >> index d6e195e..048a6ee 100644
> >> --- a/arch/powerpc/kernel/paca.c
> >> +++ b/arch/powerpc/kernel/paca.c
> >> @@ -115,6 +115,9 @@ static struct slb_shadow * __init init_slb_shadow(int cpu)
> >> {
> >> struct slb_shadow *s = &slb_shadow[cpu];
> >>
> >> + if (!slb_shadow)
> >> + return NULL;
> >> +
> >> s->persistent = cpu_to_be32(SLB_NUM_BOLTED);
> >> s->buffer_length = cpu_to_be32(sizeof(*s));
> >
> >Yeah I guess that's an OK fix.
> >
> >We must have a valid SLB shadow before we ever call _switch(), which is much
> >later. The only way we could hit this case for the real paca is if
> >allocate_slb_shadows() failed to allocate, but it would have panicked if it
> >did.
>
> Perhaps I can add BUG_ON(!slb_shadow) in allocate_slb_shadows() if you agree.
No need, memblock_alloc_base() already panics if it can't allocate.
cheers
prev parent reply other threads:[~2015-01-23 0:47 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-08 5:40 [PATCH] powerpc/kernel: Avoid memory corruption at early stage Gavin Shan
2015-01-22 6:21 ` Michael Ellerman
2015-01-22 6:40 ` Gavin Shan
2015-01-23 0:47 ` Michael Ellerman [this message]
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=1421974022.24984.1.camel@ellerman.id.au \
--to=mpe@ellerman$(echo .)id.au \
--cc=gwshan@linux$(echo .)vnet.ibm.com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox