Re: [PATCH V4 1/7] x86/traps: Move pt_regs only in fixup_bad_iret()

[Lai Jiangshan]

On Thu, Apr 7, 2022 at 3:00 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Fri, Mar 18, 2022 at 10:30:10PM +0800, Lai Jiangshan wrote:
> > From: Lai Jiangshan <jiangshan.ljs@xxxxxxxxxxxx>
> >
> > fixup_bad_iret() and sync_regs() have similar arguments and do similar
> > work that copies full or partial pt_regs to a place and switches stack
> > after return.  They are quite the same, but fixup_bad_iret() not only
> > copies the pt_regs but also the return address of error_entry() while
>
> What return address of error_entry()? You lost me here.

"return address" is the return address of a function which is
error_entry() here.

https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html

Or error_entry_ret in struct bad_iret_stack which is being removed
in the change.


>
> So fixup_bad_iret() gets the stack ptr passed in by doing:
>
>         mov     %rsp, %rdi
>         call    fixup_bad_iret
>         mov     %rax, %rsp
>
>
> and error_regs()
>
>         movq    %rsp, %rdi                      /* arg0 = pt_regs pointer */
>         call    sync_regs
>         movq    %rax, %rsp                      /* switch stack */
>
> the same way.

They are not the same way.

sync_regs() is called before the return address of error_entry()
popped into %r12 while fixup_bad_iret() is called with the return
address of error_entry() still on the stack.  And the primitives of
fixup_bad_iret() and sync_regs() are different which also means
they are not the same way.

After this change, they become the same way.

IMO, sync_regs() is grace while fixup_bad_iret() is a bad C function
or is not a pure C function because it is handling the return address
of its parent function which is better done by the compiler or ASM
code.

>
> Confused.
>
> > It is prepared for later patch to do the stack switch after the
> > error_entry() which simplifies the code further.
>
> Looking at your next patch, is all this dance done just so that you can
> do
>
>         leaq    8(%rsp), %rdi
>
> in order to pass in pt_regs to both functions?
>
> And get rid of the saving/restoring %r12?
>
> Is that what the whole noise is about?

The point is to make fixup_bad_iret() a normal C function and
the same as sync_regs() in calling.

The next patch makes error_entry() as a bunch of ASM code compiled
from a C function and pave the road to really convert it to a C
function.

Getting rid of the saving/restoring the return address in %r12
is necessary since a C function can't save/restore the return
address.

Thanks
Lai

>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

I'm sorry for using top-posting and "This patch".  I remember it.