Re: ppc64/book3s: copy interrupts till __end_handlers marker instead of __end_interrupts

[Michael Ellerman <mpe@ellerman.id.au>]

Hi Hari,

You win the "Best Change Log of the Year" award.

Some comments below ...

On Mon, 2016-28-03 at 11:23:22 UTC, Hari Bathini wrote:
> Some of the interrupt vectors on 64-bit POWER server processors  are
> only 32 bytes long (8 instructions), which is not enough for the full
> first-level interrupt handler. For these we need to branch to an out-
> of-line (OOL) handler. But when we are running a relocatable kernel,
> interrupt vectors till __end_interrupts marker are copied down to real
> address 0x100. So, branching to labels (read OOL handlers) outside this
> section should be handled differently (see LOAD_HANDLER()), considering
> relocatable kernel, which would need atleast 4 instructions.
> 
> However, branching from interrupt vector means that we corrupt the CFAR
> (come-from address register) on POWER7 and later processors as mentioned
> in commit 1707dd16. So, EXCEPTION_PROLOG_0
> (6 instructions) that contains the part up to the point where the CFAR is
> saved in the PACA should be part of the short interrupt vectors before we
> branch out to OOL handlers.
> 
> But as mentioned already, there are interrupt vectors on 64-bit POWER server
> processors that are only 32 bytes long (like vectors 0x4f00, 0x4f20, etc.),
> which cannot accomodate the above two cases at the same time owing to space
> constraint. Currently, in these interrupt vectors, we simply branch out to
> OOL handlers, without using LOAD_HANDLER(), which leaves us vulnerable when
> running a relocatable kernel (eg. kdump case). While this has been the case
> for sometime now and kdump is used widely, we were fortunate not to see any
> problems so far, for three reasons:
> 
>     1. In almost all cases, production kernel (relocatable) is used for
>        kdump as well, which would mean that crashed kernel's OOL handler
>        would be at the same place where we endup branching to, from short
>        interrupt vector of kdump kernel.
>     2. Also, OOL handler was unlikely the reason for crash in almost all
>        the kdump scenarios, which meant we had a sane OOL handler from
>        crashed kernel that we branched to.
>     3. On most 64-bit POWER server processors, page size is large enough
>        that marking interrupt vector code as executable (see commit
>        429d2e83) leads to marking OOL handler code from crashed kernel,
>        that sits right below interrupt vector code from kdump kernel, as
>        executable as well.
> 
> Let us fix this undependable code path firstly, by moving down __end_handlers
> marker down past OOL handlers. Secondly, copying interrupt vectors down till
> __end_handlers marker instead of __end_interrupts, when running a relocatable
> kernel, to make sure we endup in relocated (kdump) kernel's OOL handler instead
> of crashed kernel's. Thirdly, by marking all the interrupt vector code that is
> copied down to real address 0x100 as executable, considering the relocation on
> exception feature that allows exceptions to be raised in virtual mode (IR=DR=1).
> 
> This fix has been tested successfully in kdump scenario, on a lpar with 4K page
> size by using different default/production kernel and kdump kernel.

So I think you've missed one important case.

In do_final_fixups() we recopy the (now patched) kernel code down to zero. That
code uses __end_interrupts as its limit, so I think if you look closely your OOL
handlers down at zero will not have had feature fixups applied to them.

I think perhaps the better fix is just to move __end_interrupts down (up) to the
right location. AFAICS all users of __end_interrupts actually want that address.

It would also mean we could remove __end_handlers as unused.

So can you please check that I'm right about do_final_fixups(), and then try
moving __end_interrupts and check that works?

cheers