git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
It was writing over %esp/pt_regs semi-randomly on i686 with the expected
"system can't boot" results. As noted in:
https://bugs.freedesktop.org/show_bug.cgi?id=85277
This patch stops fscking with pt_regs. Instead it sets up the registers
for the call to __audit_syscall_entry in the most obvious conceivable
way. It then does just a tiny tiny touch of magic. We need to get what
started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
as a pair of pushes.
After the call to __audit_syscall_entry all we need to do is get that
now useless junk off the stack (pair of pops) and reload %eax with the
original syscall so other stuff can keep going about it's business.
Signed-off-by: Eric Paris <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
arch/x86/kernel/entry_32.S | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index f9e3fab..fb01d22 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -447,15 +447,14 @@ sysenter_exit:
sysenter_audit:
testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
jnz syscall_trace_entry
- addl $4,%esp
- CFI_ADJUST_CFA_OFFSET -4
- movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
- movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
- /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
- movl %ebx,%edx /* 2nd arg: 1st syscall arg */
- /* %eax already in %eax 1st arg: syscall number */
+ /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
+ movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
+ /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
+ pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
+ pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
call __audit_syscall_entry
- pushl_cfi %ebx
+ popl_cfi %ecx /* get that remapped edx off the stack */
+ popl_cfi %ecx /* get that remapped esi off the stack */
movl PT_EAX(%esp),%eax /* reload syscall number */
jmp sysenter_do_call
--
1.9.3
On 10/22/2014 09:04 PM, Eric Paris wrote:
> git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
> It was writing over %esp/pt_regs semi-randomly on i686 with the expected
> "system can't boot" results. As noted in:
>
> https://bugs.freedesktop.org/show_bug.cgi?id=85277
>
> This patch stops fscking with pt_regs. Instead it sets up the registers
> for the call to __audit_syscall_entry in the most obvious conceivable
> way. It then does just a tiny tiny touch of magic. We need to get what
> started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
> as a pair of pushes.
>
> After the call to __audit_syscall_entry all we need to do is get that
> now useless junk off the stack (pair of pops) and reload %eax with the
> original syscall so other stuff can keep going about it's business.
>
> Signed-off-by: Eric Paris <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> arch/x86/kernel/entry_32.S | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> index f9e3fab..fb01d22 100644
> --- a/arch/x86/kernel/entry_32.S
> +++ b/arch/x86/kernel/entry_32.S
> @@ -447,15 +447,14 @@ sysenter_exit:
> sysenter_audit:
> testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> jnz syscall_trace_entry
> - addl $4,%esp
> - CFI_ADJUST_CFA_OFFSET -4
> - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> - /* %eax already in %eax 1st arg: syscall number */
> + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
> + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
> call __audit_syscall_entry
> - pushl_cfi %ebx
> + popl_cfi %ecx /* get that remapped edx off the stack */
> + popl_cfi %ecx /* get that remapped esi off the stack */
> movl PT_EAX(%esp),%eax /* reload syscall number */
> jmp sysenter_do_call
>
>
This looks reasonably likely to be correct, but this code is complicated
and now ever slower.
How hard would it be to just delete it and replace it with a
straightforward two-phase trace invocation a la x86_64?
--Andy
Well, probably not something for stable/urgent...
On October 23, 2014 11:39:48 AM PDT, Andy Lutomirski <[email protected]> wrote:
>On 10/22/2014 09:04 PM, Eric Paris wrote:
>> git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very
>dumb.
>> It was writing over %esp/pt_regs semi-randomly on i686 with the
>expected
>> "system can't boot" results. As noted in:
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=85277
>>
>> This patch stops fscking with pt_regs. Instead it sets up the
>registers
>> for the call to __audit_syscall_entry in the most obvious conceivable
>> way. It then does just a tiny tiny touch of magic. We need to get
>what
>> started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as
>easy
>> as a pair of pushes.
>>
>> After the call to __audit_syscall_entry all we need to do is get that
>> now useless junk off the stack (pair of pops) and reload %eax with
>the
>> original syscall so other stuff can keep going about it's business.
>>
>> Signed-off-by: Eric Paris <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: "H. Peter Anvin" <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> arch/x86/kernel/entry_32.S | 15 +++++++--------
>> 1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
>> index f9e3fab..fb01d22 100644
>> --- a/arch/x86/kernel/entry_32.S
>> +++ b/arch/x86/kernel/entry_32.S
>> @@ -447,15 +447,14 @@ sysenter_exit:
>> sysenter_audit:
>> testl $(_TIF_WORK_SYSCALL_ENTRY &
>~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
>> jnz syscall_trace_entry
>> - addl $4,%esp
>> - CFI_ADJUST_CFA_OFFSET -4
>> - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
>> - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
>> - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
>> - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
>> - /* %eax already in %eax 1st arg: syscall number */
>> + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to
>audit */
>> + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
>> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
>> + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
>> + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
>> call __audit_syscall_entry
>> - pushl_cfi %ebx
>> + popl_cfi %ecx /* get that remapped edx off the stack */
>> + popl_cfi %ecx /* get that remapped esi off the stack */
>> movl PT_EAX(%esp),%eax /* reload syscall number */
>> jmp sysenter_do_call
>>
>>
>
>This looks reasonably likely to be correct, but this code is
>complicated
>and now ever slower.
>
>How hard would it be to just delete it and replace it with a
>straightforward two-phase trace invocation a la x86_64?
>
>--Andy
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote:
> On 10/22/2014 09:04 PM, Eric Paris wrote:
> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected
> > "system can't boot" results. As noted in:
> >
> > https://bugs.freedesktop.org/show_bug.cgi?id=85277
> >
> > This patch stops fscking with pt_regs. Instead it sets up the registers
> > for the call to __audit_syscall_entry in the most obvious conceivable
> > way. It then does just a tiny tiny touch of magic. We need to get what
> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
> > as a pair of pushes.
> >
> > After the call to __audit_syscall_entry all we need to do is get that
> > now useless junk off the stack (pair of pops) and reload %eax with the
> > original syscall so other stuff can keep going about it's business.
> >
> > Signed-off-by: Eric Paris <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: "H. Peter Anvin" <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > arch/x86/kernel/entry_32.S | 15 +++++++--------
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> > index f9e3fab..fb01d22 100644
> > --- a/arch/x86/kernel/entry_32.S
> > +++ b/arch/x86/kernel/entry_32.S
> > @@ -447,15 +447,14 @@ sysenter_exit:
> > sysenter_audit:
> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> > jnz syscall_trace_entry
> > - addl $4,%esp
> > - CFI_ADJUST_CFA_OFFSET -4
> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> > - /* %eax already in %eax 1st arg: syscall number */
> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
> > call __audit_syscall_entry
> > - pushl_cfi %ebx
> > + popl_cfi %ecx /* get that remapped edx off the stack */
> > + popl_cfi %ecx /* get that remapped esi off the stack */
> > movl PT_EAX(%esp),%eax /* reload syscall number */
> > jmp sysenter_do_call
> >
> >
>
> This looks reasonably likely to be correct, but this code is complicated
> and now ever slower.
I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by
hand. But I figured this was reasonable enough...
> How hard would it be to just delete it and replace it with a
> straightforward two-phase trace invocation a la x86_64?
For me? Hard.
On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <[email protected]> wrote:
> On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote:
>> On 10/22/2014 09:04 PM, Eric Paris wrote:
>> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
>> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected
>> > "system can't boot" results. As noted in:
>> >
>> > https://bugs.freedesktop.org/show_bug.cgi?id=85277
>> >
>> > This patch stops fscking with pt_regs. Instead it sets up the registers
>> > for the call to __audit_syscall_entry in the most obvious conceivable
>> > way. It then does just a tiny tiny touch of magic. We need to get what
>> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
>> > as a pair of pushes.
>> >
>> > After the call to __audit_syscall_entry all we need to do is get that
>> > now useless junk off the stack (pair of pops) and reload %eax with the
>> > original syscall so other stuff can keep going about it's business.
>> >
>> > Signed-off-by: Eric Paris <[email protected]>
>> > Cc: Thomas Gleixner <[email protected]>
>> > Cc: Ingo Molnar <[email protected]>
>> > Cc: "H. Peter Anvin" <[email protected]>
>> > Cc: [email protected]
>> > Cc: [email protected]
>> > Cc: [email protected]
>> > ---
>> > arch/x86/kernel/entry_32.S | 15 +++++++--------
>> > 1 file changed, 7 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
>> > index f9e3fab..fb01d22 100644
>> > --- a/arch/x86/kernel/entry_32.S
>> > +++ b/arch/x86/kernel/entry_32.S
>> > @@ -447,15 +447,14 @@ sysenter_exit:
>> > sysenter_audit:
>> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
>> > jnz syscall_trace_entry
>> > - addl $4,%esp
>> > - CFI_ADJUST_CFA_OFFSET -4
>> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
>> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
>> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
>> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
>> > - /* %eax already in %eax 1st arg: syscall number */
>> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
>> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
>> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
>> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
>> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
>> > call __audit_syscall_entry
>> > - pushl_cfi %ebx
>> > + popl_cfi %ecx /* get that remapped edx off the stack */
>> > + popl_cfi %ecx /* get that remapped esi off the stack */
>> > movl PT_EAX(%esp),%eax /* reload syscall number */
>> > jmp sysenter_do_call
>> >
>> >
>>
>> This looks reasonably likely to be correct, but this code is complicated
>> and now ever slower.
>
> I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by
> hand. But I figured this was reasonable enough...
>
I'm not complaining about your new assembly in particular. There's
just too much assembly in there in general.
But I feel like I'm missing something in the new code. Aren't you
corrupting ecx with those popl_cfi insns?
>> How hard would it be to just delete it and replace it with a
>> straightforward two-phase trace invocation a la x86_64?
>
> For me? Hard.
>
I can try it eventually, but my track record with the 32-bit entry
code isn't so good. I agree that this would be pretty nasty for
urgent, but at least neither that or your fix would need to go to
-stable.
--Andy
On Thu, 2014-10-23 at 15:30 -0400, Eric Paris wrote:
> On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote:
> > On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <[email protected]> wrote:
> > > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote:
> > >> On 10/22/2014 09:04 PM, Eric Paris wrote:
> > >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
> > >> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected
> > >> > "system can't boot" results. As noted in:
> > >> >
> > >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277
> > >> >
> > >> > This patch stops fscking with pt_regs. Instead it sets up the registers
> > >> > for the call to __audit_syscall_entry in the most obvious conceivable
> > >> > way. It then does just a tiny tiny touch of magic. We need to get what
> > >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
> > >> > as a pair of pushes.
> > >> >
> > >> > After the call to __audit_syscall_entry all we need to do is get that
> > >> > now useless junk off the stack (pair of pops) and reload %eax with the
> > >> > original syscall so other stuff can keep going about it's business.
> > >> >
> > >> > Signed-off-by: Eric Paris <[email protected]>
> > >> > Cc: Thomas Gleixner <[email protected]>
> > >> > Cc: Ingo Molnar <[email protected]>
> > >> > Cc: "H. Peter Anvin" <[email protected]>
> > >> > Cc: [email protected]
> > >> > Cc: [email protected]
> > >> > Cc: [email protected]
> > >> > ---
> > >> > arch/x86/kernel/entry_32.S | 15 +++++++--------
> > >> > 1 file changed, 7 insertions(+), 8 deletions(-)
> > >> >
> > >> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> > >> > index f9e3fab..fb01d22 100644
> > >> > --- a/arch/x86/kernel/entry_32.S
> > >> > +++ b/arch/x86/kernel/entry_32.S
> > >> > @@ -447,15 +447,14 @@ sysenter_exit:
> > >> > sysenter_audit:
> > >> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> > >> > jnz syscall_trace_entry
> > >> > - addl $4,%esp
> > >> > - CFI_ADJUST_CFA_OFFSET -4
> > >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> > >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> > >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> > >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> > >> > - /* %eax already in %eax 1st arg: syscall number */
> > >> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> > >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
> > >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> > >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
> > >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
> > >> > call __audit_syscall_entry
> > >> > - pushl_cfi %ebx
> > >> > + popl_cfi %ecx /* get that remapped edx off the stack */
> > >> > + popl_cfi %ecx /* get that remapped esi off the stack */
> > >> > movl PT_EAX(%esp),%eax /* reload syscall number */
> > >> > jmp sysenter_do_call
> > >> >
> > >> >
> > >>
> > >> This looks reasonably likely to be correct, but this code is complicated
> > >> and now ever slower.
> > >
> > > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by
> > > hand. But I figured this was reasonable enough...
> > >
> >
> > I'm not complaining about your new assembly in particular. There's
> > just too much assembly in there in general.
> >
> > But I feel like I'm missing something in the new code. Aren't you
> > corrupting ecx with those popl_cfi insns?
>
> After the call __audit_syscall_entry aren't they already polluted?
> Isn't that the reason we need to reload EAX?
Well, I guess EAX is special...
> You can verify this leaves
> things in a similar state (although slightly differently polluted) than
> before it got screwed up. Here is diff between before the breakage and
> what I propose we do now.
>
> (I admit I don't understand how the pushl_cfi %ebx wasn't messing up
> PT_EBX)
>
> /me anxiously awaits x86 guy to tell me how dumb I am....
>
> $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f -- arch/x86/kernel/entry_32.S
> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> index 0d0c9d4..fb01d22 100644
> --- a/arch/x86/kernel/entry_32.S
> +++ b/arch/x86/kernel/entry_32.S
> @@ -447,16 +447,14 @@ sysenter_exit:
> sysenter_audit:
> testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> jnz syscall_trace_entry
> - addl $4,%esp
> - CFI_ADJUST_CFA_OFFSET -4
> - /* %esi already in 8(%esp) 6th arg: 4th syscall arg */
> - /* %edx already in 4(%esp) 5th arg: 3rd syscall arg */
> - /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg */
> - movl %ebx,%ecx /* 3rd arg: 1st syscall arg */
> - movl %eax,%edx /* 2nd arg: syscall number */
> - movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */
> + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
> + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
> call __audit_syscall_entry
> - pushl_cfi %ebx
> + popl_cfi %ecx /* get that remapped edx off the stack */
> + popl_cfi %ecx /* get that remapped esi off the stack */
> movl PT_EAX(%esp),%eax /* reload syscall number */
> jmp sysenter_do_call
>
On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote:
> On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <[email protected]> wrote:
> > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote:
> >> On 10/22/2014 09:04 PM, Eric Paris wrote:
> >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
> >> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected
> >> > "system can't boot" results. As noted in:
> >> >
> >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277
> >> >
> >> > This patch stops fscking with pt_regs. Instead it sets up the registers
> >> > for the call to __audit_syscall_entry in the most obvious conceivable
> >> > way. It then does just a tiny tiny touch of magic. We need to get what
> >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
> >> > as a pair of pushes.
> >> >
> >> > After the call to __audit_syscall_entry all we need to do is get that
> >> > now useless junk off the stack (pair of pops) and reload %eax with the
> >> > original syscall so other stuff can keep going about it's business.
> >> >
> >> > Signed-off-by: Eric Paris <[email protected]>
> >> > Cc: Thomas Gleixner <[email protected]>
> >> > Cc: Ingo Molnar <[email protected]>
> >> > Cc: "H. Peter Anvin" <[email protected]>
> >> > Cc: [email protected]
> >> > Cc: [email protected]
> >> > Cc: [email protected]
> >> > ---
> >> > arch/x86/kernel/entry_32.S | 15 +++++++--------
> >> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> >> > index f9e3fab..fb01d22 100644
> >> > --- a/arch/x86/kernel/entry_32.S
> >> > +++ b/arch/x86/kernel/entry_32.S
> >> > @@ -447,15 +447,14 @@ sysenter_exit:
> >> > sysenter_audit:
> >> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> >> > jnz syscall_trace_entry
> >> > - addl $4,%esp
> >> > - CFI_ADJUST_CFA_OFFSET -4
> >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> >> > - /* %eax already in %eax 1st arg: syscall number */
> >> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
> >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
> >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
> >> > call __audit_syscall_entry
> >> > - pushl_cfi %ebx
> >> > + popl_cfi %ecx /* get that remapped edx off the stack */
> >> > + popl_cfi %ecx /* get that remapped esi off the stack */
> >> > movl PT_EAX(%esp),%eax /* reload syscall number */
> >> > jmp sysenter_do_call
> >> >
> >> >
> >>
> >> This looks reasonably likely to be correct, but this code is complicated
> >> and now ever slower.
> >
> > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by
> > hand. But I figured this was reasonable enough...
> >
>
> I'm not complaining about your new assembly in particular. There's
> just too much assembly in there in general.
>
> But I feel like I'm missing something in the new code. Aren't you
> corrupting ecx with those popl_cfi insns?
After the call __audit_syscall_entry aren't they already polluted?
Isn't that the reason we need to reload EAX? You can verify this leaves
things in a similar state (although slightly differently polluted) than
before it got screwed up. Here is diff between before the breakage and
what I propose we do now.
(I admit I don't understand how the pushl_cfi %ebx wasn't messing up
PT_EBX)
/me anxiously awaits x86 guy to tell me how dumb I am....
$ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f -- arch/x86/kernel/entry_32.S
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 0d0c9d4..fb01d22 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -447,16 +447,14 @@ sysenter_exit:
sysenter_audit:
testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
jnz syscall_trace_entry
- addl $4,%esp
- CFI_ADJUST_CFA_OFFSET -4
- /* %esi already in 8(%esp) 6th arg: 4th syscall arg */
- /* %edx already in 4(%esp) 5th arg: 3rd syscall arg */
- /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg */
- movl %ebx,%ecx /* 3rd arg: 1st syscall arg */
- movl %eax,%edx /* 2nd arg: syscall number */
- movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */
+ /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
+ movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
+ /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
+ pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
+ pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
call __audit_syscall_entry
- pushl_cfi %ebx
+ popl_cfi %ecx /* get that remapped edx off the stack */
+ popl_cfi %ecx /* get that remapped esi off the stack */
movl PT_EAX(%esp),%eax /* reload syscall number */
jmp sysenter_do_call
On Thu, Oct 23, 2014 at 12:30 PM, Eric Paris <[email protected]> wrote:
> On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote:
>> On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <[email protected]> wrote:
>> > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote:
>> >> On 10/22/2014 09:04 PM, Eric Paris wrote:
>> >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
>> >> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected
>> >> > "system can't boot" results. As noted in:
>> >> >
>> >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277
>> >> >
>> >> > This patch stops fscking with pt_regs. Instead it sets up the registers
>> >> > for the call to __audit_syscall_entry in the most obvious conceivable
>> >> > way. It then does just a tiny tiny touch of magic. We need to get what
>> >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
>> >> > as a pair of pushes.
>> >> >
>> >> > After the call to __audit_syscall_entry all we need to do is get that
>> >> > now useless junk off the stack (pair of pops) and reload %eax with the
>> >> > original syscall so other stuff can keep going about it's business.
>> >> >
>> >> > Signed-off-by: Eric Paris <[email protected]>
>> >> > Cc: Thomas Gleixner <[email protected]>
>> >> > Cc: Ingo Molnar <[email protected]>
>> >> > Cc: "H. Peter Anvin" <[email protected]>
>> >> > Cc: [email protected]
>> >> > Cc: [email protected]
>> >> > Cc: [email protected]
>> >> > ---
>> >> > arch/x86/kernel/entry_32.S | 15 +++++++--------
>> >> > 1 file changed, 7 insertions(+), 8 deletions(-)
>> >> >
>> >> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
>> >> > index f9e3fab..fb01d22 100644
>> >> > --- a/arch/x86/kernel/entry_32.S
>> >> > +++ b/arch/x86/kernel/entry_32.S
>> >> > @@ -447,15 +447,14 @@ sysenter_exit:
>> >> > sysenter_audit:
>> >> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
>> >> > jnz syscall_trace_entry
>> >> > - addl $4,%esp
>> >> > - CFI_ADJUST_CFA_OFFSET -4
>> >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
>> >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
>> >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
>> >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
>> >> > - /* %eax already in %eax 1st arg: syscall number */
>> >> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
>> >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
>> >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
>> >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
>> >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
>> >> > call __audit_syscall_entry
>> >> > - pushl_cfi %ebx
>> >> > + popl_cfi %ecx /* get that remapped edx off the stack */
>> >> > + popl_cfi %ecx /* get that remapped esi off the stack */
>> >> > movl PT_EAX(%esp),%eax /* reload syscall number */
>> >> > jmp sysenter_do_call
>> >> >
>> >> >
>> >>
>> >> This looks reasonably likely to be correct, but this code is complicated
>> >> and now ever slower.
>> >
>> > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by
>> > hand. But I figured this was reasonable enough...
>> >
>>
>> I'm not complaining about your new assembly in particular. There's
>> just too much assembly in there in general.
>>
>> But I feel like I'm missing something in the new code. Aren't you
>> corrupting ecx with those popl_cfi insns?
>
> After the call __audit_syscall_entry aren't they already polluted?
> Isn't that the reason we need to reload EAX? You can verify this leaves
> things in a similar state (although slightly differently polluted) than
> before it got screwed up. Here is diff between before the breakage and
> what I propose we do now.
>
> (I admit I don't understand how the pushl_cfi %ebx wasn't messing up
> PT_EBX)
>
> /me anxiously awaits x86 guy to tell me how dumb I am....
>
hpa, do you have time to figure this out? I don't know the 32-bit ABI
well enough, nor will I have time to disassemble things or find and
read the spec to figure this out in the next few days.
--Andy
> $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f -- arch/x86/kernel/entry_32.S
> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> index 0d0c9d4..fb01d22 100644
> --- a/arch/x86/kernel/entry_32.S
> +++ b/arch/x86/kernel/entry_32.S
> @@ -447,16 +447,14 @@ sysenter_exit:
> sysenter_audit:
> testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> jnz syscall_trace_entry
> - addl $4,%esp
> - CFI_ADJUST_CFA_OFFSET -4
> - /* %esi already in 8(%esp) 6th arg: 4th syscall arg */
> - /* %edx already in 4(%esp) 5th arg: 3rd syscall arg */
> - /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg */
> - movl %ebx,%ecx /* 3rd arg: 1st syscall arg */
> - movl %eax,%edx /* 2nd arg: syscall number */
> - movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */
> + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
> + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
> call __audit_syscall_entry
> - pushl_cfi %ebx
> + popl_cfi %ecx /* get that remapped edx off the stack */
> + popl_cfi %ecx /* get that remapped esi off the stack */
> movl PT_EAX(%esp),%eax /* reload syscall number */
> jmp sysenter_do_call
>
>
--
Andy Lutomirski
AMA Capital Management, LLC
Yes, I will look at this tomorrow.
For the record, the calling convention is that eax, edx, ecx are clobbered, and used for the three first arguments in that order. eax, edx are used for the return value(s).
The exception is for __asmlinkage functions where all arguments are passed on the stack; something I would like to get rid of but would require changing a lot of assembly code.
The same registers are still clobbered, though.
On October 23, 2014 1:30:40 PM PDT, Andy Lutomirski <[email protected]> wrote:
>On Thu, Oct 23, 2014 at 12:30 PM, Eric Paris <[email protected]> wrote:
>> On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote:
>>> On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <[email protected]>
>wrote:
>>> > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote:
>>> >> On 10/22/2014 09:04 PM, Eric Paris wrote:
>>> >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very
>very dumb.
>>> >> > It was writing over %esp/pt_regs semi-randomly on i686 with
>the expected
>>> >> > "system can't boot" results. As noted in:
>>> >> >
>>> >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277
>>> >> >
>>> >> > This patch stops fscking with pt_regs. Instead it sets up the
>registers
>>> >> > for the call to __audit_syscall_entry in the most obvious
>conceivable
>>> >> > way. It then does just a tiny tiny touch of magic. We need to
>get what
>>> >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This
>is as easy
>>> >> > as a pair of pushes.
>>> >> >
>>> >> > After the call to __audit_syscall_entry all we need to do is
>get that
>>> >> > now useless junk off the stack (pair of pops) and reload %eax
>with the
>>> >> > original syscall so other stuff can keep going about it's
>business.
>>> >> >
>>> >> > Signed-off-by: Eric Paris <[email protected]>
>>> >> > Cc: Thomas Gleixner <[email protected]>
>>> >> > Cc: Ingo Molnar <[email protected]>
>>> >> > Cc: "H. Peter Anvin" <[email protected]>
>>> >> > Cc: [email protected]
>>> >> > Cc: [email protected]
>>> >> > Cc: [email protected]
>>> >> > ---
>>> >> > arch/x86/kernel/entry_32.S | 15 +++++++--------
>>> >> > 1 file changed, 7 insertions(+), 8 deletions(-)
>>> >> >
>>> >> > diff --git a/arch/x86/kernel/entry_32.S
>b/arch/x86/kernel/entry_32.S
>>> >> > index f9e3fab..fb01d22 100644
>>> >> > --- a/arch/x86/kernel/entry_32.S
>>> >> > +++ b/arch/x86/kernel/entry_32.S
>>> >> > @@ -447,15 +447,14 @@ sysenter_exit:
>>> >> > sysenter_audit:
>>> >> > testl $(_TIF_WORK_SYSCALL_ENTRY &
>~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
>>> >> > jnz syscall_trace_entry
>>> >> > - addl $4,%esp
>>> >> > - CFI_ADJUST_CFA_OFFSET -4
>>> >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg
>*/
>>> >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg
>*/
>>> >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg
>*/
>>> >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg
>*/
>>> >> > - /* %eax already in %eax 1st arg: syscall number
>*/
>>> >> > + /* movl PT_EAX(%esp), %eax already set, syscall
>number: 1st arg to audit */
>>> >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit
>*/
>>> >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to
>audit */
>>> >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
>>> >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
>>> >> > call __audit_syscall_entry
>>> >> > - pushl_cfi %ebx
>>> >> > + popl_cfi %ecx /* get that remapped edx off the stack */
>>> >> > + popl_cfi %ecx /* get that remapped esi off the stack */
>>> >> > movl PT_EAX(%esp),%eax /* reload syscall number */
>>> >> > jmp sysenter_do_call
>>> >> >
>>> >> >
>>> >>
>>> >> This looks reasonably likely to be correct, but this code is
>complicated
>>> >> and now ever slower.
>>> >
>>> > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET
>by
>>> > hand. But I figured this was reasonable enough...
>>> >
>>>
>>> I'm not complaining about your new assembly in particular. There's
>>> just too much assembly in there in general.
>>>
>>> But I feel like I'm missing something in the new code. Aren't you
>>> corrupting ecx with those popl_cfi insns?
>>
>> After the call __audit_syscall_entry aren't they already polluted?
>> Isn't that the reason we need to reload EAX? You can verify this
>leaves
>> things in a similar state (although slightly differently polluted)
>than
>> before it got screwed up. Here is diff between before the breakage
>and
>> what I propose we do now.
>>
>> (I admit I don't understand how the pushl_cfi %ebx wasn't messing up
>> PT_EBX)
>>
>> /me anxiously awaits x86 guy to tell me how dumb I am....
>>
>
>hpa, do you have time to figure this out? I don't know the 32-bit ABI
>well enough, nor will I have time to disassemble things or find and
>read the spec to figure this out in the next few days.
>
>--Andy
>
>> $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f --
>arch/x86/kernel/entry_32.S
>> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
>> index 0d0c9d4..fb01d22 100644
>> --- a/arch/x86/kernel/entry_32.S
>> +++ b/arch/x86/kernel/entry_32.S
>> @@ -447,16 +447,14 @@ sysenter_exit:
>> sysenter_audit:
>> testl $(_TIF_WORK_SYSCALL_ENTRY &
>~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
>> jnz syscall_trace_entry
>> - addl $4,%esp
>> - CFI_ADJUST_CFA_OFFSET -4
>> - /* %esi already in 8(%esp) 6th arg: 4th syscall arg
>*/
>> - /* %edx already in 4(%esp) 5th arg: 3rd syscall arg
>*/
>> - /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg
>*/
>> - movl %ebx,%ecx /* 3rd arg: 1st syscall arg
>*/
>> - movl %eax,%edx /* 2nd arg: syscall number */
>> - movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */
>> + /* movl PT_EAX(%esp), %eax already set, syscall number:
>1st arg to audit */
>> + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit
>*/
>> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to
>audit */
>> + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
>> + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
>> call __audit_syscall_entry
>> - pushl_cfi %ebx
>> + popl_cfi %ecx /* get that remapped edx off the stack */
>> + popl_cfi %ecx /* get that remapped esi off the stack */
>> movl PT_EAX(%esp),%eax /* reload syscall number */
>> jmp sysenter_do_call
>>
>>
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On 10/23/2014 12:38 PM, Eric Paris wrote:
>>
>> After the call __audit_syscall_entry aren't they already polluted?
>> Isn't that the reason we need to reload EAX?
>
> Well, I guess EAX is special...
>
Because system calls are "asmlinkage", all the parameters are on the
stack, but %eax is used as the index into the system call table. This
should thus be fine until we get rid of regparm(0) entirely, if that
ever happens.
-hpa
Commit-ID: 26c2d2b39128adba276d140eefa2745591b88536
Gitweb: http://git.kernel.org/tip/26c2d2b39128adba276d140eefa2745591b88536
Author: Eric Paris <[email protected]>
AuthorDate: Thu, 23 Oct 2014 00:04:03 -0400
Committer: H. Peter Anvin <[email protected]>
CommitDate: Fri, 24 Oct 2014 13:27:56 -0700
i386/audit: stop scribbling on the stack frame
git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
It was writing over %esp/pt_regs semi-randomly on i686 with the expected
"system can't boot" results. As noted in:
https://bugs.freedesktop.org/show_bug.cgi?id=85277
This patch stops fscking with pt_regs. Instead it sets up the registers
for the call to __audit_syscall_entry in the most obvious conceivable
way. It then does just a tiny tiny touch of magic. We need to get what
started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
as a pair of pushes.
After the call to __audit_syscall_entry all we need to do is get that
now useless junk off the stack (pair of pops) and reload %eax with the
original syscall so other stuff can keep going about it's business.
Reported-by: Paulo Zanoni <[email protected]>
Signed-off-by: Eric Paris <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Cc: Richard Guy Briggs <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/kernel/entry_32.S | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index b553ed8..344b63f 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -447,15 +447,14 @@ sysenter_exit:
sysenter_audit:
testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
jnz syscall_trace_entry
- addl $4,%esp
- CFI_ADJUST_CFA_OFFSET -4
- movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
- movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
- /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
- movl %ebx,%edx /* 2nd arg: 1st syscall arg */
- /* %eax already in %eax 1st arg: syscall number */
+ /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
+ movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
+ /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
+ pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
+ pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
call __audit_syscall_entry
- pushl_cfi %ebx
+ popl_cfi %ecx /* get that remapped edx off the stack */
+ popl_cfi %ecx /* get that remapped esi off the stack */
movl PT_EAX(%esp),%eax /* reload syscall number */
jmp sysenter_do_call
On Fri, Oct 24, 2014 at 1:19 PM, H. Peter Anvin <[email protected]> wrote:
> On 10/23/2014 12:38 PM, Eric Paris wrote:
>>>
>>> After the call __audit_syscall_entry aren't they already polluted?
>>> Isn't that the reason we need to reload EAX?
>>
>> Well, I guess EAX is special...
>>
>
> Because system calls are "asmlinkage", all the parameters are on the
> stack, but %eax is used as the index into the system call table. This
> should thus be fine until we get rid of regparm(0) entirely, if that
> ever happens.
>
...and because __audit_syscall_entry *isn't* asmlinkage, it uses the
other convention, which is where the confusion comes from. And, by
the time you get to sysenter_do_call, nothing cares about ecx, so you
can freely clobber it while popping from the stack. I get it now.
--Andy
> -hpa
>
>
--
Andy Lutomirski
AMA Capital Management, LLC
On Fri, 24 Oct 2014, tip-bot for Eric Paris wrote:
> sysenter_audit:
> testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> jnz syscall_trace_entry
> - addl $4,%esp
> - CFI_ADJUST_CFA_OFFSET -4
> - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> - /* %eax already in %eax 1st arg: syscall number */
> + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
> + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
Why are we grabbing that from the stack? AFAICT all arguments are in
the registers still.
Thanks,
tglx
On 14/10/23, Eric Paris wrote:
> On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote:
> > On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <[email protected]> wrote:
> > > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote:
> > >> On 10/22/2014 09:04 PM, Eric Paris wrote:
> > >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
> > >> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected
> > >> > "system can't boot" results. As noted in:
> > >> >
> > >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277
> > >> >
> > >> > This patch stops fscking with pt_regs. Instead it sets up the registers
> > >> > for the call to __audit_syscall_entry in the most obvious conceivable
> > >> > way. It then does just a tiny tiny touch of magic. We need to get what
> > >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
> > >> > as a pair of pushes.
> > >> >
> > >> > After the call to __audit_syscall_entry all we need to do is get that
> > >> > now useless junk off the stack (pair of pops) and reload %eax with the
> > >> > original syscall so other stuff can keep going about it's business.
> > >> >
> > >> > Signed-off-by: Eric Paris <[email protected]>
> > >> > Cc: Thomas Gleixner <[email protected]>
> > >> > Cc: Ingo Molnar <[email protected]>
> > >> > Cc: "H. Peter Anvin" <[email protected]>
> > >> > Cc: [email protected]
> > >> > Cc: [email protected]
> > >> > Cc: [email protected]
> > >> > ---
> > >> > arch/x86/kernel/entry_32.S | 15 +++++++--------
> > >> > 1 file changed, 7 insertions(+), 8 deletions(-)
> > >> >
> > >> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> > >> > index f9e3fab..fb01d22 100644
> > >> > --- a/arch/x86/kernel/entry_32.S
> > >> > +++ b/arch/x86/kernel/entry_32.S
> > >> > @@ -447,15 +447,14 @@ sysenter_exit:
> > >> > sysenter_audit:
> > >> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> > >> > jnz syscall_trace_entry
> > >> > - addl $4,%esp
> > >> > - CFI_ADJUST_CFA_OFFSET -4
> > >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> > >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> > >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> > >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> > >> > - /* %eax already in %eax 1st arg: syscall number */
> > >> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> > >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
> > >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> > >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
> > >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
> > >> > call __audit_syscall_entry
> > >> > - pushl_cfi %ebx
> > >> > + popl_cfi %ecx /* get that remapped edx off the stack */
> > >> > + popl_cfi %ecx /* get that remapped esi off the stack */
> > >> > movl PT_EAX(%esp),%eax /* reload syscall number */
> > >> > jmp sysenter_do_call
> > >> >
> > >> >
> > >>
> > >> This looks reasonably likely to be correct, but this code is complicated
> > >> and now ever slower.
> > >
> > > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by
> > > hand. But I figured this was reasonable enough...
> > >
> >
> > I'm not complaining about your new assembly in particular. There's
> > just too much assembly in there in general.
> >
> > But I feel like I'm missing something in the new code. Aren't you
> > corrupting ecx with those popl_cfi insns?
>
> After the call __audit_syscall_entry aren't they already polluted?
> Isn't that the reason we need to reload EAX? You can verify this leaves
> things in a similar state (although slightly differently polluted) than
> before it got screwed up. Here is diff between before the breakage and
> what I propose we do now.
>
> (I admit I don't understand how the pushl_cfi %ebx wasn't messing up
> PT_EBX)
(Credit to HPA for walking me through some of this... I had to stare at
it for a while...)
The bottom of the stack was dropped to reuse syscall args a1-a3 on the
stack for the call to __audit_syscall_entry while %ebx wasn't changed in
the call. a0 was in the unchanged %ebx and the pushl_cfi directly
restores the value in PT_EBX when it puts a0 back on the stack and
restores the original value of %esp.
> /me anxiously awaits x86 guy to tell me how dumb I am....
("Dumber" tries to explain...)
> $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f -- arch/x86/kernel/entry_32.S
> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> index 0d0c9d4..fb01d22 100644
> --- a/arch/x86/kernel/entry_32.S
> +++ b/arch/x86/kernel/entry_32.S
> @@ -447,16 +447,14 @@ sysenter_exit:
> sysenter_audit:
> testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> jnz syscall_trace_entry
> - addl $4,%esp
> - CFI_ADJUST_CFA_OFFSET -4
> - /* %esi already in 8(%esp) 6th arg: 4th syscall arg */
> - /* %edx already in 4(%esp) 5th arg: 3rd syscall arg */
> - /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg */
> - movl %ebx,%ecx /* 3rd arg: 1st syscall arg */
> - movl %eax,%edx /* 2nd arg: syscall number */
> - movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */
> + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
> + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
> call __audit_syscall_entry
> - pushl_cfi %ebx
> + popl_cfi %ecx /* get that remapped edx off the stack */
> + popl_cfi %ecx /* get that remapped esi off the stack */
> movl PT_EAX(%esp),%eax /* reload syscall number */
> jmp sysenter_do_call
>
>
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
On 14/10/24, Andy Lutomirski wrote:
> On Fri, Oct 24, 2014 at 1:19 PM, H. Peter Anvin <[email protected]> wrote:
> > On 10/23/2014 12:38 PM, Eric Paris wrote:
> >>> After the call __audit_syscall_entry aren't they already polluted?
> >>> Isn't that the reason we need to reload EAX?
> >>
> >> Well, I guess EAX is special...
> >
> > Because system calls are "asmlinkage", all the parameters are on the
> > stack, but %eax is used as the index into the system call table. This
> > should thus be fine until we get rid of regparm(0) entirely, if that
> > ever happens.
>
> ...and because __audit_syscall_entry *isn't* asmlinkage, it uses the
> other convention, which is where the confusion comes from. And, by
> the time you get to sysenter_do_call, nothing cares about ecx, so you
> can freely clobber it while popping from the stack. I get it now.
So you could just as easily clobber %eax since that'll be restored from
PT_EAX(%esp) anyways in the following step...
Or instead of popping these two values, could ajust the stack with
addl $8,%esp
CFI_ADJUST_CFA_OFFSET -8
since we don't need either value popped off the stack?
> --Andy
>
> > -hpa
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
It was writing over %esp/pt_regs semi-randomly on i686 with the expected
"system can't boot" results. As noted in:
https://bugs.freedesktop.org/show_bug.cgi?id=85277
This patch stops fscking with pt_regs. Instead it sets up the registers
for the call to __audit_syscall_entry in the most obvious conceivable
way. It then does just a tiny tiny touch of magic. We need to get what
started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
as a pair of pushes using the values still in those registers.
After the call to __audit_syscall_entry all we need to do is get that
now useless junk off the stack (pair of pops) and reload %eax with the
original syscall so other stuff can keep going about it's business.
Reported-by: Paulo Zanoni <[email protected]>
Signed-off-by: Eric Paris <[email protected]>
Signed-off-by: Richard Guy Briggs <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
On 14/10/25, Thomas Gleixner wrote:
> Why are we grabbing that from the stack? AFAICT all arguments are in
> the registers still.
Right, re-arranging the instructions slightly to avoid overwriting %edx
with %ebx before needing it to push onto the stack, how does this look?
arch/x86/kernel/entry_32.S | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index b553ed8..344b63f 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -447,15 +447,14 @@ sysenter_exit:
sysenter_audit:
testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
jnz syscall_trace_entry
- addl $4,%esp
- CFI_ADJUST_CFA_OFFSET -4
- movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
- movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
- /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
- movl %ebx,%edx /* 2nd arg: 1st syscall arg */
- /* %eax already in %eax 1st arg: syscall number */
+ /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
+ /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
+ pushl_cfi %esi /* a3: 5th arg */
+ pushl_cfi %edx /* a2: 4th arg */
+ movl %ebx, %edx /* ebx/a0: 2nd arg to audit */
call __audit_syscall_entry
- pushl_cfi %ebx
+ popl_cfi %ecx /* get that remapped edx off the stack */
+ popl_cfi %ecx /* get that remapped esi off the stack */
movl PT_EAX(%esp),%eax /* reload syscall number */
jmp sysenter_do_call
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
My patch was already committed to the -tip urgent branch. I believe any
optimization should be based on that branch, Richard. If you are trying
to wrangle every bit of speed out of this, should you
push %esi;
push %edi;
CFI_ADJUST_CFA_OFFSET 8
call __audit_syscall_entry
pop;
pop;
CFI_ADJUST_CFA_OFFSET -8
Instead of using the pushl_cfi and popl_cfi macros?
I wrote my patch to be obviously correct, but agree there are certainly
some speedups possible.
-Eric
On Sun, 2014-10-26 at 22:34 -0400, Richard Guy Briggs wrote:
> git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
> It was writing over %esp/pt_regs semi-randomly on i686 with the expected
> "system can't boot" results. As noted in:
>
> https://bugs.freedesktop.org/show_bug.cgi?id=85277
>
> This patch stops fscking with pt_regs. Instead it sets up the registers
> for the call to __audit_syscall_entry in the most obvious conceivable
> way. It then does just a tiny tiny touch of magic. We need to get what
> started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
> as a pair of pushes using the values still in those registers.
>
> After the call to __audit_syscall_entry all we need to do is get that
> now useless junk off the stack (pair of pops) and reload %eax with the
> original syscall so other stuff can keep going about it's business.
>
> Reported-by: Paulo Zanoni <[email protected]>
> Signed-off-by: Eric Paris <[email protected]>
> Signed-off-by: Richard Guy Briggs <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> ---
> On 14/10/25, Thomas Gleixner wrote:
> > Why are we grabbing that from the stack? AFAICT all arguments are in
> > the registers still.
>
> Right, re-arranging the instructions slightly to avoid overwriting %edx
> with %ebx before needing it to push onto the stack, how does this look?
>
> arch/x86/kernel/entry_32.S | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> index b553ed8..344b63f 100644
> --- a/arch/x86/kernel/entry_32.S
> +++ b/arch/x86/kernel/entry_32.S
> @@ -447,15 +447,14 @@ sysenter_exit:
> sysenter_audit:
> testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> jnz syscall_trace_entry
> - addl $4,%esp
> - CFI_ADJUST_CFA_OFFSET -4
> - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> - /* %eax already in %eax 1st arg: syscall number */
> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> + pushl_cfi %esi /* a3: 5th arg */
> + pushl_cfi %edx /* a2: 4th arg */
> + movl %ebx, %edx /* ebx/a0: 2nd arg to audit */
> call __audit_syscall_entry
> - pushl_cfi %ebx
> + popl_cfi %ecx /* get that remapped edx off the stack */
> + popl_cfi %ecx /* get that remapped esi off the stack */
> movl PT_EAX(%esp),%eax /* reload syscall number */
> jmp sysenter_do_call
>
>
> - RGB
>
> --
> Richard Guy Briggs <[email protected]>
> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
On 10/27/2014 06:55 AM, Eric Paris wrote:
> My patch was already committed to the -tip urgent branch. I believe any
> optimization should be based on that branch, Richard. If you are trying
> to wrangle every bit of speed out of this, should you
>
> push %esi;
> push %edi;
> CFI_ADJUST_CFA_OFFSET 8
> call __audit_syscall_entry
> pop;
> pop;
> CFI_ADJUST_CFA_OFFSET -8
>
> Instead of using the pushl_cfi and popl_cfi macros?
>
> I wrote my patch to be obviously correct, but agree there are certainly
> some speedups possible.
>
Uh... not only is that plain wrong (the CFI should be adjusted after
each instruction that changes the stack pointer), but what the heck is
wrong with using the macros?
-hpa
On Mon, 2014-10-27 at 10:02 -0700, H. Peter Anvin wrote:
> On 10/27/2014 06:55 AM, Eric Paris wrote:
> > My patch was already committed to the -tip urgent branch. I believe any
> > optimization should be based on that branch, Richard. If you are trying
> > to wrangle every bit of speed out of this, should you
> >
> > push %esi;
> > push %edi;
> > CFI_ADJUST_CFA_OFFSET 8
> > call __audit_syscall_entry
> > pop;
> > pop;
> > CFI_ADJUST_CFA_OFFSET -8
> >
> > Instead of using the pushl_cfi and popl_cfi macros?
> >
> > I wrote my patch to be obviously correct, but agree there are certainly
> > some speedups possible.
> >
>
> Uh... not only is that plain wrong (the CFI should be adjusted after
> each instruction that changes the stack pointer),
Sure, things would be screwed up between the two push's
> but what the heck is
> wrong with using the macros?
I was asking if that would save an instruction or two by consolidating
the CFI update and if so would that tradeoff be worth it, given the
regularity of this code being run.
>
> -hpa
>
>
>
On 14/10/27, Eric Paris wrote:
> My patch was already committed to the -tip urgent branch. I believe any
> optimization should be based on that branch, Richard. If you are trying
> to wrangle every bit of speed out of this, should you
>
> push %esi;
> push %edi;
> CFI_ADJUST_CFA_OFFSET 8
> call __audit_syscall_entry
> pop;
> pop;
> CFI_ADJUST_CFA_OFFSET -8
>
> Instead of using the pushl_cfi and popl_cfi macros?
>
> I wrote my patch to be obviously correct, but agree there are certainly
> some speedups possible.
I was just more surprised that you didn't use the assumptions and
approach of the original code before I botched it, which simply re-used
values in registers that were already there, rather than fetching them
from the pt_regs struct on the stack and adjusting the reference with an
offset on the fly as you're pushing items onto the stack.
> -Eric
>
> On Sun, 2014-10-26 at 22:34 -0400, Richard Guy Briggs wrote:
> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected
> > "system can't boot" results. As noted in:
> >
> > https://bugs.freedesktop.org/show_bug.cgi?id=85277
> >
> > This patch stops fscking with pt_regs. Instead it sets up the registers
> > for the call to __audit_syscall_entry in the most obvious conceivable
> > way. It then does just a tiny tiny touch of magic. We need to get what
> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
> > as a pair of pushes using the values still in those registers.
> >
> > After the call to __audit_syscall_entry all we need to do is get that
> > now useless junk off the stack (pair of pops) and reload %eax with the
> > original syscall so other stuff can keep going about it's business.
> >
> > Reported-by: Paulo Zanoni <[email protected]>
> > Signed-off-by: Eric Paris <[email protected]>
> > Signed-off-by: Richard Guy Briggs <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: "H. Peter Anvin" <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> >
> > ---
> > On 14/10/25, Thomas Gleixner wrote:
> > > Why are we grabbing that from the stack? AFAICT all arguments are in
> > > the registers still.
> >
> > Right, re-arranging the instructions slightly to avoid overwriting %edx
> > with %ebx before needing it to push onto the stack, how does this look?
> >
> > arch/x86/kernel/entry_32.S | 15 +++++++--------
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> > index b553ed8..344b63f 100644
> > --- a/arch/x86/kernel/entry_32.S
> > +++ b/arch/x86/kernel/entry_32.S
> > @@ -447,15 +447,14 @@ sysenter_exit:
> > sysenter_audit:
> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> > jnz syscall_trace_entry
> > - addl $4,%esp
> > - CFI_ADJUST_CFA_OFFSET -4
> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> > - /* %eax already in %eax 1st arg: syscall number */
> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> > + pushl_cfi %esi /* a3: 5th arg */
> > + pushl_cfi %edx /* a2: 4th arg */
> > + movl %ebx, %edx /* ebx/a0: 2nd arg to audit */
> > call __audit_syscall_entry
> > - pushl_cfi %ebx
> > + popl_cfi %ecx /* get that remapped edx off the stack */
> > + popl_cfi %ecx /* get that remapped esi off the stack */
> > movl PT_EAX(%esp),%eax /* reload syscall number */
> > jmp sysenter_do_call
> >
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <[email protected]>
> > Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
> > Remote, Ottawa, Canada
> > Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
>
>
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
On Sun, 26 Oct 2014, Richard Guy Briggs wrote:
> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> index b553ed8..344b63f 100644
> --- a/arch/x86/kernel/entry_32.S
> +++ b/arch/x86/kernel/entry_32.S
> @@ -447,15 +447,14 @@ sysenter_exit:
> sysenter_audit:
> testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> jnz syscall_trace_entry
> - addl $4,%esp
> - CFI_ADJUST_CFA_OFFSET -4
> - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> - /* %eax already in %eax 1st arg: syscall number */
> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> + pushl_cfi %esi /* a3: 5th arg */
> + pushl_cfi %edx /* a2: 4th arg */
> + movl %ebx, %edx /* ebx/a0: 2nd arg to audit */
> call __audit_syscall_entry
> - pushl_cfi %ebx
> + popl_cfi %ecx /* get that remapped edx off the stack */
> + popl_cfi %ecx /* get that remapped esi off the stack */
Why use pop instead of simply adjusting esp and CFI by 8?
Thanks,
tglx
On Mon, 2014-10-27 at 21:52 +0100, Thomas Gleixner wrote:
> On Sun, 26 Oct 2014, Richard Guy Briggs wrote:
> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> > index b553ed8..344b63f 100644
> > --- a/arch/x86/kernel/entry_32.S
> > +++ b/arch/x86/kernel/entry_32.S
> > @@ -447,15 +447,14 @@ sysenter_exit:
> > sysenter_audit:
> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
> > jnz syscall_trace_entry
> > - addl $4,%esp
> > - CFI_ADJUST_CFA_OFFSET -4
> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> > - /* %eax already in %eax 1st arg: syscall number */
> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
> > + pushl_cfi %esi /* a3: 5th arg */
> > + pushl_cfi %edx /* a2: 4th arg */
> > + movl %ebx, %edx /* ebx/a0: 2nd arg to audit */
> > call __audit_syscall_entry
> > - pushl_cfi %ebx
> > + popl_cfi %ecx /* get that remapped edx off the stack */
> > + popl_cfi %ecx /* get that remapped esi off the stack */
>
> Why use pop instead of simply adjusting esp and CFI by 8?
Certainly seems like a good idea for RGB's perf improvement patch to go
on top of -tip urgent.
-Eric
On Mon, 27 Oct 2014, Eric Paris wrote:
> My patch was already committed to the -tip urgent branch. I believe any
> optimization should be based on that branch, Richard. If you are trying
> to wrangle every bit of speed out of this, should you
>
> push %esi;
> push %edi;
> CFI_ADJUST_CFA_OFFSET 8
Wrong. You want to use pushl_cfi as you need the CFI adjustment after
each modification of esp.
> call __audit_syscall_entry
> pop;
> pop;
> CFI_ADJUST_CFA_OFFSET -8
>
> Instead of using the pushl_cfi and popl_cfi macros?
Wrong again. See above. Aside of that, why do you want to use pop at
all? All we care about is adjusting esp, right?
Thanks,
tglx
On 14/10/27, Thomas Gleixner wrote:
> On Mon, 27 Oct 2014, Eric Paris wrote:
> > My patch was already committed to the -tip urgent branch. I believe any
> > optimization should be based on that branch, Richard. If you are trying
> > to wrangle every bit of speed out of this, should you
> >
> > push %esi;
> > push %edi;
> > CFI_ADJUST_CFA_OFFSET 8
>
> Wrong. You want to use pushl_cfi as you need the CFI adjustment after
> each modification of esp.
>
> > call __audit_syscall_entry
> > pop;
> > pop;
> > CFI_ADJUST_CFA_OFFSET -8
> >
> > Instead of using the pushl_cfi and popl_cfi macros?
>
> Wrong again. See above. Aside of that, why do you want to use pop at
> all? All we care about is adjusting esp, right?
Right. We don't care about those two values on the bottom of the stack.
Just move the extended stack pointer and adjust CFI.
> Thanks,
>
> tglx
- RGB
--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
* Eric Paris <[email protected]> wrote:
> On Mon, 2014-10-27 at 10:02 -0700, H. Peter Anvin wrote:
> > On 10/27/2014 06:55 AM, Eric Paris wrote:
> > > My patch was already committed to the -tip urgent branch. I believe any
> > > optimization should be based on that branch, Richard. If you are trying
> > > to wrangle every bit of speed out of this, should you
> > >
> > > push %esi;
> > > push %edi;
> > > CFI_ADJUST_CFA_OFFSET 8
> > > call __audit_syscall_entry
> > > pop;
> > > pop;
> > > CFI_ADJUST_CFA_OFFSET -8
> > >
> > > Instead of using the pushl_cfi and popl_cfi macros?
> > >
> > > I wrote my patch to be obviously correct, but agree there are certainly
> > > some speedups possible.
> > >
> >
> > Uh... not only is that plain wrong (the CFI should be adjusted after
> > each instruction that changes the stack pointer),
>
> Sure, things would be screwed up between the two push's
>
> > but what the heck is
> > wrong with using the macros?
>
> I was asking if that would save an instruction or two by
> consolidating the CFI update and if so would that tradeoff be
> worth it, given the regularity of this code being run.
CFI updates have no effect on runtime behavior whatsoever (they
don't emit any instructions), they only affect the debug info
being constructed.
Thanks,
Ingo