livepatch: hybrid consistency model

In preparation for being able to determine whether a given stack trace is reliable, allow the stacktrace_ops functions to propagate errors to dump_trace().

Quoted 3 lines · Josh Poimboeuf

In preparation for being able to determine whether a given stack trace is reliable, allow the stacktrace_ops functions to propagate errors to dump_trace().

Hi, Josh.

Have you considered to make walk_stack function as non-return function, since there is no obvious error during detecting the frame points?

A preempted function might not have had a chance to save the frame pointer to the stack yet, which can result in its caller getting skipped on a stack trace.

Add a flag to indicate when the task has been preempted so that stack dump code can determine whether the stack trace is reliable.

Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for x86_64. The bit getting set indicates the thread has a pending patch which needs to be applied when the thread exits the kernel.

The bit is placed in the least-significant word of the thread_info flags

Thanks to all the recent x86 entry code refactoring, most tasks' kernel stacks start at the same offset right above their saved pt_regs, regardless of which syscall was used to enter the kernel. That creates a nice convention which makes it straightforward to identify the "bottom" of the stack, which can be useful for stack walking code which needs to verify the stack is sane.

However there are still a few types of tasks which don't yet follow that convention:

1) CPU idle tasks, aka the "swapper" tasks

2) freshly forked TIF_FORK tasks which don't have a stack at all

Make the idle tasks conform to the new stack bottom convention by starting their stack at a sizeof(pt_regs) offset from the end of the stack page.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/kernel/head_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/x86/kernel/head_64.S +0 -0

index 6dbd2c0..0b12311 100644
@@ -296,8 +296,9 @@ ENTRY(start_cpu)
         *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,

Thanks to all the recent x86 entry code refactoring, most tasks' kernel stacks start at the same offset right above their saved pt_regs, regardless of which syscall was used to enter the kernel. That creates a nice convention which makes it straightforward to identify the "bottom" of the stack, which can be useful for stack walking code which needs to verify the stack is sane.

However there are still a few types of tasks which don't yet follow that convention:

1) CPU idle tasks, aka the "swapper" tasks

2) freshly forked TIF_FORK tasks which don't have a stack at all

Make the idle tasks conform to the new stack bottom convention by starting their stack at a sizeof(pt_regs) offset from the end of the stack page.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/kernel/head_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/x86/kernel/head_64.S +3 -2

index 6dbd2c0..0b12311 100644
@@ -296,8 +296,9 @@ ENTRY(start_cpu)
         *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
         *              address given in m16:64.
         */
-       movq    initial_code(%rip),%rax
-       pushq   $0              # fake return address to stop unwinder
+       call    1f              # put return address on stack for unwinder
+1:     xorq    %rbp, %rbp      # clear frame pointer
+       movq    initial_code(%rip), %rax
        pushq   $__KERNEL_CS    # set correct cs
        pushq   %rax            # target address in negative space
        lretq

Quoted 6 lines · Josh Poimboeuf

A preempted function might not have had a chance to save the frame pointer to the stack yet, which can result in its caller getting skipped on a stack trace.

Add a flag to indicate when the task has been preempted so that stack dump code can determine whether the stack trace is reliable.

I think I like this, but how do you handle the rather similar case in which a task goes to sleep because it's waiting on IO that happened in response to get_user, put_user, copy_from_user, etc?

Quoted 5 lines · Josh Poimboeuf

Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for x86_64. The bit getting set indicates the thread has a pending patch which needs to be applied when the thread exits the kernel.

Quoted 1 line

The bit is placed in the least-significant word of the thread_info flags

NAK to that part.

The least-significant word thing is a huge hack that has gotten out of control. Please add the thing explicitly to all relevant masks.

Quoted 11 lines · Andy Lutomirski

Quoted 6 lines · Josh Poimboeuf

A preempted function might not have had a chance to save the frame pointer to the stack yet, which can result in its caller getting skipped on a stack trace.

Add a flag to indicate when the task has been preempted so that stack dump code can determine whether the stack trace is reliable.

I think I like this, but how do you handle the rather similar case in which a task goes to sleep because it's waiting on IO that happened in response to get_user, put_user, copy_from_user, etc?

Hm, good question. I was thinking that page faults had a dedicated stack, but now looking at the entry and traps code, that doesn't seem to be the case.

Anyway I think it shouldn't be a problem if we make sure that any kernel function which might trigger a valid page fault (e.g., copy_user_generic_string) do the proper frame pointer setup first. Then the stack should still be reliable.

In fact I might be able to teach objtool to enforce that: any function which uses an exception table should create a stack frame.

Or alternatively, maybe set some kind of flag for page faults, similar to what I did with this patch.

Quoted 22 lines · Josh Poimboeuf

Quoted 11 lines · Andy Lutomirski

Quoted 6 lines · Josh Poimboeuf

A preempted function might not have had a chance to save the frame pointer to the stack yet, which can result in its caller getting skipped on a stack trace.

Add a flag to indicate when the task has been preempted so that stack dump code can determine whether the stack trace is reliable.

I think I like this, but how do you handle the rather similar case in which a task goes to sleep because it's waiting on IO that happened in response to get_user, put_user, copy_from_user, etc?

Hm, good question. I was thinking that page faults had a dedicated stack, but now looking at the entry and traps code, that doesn't seem to be the case.

Anyway I think it shouldn't be a problem if we make sure that any kernel function which might trigger a valid page fault (e.g., copy_user_generic_string) do the proper frame pointer setup first. Then the stack should still be reliable.

Quoted 6 lines

In fact I might be able to teach objtool to enforce that: any function which uses an exception table should create a stack frame.

Or alternatively, maybe set some kind of flag for page faults, similar to what I did with this patch.

How about doing it the other way around: teach the unwinder to detect when it hits a non-outermost entry (i.e. it lands in idtentry, etc) and use some reasonable heuristic as to whether it's okay to keep unwinding. You should be able to handle preemption like that, too -- the unwind process will end up in an IRQ frame.

Quoted 33 lines · Josh Poimboeuf

Thanks to all the recent x86 entry code refactoring, most tasks' kernel stacks start at the same offset right above their saved pt_regs, regardless of which syscall was used to enter the kernel. That creates a nice convention which makes it straightforward to identify the "bottom" of the stack, which can be useful for stack walking code which needs to verify the stack is sane.

However there are still a few types of tasks which don't yet follow that convention:

1) CPU idle tasks, aka the "swapper" tasks

2) freshly forked TIF_FORK tasks which don't have a stack at all

Make the idle tasks conform to the new stack bottom convention by starting their stack at a sizeof(pt_regs) offset from the end of the stack page.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/kernel/head_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/x86/kernel/head_64.S +0 -2

index 6dbd2c0..0b12311 100644
@@ -296,8 +296,9 @@ ENTRY(start_cpu)
         *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
         *              address given in m16:64.
         */
-       movq    initial_code(%rip),%rax
-       pushq   $0              # fake return address to stop unwinder

arch/x86/kernel/head_64.S ENTRY(start_cpu) · line 304

Quoted 6 lines

arch/x86/kernel/head_64.S +3 -0

+       call    1f              # put return address on stack for unwinder
+1:     xorq    %rbp, %rbp      # clear frame pointer
+       movq    initial_code(%rip), %rax
        pushq   $__KERNEL_CS    # set correct cs
        pushq   %rax            # target address in negative space
        lretq

This chunk looks like it should be a separate patch.

Quoted 30 lines · Andy Lutomirski

Quoted 28 lines · Josh Poimboeuf

Quoted 11 lines · Andy Lutomirski

Quoted 6 lines · Josh Poimboeuf

A preempted function might not have had a chance to save the frame pointer to the stack yet, which can result in its caller getting skipped on a stack trace.

Add a flag to indicate when the task has been preempted so that stack dump code can determine whether the stack trace is reliable.

I think I like this, but how do you handle the rather similar case in which a task goes to sleep because it's waiting on IO that happened in response to get_user, put_user, copy_from_user, etc?

Hm, good question. I was thinking that page faults had a dedicated stack, but now looking at the entry and traps code, that doesn't seem to be the case.

Anyway I think it shouldn't be a problem if we make sure that any kernel function which might trigger a valid page fault (e.g., copy_user_generic_string) do the proper frame pointer setup first. Then the stack should still be reliable.

In fact I might be able to teach objtool to enforce that: any function which uses an exception table should create a stack frame.

Or alternatively, maybe set some kind of flag for page faults, similar to what I did with this patch.

Quoted 5 lines

How about doing it the other way around: teach the unwinder to detect when it hits a non-outermost entry (i.e. it lands in idtentry, etc) and use some reasonable heuristic as to whether it's okay to keep unwinding. You should be able to handle preemption like that, too -- the unwind process will end up in an IRQ frame.

How exactly would the unwinder detect if a text address is in an idtentry? Maybe put all the idt entries in a special ELF section?

Quoted 41 lines · Josh Poimboeuf

Thanks to all the recent x86 entry code refactoring, most tasks' kernel stacks start at the same offset right above their saved pt_regs, regardless of which syscall was used to enter the kernel. That creates a nice convention which makes it straightforward to identify the "bottom" of the stack, which can be useful for stack walking code which needs to verify the stack is sane.

However there are still a few types of tasks which don't yet follow that convention:

1) CPU idle tasks, aka the "swapper" tasks

2) freshly forked TIF_FORK tasks which don't have a stack at all

Make the idle tasks conform to the new stack bottom convention by starting their stack at a sizeof(pt_regs) offset from the end of the stack page.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/kernel/head_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/x86/kernel/head_64.S +3 -2

index 6dbd2c0..0b12311 100644
@@ -296,8 +296,9 @@ ENTRY(start_cpu)
         *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
         *              address given in m16:64.
         */
-       movq    initial_code(%rip),%rax
-       pushq   $0              # fake return address to stop unwinder
+       call    1f              # put return address on stack for unwinder
+1:     xorq    %rbp, %rbp      # clear frame pointer
+       movq    initial_code(%rip), %rax
        pushq   $__KERNEL_CS    # set correct cs
        pushq   %rax            # target address in negative space
        lretq

@@ -325,7 +326,7 @@ ENDPROC(start_cpu0)
        GLOBAL(initial_gs)

arch/x86/kernel/head_64.S ENDPROC(start_cpu0) · line 329

Quoted 4 lines

arch/x86/kernel/head_64.S +1 -1

        .quad   INIT_PER_CPU_VAR(irq_stack_union)
        GLOBAL(initial_stack)
-       .quad  init_thread_union+THREAD_SIZE-8
+       .quad  init_thread_union + THREAD_SIZE - SIZEOF_PTREGS

As long as you're doing this, could you also set orig_ax to -1? I remember running into some oddities resulting from orig_ax containing garbage at some point.

Quoted 31 lines · Josh Poimboeuf

Quoted 30 lines · Andy Lutomirski

Quoted 28 lines · Josh Poimboeuf

Quoted 11 lines · Andy Lutomirski

Quoted 6 lines · Josh Poimboeuf

A preempted function might not have had a chance to save the frame pointer to the stack yet, which can result in its caller getting skipped on a stack trace.

Add a flag to indicate when the task has been preempted so that stack dump code can determine whether the stack trace is reliable.

I think I like this, but how do you handle the rather similar case in which a task goes to sleep because it's waiting on IO that happened in response to get_user, put_user, copy_from_user, etc?

Hm, good question. I was thinking that page faults had a dedicated stack, but now looking at the entry and traps code, that doesn't seem to be the case.

Anyway I think it shouldn't be a problem if we make sure that any kernel function which might trigger a valid page fault (e.g., copy_user_generic_string) do the proper frame pointer setup first. Then the stack should still be reliable.

In fact I might be able to teach objtool to enforce that: any function which uses an exception table should create a stack frame.

Or alternatively, maybe set some kind of flag for page faults, similar to what I did with this patch.

Quoted 9 lines

Quoted 5 lines

How about doing it the other way around: teach the unwinder to detect when it hits a non-outermost entry (i.e. it lands in idtentry, etc) and use some reasonable heuristic as to whether it's okay to keep unwinding. You should be able to handle preemption like that, too -- the unwind process will end up in an IRQ frame.

How exactly would the unwinder detect if a text address is in an idtentry? Maybe put all the idt entries in a special ELF section?

Hmm.

What actually happens when you unwind all the way into the entry code?
Don't you end up in something that isn't in an ELF function? Can you
detect that?

Ideally, the unwinder could actually detect that it's
hit a pt_regs struct and report that. If used for stack dumps, it could display some indication of this and then continue its unwinding by decoding the pt_regs. If used for patching, it could take some other appropriate action.

I would have no objection to annotating all the pt_regs-style entry code, whether by putting it in a separate section or by making a table of addresses.

There are a couple of nasty cases if NMI or MCE is involved but, as of 4.6, outside of NMI, MCE, and vmalloc faults (ugh!), there should always be a complete pt_regs on the stack before interrupts get enabled for each entry. Of course, finding the thing may be nontrivial in case other things were pushed.

I suppose we could try to rejigger the code so that rbp points to pt_regs or similar.

Quoted 42 lines · Andy Lutomirski

Quoted 31 lines · Josh Poimboeuf

Quoted 30 lines · Andy Lutomirski

Quoted 28 lines · Josh Poimboeuf

Quoted 11 lines · Andy Lutomirski

Quoted 6 lines · Josh Poimboeuf

A preempted function might not have had a chance to save the frame pointer to the stack yet, which can result in its caller getting skipped on a stack trace.

Add a flag to indicate when the task has been preempted so that stack dump code can determine whether the stack trace is reliable.

I think I like this, but how do you handle the rather similar case in which a task goes to sleep because it's waiting on IO that happened in response to get_user, put_user, copy_from_user, etc?

Hm, good question. I was thinking that page faults had a dedicated stack, but now looking at the entry and traps code, that doesn't seem to be the case.

Anyway I think it shouldn't be a problem if we make sure that any kernel function which might trigger a valid page fault (e.g., copy_user_generic_string) do the proper frame pointer setup first. Then the stack should still be reliable.

In fact I might be able to teach objtool to enforce that: any function which uses an exception table should create a stack frame.

Or alternatively, maybe set some kind of flag for page faults, similar to what I did with this patch.

Quoted 9 lines

Quoted 5 lines

How about doing it the other way around: teach the unwinder to detect when it hits a non-outermost entry (i.e. it lands in idtentry, etc) and use some reasonable heuristic as to whether it's okay to keep unwinding. You should be able to handle preemption like that, too -- the unwind process will end up in an IRQ frame.

How exactly would the unwinder detect if a text address is in an idtentry? Maybe put all the idt entries in a special ELF section?

Quoted 5 lines

Hmm.

What actually happens when you unwind all the way into the entry code?
Don't you end up in something that isn't in an ELF function? Can you
detect that?

For entry from user space (e.g., syscalls), it's easy to detect because there's always a pt_regs at the bottom of the stack. So if the unwinder reaches the stack address at (thread.sp0 - sizeof(pt_regs)), it knows it's done.

But for nested entry (e.g. in-kernel irqs/exceptions like preemption and page faults which don't have dedicated stacks), where the pt_regs is stored somewhere in the middle of the stack instead of the bottom, there's no reliable way to detect that.

Quoted 6 lines

Ideally, the unwinder could actually detect that it's
hit a pt_regs struct and report that. If used for stack dumps, it could display some indication of this and then continue its unwinding by decoding the pt_regs. If used for patching, it could take some other appropriate action.

Quoted 3 lines

I would have no objection to annotating all the pt_regs-style entry code, whether by putting it in a separate section or by making a table of addresses.

I think the easiest way to make it work would be to modify the idtentry macro to put all the idt entries in a dedicated section. Then the unwinder could easily detect any calls from that code.

Quoted 6 lines

There are a couple of nasty cases if NMI or MCE is involved but, as of 4.6, outside of NMI, MCE, and vmalloc faults (ugh!), there should always be a complete pt_regs on the stack before interrupts get enabled for each entry. Of course, finding the thing may be nontrivial in case other things were pushed.

NMI, MCE and interrupts aren't a problem because they have dedicated stacks, which are easy to detect. If the tasks' stack is on an exception stack or an irq stack, we consider it unreliable.

And also, they don't sleep. The stack of any running task (other than current) is automatically considered unreliable anyway, since they could be modifying it while we're reading it.

Quoted 3 lines

I suppose we could try to rejigger the code so that rbp points to pt_regs or similar.

I think we should avoid doing something like that because it would break gdb and all the other unwinders who don't know about it.

Quoted 42 lines · Andy Lutomirski

Quoted 41 lines · Josh Poimboeuf

Thanks to all the recent x86 entry code refactoring, most tasks' kernel stacks start at the same offset right above their saved pt_regs, regardless of which syscall was used to enter the kernel. That creates a nice convention which makes it straightforward to identify the "bottom" of the stack, which can be useful for stack walking code which needs to verify the stack is sane.

However there are still a few types of tasks which don't yet follow that convention:

1) CPU idle tasks, aka the "swapper" tasks

2) freshly forked TIF_FORK tasks which don't have a stack at all

Make the idle tasks conform to the new stack bottom convention by starting their stack at a sizeof(pt_regs) offset from the end of the stack page.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/kernel/head_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/x86/kernel/head_64.S +3 -2

index 6dbd2c0..0b12311 100644
@@ -296,8 +296,9 @@ ENTRY(start_cpu)
         *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
         *              address given in m16:64.
         */
-       movq    initial_code(%rip),%rax
-       pushq   $0              # fake return address to stop unwinder
+       call    1f              # put return address on stack for unwinder
+1:     xorq    %rbp, %rbp      # clear frame pointer
+       movq    initial_code(%rip), %rax
        pushq   $__KERNEL_CS    # set correct cs
        pushq   %rax            # target address in negative space
        lretq

@@ -325,7 +326,7 @@ ENDPROC(start_cpu0)
        GLOBAL(initial_gs)

Quoted 8 lines

arch/x86/kernel/head_64.S ENDPROC(start_cpu0) · line 329

Quoted 4 lines

arch/x86/kernel/head_64.S +1 -1

        .quad   INIT_PER_CPU_VAR(irq_stack_union)
        GLOBAL(initial_stack)
-       .quad  init_thread_union+THREAD_SIZE-8
+       .quad  init_thread_union + THREAD_SIZE - SIZEOF_PTREGS

As long as you're doing this, could you also set orig_ax to -1? I remember running into some oddities resulting from orig_ax containing garbage at some point.

I assume you mean to initialize the orig_rax value in the pt_regs at the bottom of the stack of the idle task?

How could that cause a problem? Since the idle task never returns from a system call, I'd assume that memory never gets accessed?

--
Josh

Quoted 3 lines

NMI, MCE and interrupts aren't a problem because they have dedicated stacks, which are easy to detect. If the tasks' stack is on an exception stack or an irq stack, we consider it unreliable.

Only on x86_64.

Quoted 3 lines · Josh Poimboeuf

I think the easiest way to make it work would be to modify the idtentry macro to put all the idt entries in a dedicated section. Then the unwinder could easily detect any calls from that code.

That would work. Would it make sense to do the same for the irq entries?

Quoted 5 lines

Quoted 2 lines

I suppose we could try to rejigger the code so that rbp points to pt_regs or similar.

I think we should avoid doing something like that because it would break gdb and all the other unwinders who don't know about it.

How so?

Currently, rbp in the entry code is meaningless. I'm suggesting that, when we do, for example, 'call \do_sym' in idtentry, we point rbp to the pt_regs. Currently it points to something stale (which the dump_stack code might be relying on. Hmm.) But it's probably also safe to assume that if you unwind to the 'call \do_sym', then pt_regs is the next thing on the stack, so just doing the section thing would work.

We should really re-add DWARF some day.

Quoted 4 lines · Andy Lutomirski

Quoted 3 lines

NMI, MCE and interrupts aren't a problem because they have dedicated stacks, which are easy to detect. If the tasks' stack is on an exception stack or an irq stack, we consider it unreliable.

Quoted 1 line

Only on x86_64.

Well, MCEs are more or less x86-specific as well. But otherwise good point, thanks Andy.

So, how does stack layout generally look like in case when NMI is actually running on proper kernel stack? I thought it's guaranteed to contain pt_regs anyway in all cases. Is that not guaranteed to be the case?

Quoted 52 lines · Josh Poimboeuf

Quoted 50 lines · Andy Lutomirski

Quoted 40 lines · Josh Poimboeuf

Thanks to all the recent x86 entry code refactoring, most tasks' kernel stacks start at the same offset right above their saved pt_regs, regardless of which syscall was used to enter the kernel. That creates a nice convention which makes it straightforward to identify the "bottom" of the stack, which can be useful for stack walking code which needs to verify the stack is sane.

However there are still a few types of tasks which don't yet follow that convention:

1) CPU idle tasks, aka the "swapper" tasks

2) freshly forked TIF_FORK tasks which don't have a stack at all

Make the idle tasks conform to the new stack bottom convention by starting their stack at a sizeof(pt_regs) offset from the end of the stack page.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/kernel/head_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/x86/kernel/head_64.S +3 -2

index 6dbd2c0..0b12311 100644
@@ -296,8 +296,9 @@ ENTRY(start_cpu)
         *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
         *              address given in m16:64.
         */
-       movq    initial_code(%rip),%rax
-       pushq   $0              # fake return address to stop unwinder
+       call    1f              # put return address on stack for unwinder
+1:     xorq    %rbp, %rbp      # clear frame pointer
+       movq    initial_code(%rip), %rax
        pushq   $__KERNEL_CS    # set correct cs
        pushq   %rax            # target address in negative space
        lretq

arch/x86/kernel/head_64.S ENDPROC(start_cpu0) · line 329

Quoted 5 lines

arch/x86/kernel/head_64.S +1 -1

@@ -325,7 +326,7 @@ ENDPROC(start_cpu0)
        GLOBAL(initial_gs)
        .quad   INIT_PER_CPU_VAR(irq_stack_union)
        GLOBAL(initial_stack)
-       .quad  init_thread_union+THREAD_SIZE-8
+       .quad  init_thread_union + THREAD_SIZE - SIZEOF_PTREGS

As long as you're doing this, could you also set orig_ax to -1? I remember running into some oddities resulting from orig_ax containing garbage at some point.

Quoted 6 lines

I assume you mean to initialize the orig_rax value in the pt_regs at the bottom of the stack of the idle task?

How could that cause a problem? Since the idle task never returns from a system call, I'd assume that memory never gets accessed?

Look at collect_syscall in lib/syscall.c

Quoted 6 lines · Andy Lutomirski

Quoted 3 lines · Josh Poimboeuf

I think the easiest way to make it work would be to modify the idtentry macro to put all the idt entries in a dedicated section. Then the unwinder could easily detect any calls from that code.

That would work. Would it make sense to do the same for the irq entries?

Yes, I think so.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I suppose we could try to rejigger the code so that rbp points to pt_regs or similar.

I think we should avoid doing something like that because it would break gdb and all the other unwinders who don't know about it.

How so?

Quoted 7 lines

Currently, rbp in the entry code is meaningless. I'm suggesting that, when we do, for example, 'call \do_sym' in idtentry, we point rbp to the pt_regs. Currently it points to something stale (which the dump_stack code might be relying on. Hmm.) But it's probably also safe to assume that if you unwind to the 'call \do_sym', then pt_regs is the next thing on the stack, so just doing the section thing would work.

Yes, rbp is meaningless on the entry from user space. But if an in-kernel interrupt occurs (e.g. page fault, preemption) and you have nested entry, rbp keeps its old value, right? So the unwinder can walk past the nested entry frame and keep going until it gets to the original entry.

Quoted 2 lines

We should really re-add DWARF some day.

Working on it :-)

Quoted 4 lines · Andy Lutomirski

Quoted 3 lines

NMI, MCE and interrupts aren't a problem because they have dedicated stacks, which are easy to detect. If the tasks' stack is on an exception stack or an irq stack, we consider it unreliable.

Quoted 1 line

Only on x86_64.

Well, MCEs are more or less x86-specific as well. But otherwise good point, thanks Andy.

So, how does stack layout generally look like in case when NMI is actually running on proper kernel stack? I thought it's guaranteed to contain pt_regs anyway in all cases. Is that not guaranteed to be the case?

Quoted 60 lines · Andy Lutomirski

Quoted 58 lines · Josh Poimboeuf

Quoted 42 lines · Andy Lutomirski

Quoted 41 lines · Josh Poimboeuf

Thanks to all the recent x86 entry code refactoring, most tasks' kernel stacks start at the same offset right above their saved pt_regs, regardless of which syscall was used to enter the kernel. That creates a nice convention which makes it straightforward to identify the "bottom" of the stack, which can be useful for stack walking code which needs to verify the stack is sane.

However there are still a few types of tasks which don't yet follow that convention:

1) CPU idle tasks, aka the "swapper" tasks

2) freshly forked TIF_FORK tasks which don't have a stack at all

Make the idle tasks conform to the new stack bottom convention by starting their stack at a sizeof(pt_regs) offset from the end of the stack page.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/x86/kernel/head_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/x86/kernel/head_64.S +3 -2

index 6dbd2c0..0b12311 100644
@@ -296,8 +296,9 @@ ENTRY(start_cpu)
         *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
         *              address given in m16:64.
         */
-       movq    initial_code(%rip),%rax
-       pushq   $0              # fake return address to stop unwinder
+       call    1f              # put return address on stack for unwinder
+1:     xorq    %rbp, %rbp      # clear frame pointer
+       movq    initial_code(%rip), %rax
        pushq   $__KERNEL_CS    # set correct cs
        pushq   %rax            # target address in negative space
        lretq

@@ -325,7 +326,7 @@ ENDPROC(start_cpu0)
        GLOBAL(initial_gs)

Quoted 8 lines

arch/x86/kernel/head_64.S ENDPROC(start_cpu0) · line 329

Quoted 4 lines

arch/x86/kernel/head_64.S +1 -1

        .quad   INIT_PER_CPU_VAR(irq_stack_union)
        GLOBAL(initial_stack)
-       .quad  init_thread_union+THREAD_SIZE-8
+       .quad  init_thread_union + THREAD_SIZE - SIZEOF_PTREGS

As long as you're doing this, could you also set orig_ax to -1? I remember running into some oddities resulting from orig_ax containing garbage at some point.

I assume you mean to initialize the orig_rax value in the pt_regs at the bottom of the stack of the idle task?

How could that cause a problem? Since the idle task never returns from a system call, I'd assume that memory never gets accessed?

Quoted 1 line

Look at collect_syscall in lib/syscall.c

I don't see how collect_syscall() can be called for the per-cpu idle "swapper" tasks (which is what the above code affects). They don't have pids or /proc entries so you can't do /proc/<pid>/syscall on them.

--
Josh

Quoted 19 lines · Josh Poimboeuf

Quoted 9 lines · Andy Lutomirski

Quoted 5 lines · Josh Poimboeuf

Quoted 2 lines

I suppose we could try to rejigger the code so that rbp points to pt_regs or similar.

I think we should avoid doing something like that because it would break gdb and all the other unwinders who don't know about it.

How so?

Quoted 7 lines

Currently, rbp in the entry code is meaningless. I'm suggesting that, when we do, for example, 'call \do_sym' in idtentry, we point rbp to the pt_regs. Currently it points to something stale (which the dump_stack code might be relying on. Hmm.) But it's probably also safe to assume that if you unwind to the 'call \do_sym', then pt_regs is the next thing on the stack, so just doing the section thing would work.

Quoted 5 lines

Yes, rbp is meaningless on the entry from user space. But if an in-kernel interrupt occurs (e.g. page fault, preemption) and you have nested entry, rbp keeps its old value, right? So the unwinder can walk past the nested entry frame and keep going until it gets to the original entry.

Yes.

It would be nice if we could do better, though, and actually notice the pt_regs and identify the entry. For example, I'd love to see "page fault, RIP=xyz" printed in the middle of a stack dump on a crash.

Also, I think that just following rbp links will lose the actual function that took the page fault (or whatever function pt_regs->ip actually points to).

Have you looked at my vdso unwinding test at all? If we could do something similar for the kernel, IMO it would make testing much more pleasant.

Quoted 28 lines · Andy Lutomirski

Quoted 19 lines · Josh Poimboeuf

Quoted 9 lines · Andy Lutomirski

Quoted 5 lines · Josh Poimboeuf

Quoted 2 lines

I suppose we could try to rejigger the code so that rbp points to pt_regs or similar.

I think we should avoid doing something like that because it would break gdb and all the other unwinders who don't know about it.

How so?

Quoted 7 lines

Currently, rbp in the entry code is meaningless. I'm suggesting that, when we do, for example, 'call \do_sym' in idtentry, we point rbp to the pt_regs. Currently it points to something stale (which the dump_stack code might be relying on. Hmm.) But it's probably also safe to assume that if you unwind to the 'call \do_sym', then pt_regs is the next thing on the stack, so just doing the section thing would work.

Quoted 5 lines

Yes, rbp is meaningless on the entry from user space. But if an in-kernel interrupt occurs (e.g. page fault, preemption) and you have nested entry, rbp keeps its old value, right? So the unwinder can walk past the nested entry frame and keep going until it gets to the original entry.

Yes.

Quoted 8 lines

It would be nice if we could do better, though, and actually notice the pt_regs and identify the entry. For example, I'd love to see "page fault, RIP=xyz" printed in the middle of a stack dump on a crash.

Also, I think that just following rbp links will lose the actual function that took the page fault (or whatever function pt_regs->ip actually points to).

Hm. I think we could fix all that in a more standard way. Whenever a new pt_regs frame gets saved on entry, we could also create a new stack frame which points to a fake kernel_entry() function. That would tell the unwinder there's a pt_regs frame without otherwise breaking frame pointers across the frame.

Then I guess we wouldn't need my other solution of putting the idt entries in a special section.

How does that sound?

Quoted 4 lines

Have you looked at my vdso unwinding test at all? If we could do something similar for the kernel, IMO it would make testing much more pleasant.

I found it, but I'm not sure what it would mean to do something similar for the kernel. Do you mean doing something like an NMI sampling-based approach where we periodically do a random stack sanity check?

Quoted 44 lines · Josh Poimboeuf

Quoted 28 lines · Andy Lutomirski

Quoted 19 lines · Josh Poimboeuf

Quoted 9 lines · Andy Lutomirski

Quoted 5 lines · Josh Poimboeuf

Quoted 2 lines

I suppose we could try to rejigger the code so that rbp points to pt_regs or similar.

I think we should avoid doing something like that because it would break gdb and all the other unwinders who don't know about it.

How so?

Quoted 7 lines

Currently, rbp in the entry code is meaningless. I'm suggesting that, when we do, for example, 'call \do_sym' in idtentry, we point rbp to the pt_regs. Currently it points to something stale (which the dump_stack code might be relying on. Hmm.) But it's probably also safe to assume that if you unwind to the 'call \do_sym', then pt_regs is the next thing on the stack, so just doing the section thing would work.

Quoted 5 lines

Yes, rbp is meaningless on the entry from user space. But if an in-kernel interrupt occurs (e.g. page fault, preemption) and you have nested entry, rbp keeps its old value, right? So the unwinder can walk past the nested entry frame and keep going until it gets to the original entry.

Yes.

Quoted 8 lines

It would be nice if we could do better, though, and actually notice the pt_regs and identify the entry. For example, I'd love to see "page fault, RIP=xyz" printed in the middle of a stack dump on a crash.

Also, I think that just following rbp links will lose the actual function that took the page fault (or whatever function pt_regs->ip actually points to).

Hm. I think we could fix all that in a more standard way. Whenever a new pt_regs frame gets saved on entry, we could also create a new stack frame which points to a fake kernel_entry() function. That would tell the unwinder there's a pt_regs frame without otherwise breaking frame pointers across the frame.

Quoted 4 lines

Then I guess we wouldn't need my other solution of putting the idt entries in a special section.

How does that sound?

Let me try to understand.

The normal call sequence is call; push %rbp; mov %rsp, %rbp. So rbp points to (prev rbp, prev rip) on the stack, and you can follow the chain back. Right now, on a user access page fault or similar, we have rbp (probably) pointing to the interrupted frame, and the interrupted rip isn't saved anywhere that a naive unwinder can find it. (It's in pt_regs, but the rbp chain skips right over that.)

We could change the entry code so that an interrupt / idtentry does:

push pt_regs
push kernel_entry
push %rbp
mov %rsp, %rbp
call handler
pop %rbp
addq $8, %rsp

or similar. That would make it appear that the actual C handler was caused by a dummy function "kernel_entry". Now the unwinder would get to kernel_entry, but it *still* wouldn't find its way to the calling frame, which only solves part of the problem. We could at least teach the unwinder how kernel_entry works and let it decode pt_regs to continue unwinding. This would be nice, and I think it could work.

I think I like this, except that, if it used a separate section, it could potentially be faster, as, for each actual entry type, the offset from the C handler frame to pt_regs is a foregone conclusion.

But this is pretty simple and performance is already abysmal in most handlers.

There's an added benefit to using a separate section, though: we could also annotate the calls with what type of entry they were so the unwinder could print it out nicely.

Quoted 7 lines

Quoted 3 lines

Have you looked at my vdso unwinding test at all? If we could do something similar for the kernel, IMO it would make testing much more pleasant.

I found it, but I'm not sure what it would mean to do something similar for the kernel. Do you mean doing something like an NMI sampling-based approach where we periodically do a random stack sanity check?

I was imagining something a little more strict: single-step interesting parts of the kernel and make sure that each step unwinds correctly. That could detect missing frames and similar.

Quoted 69 lines · Andy Lutomirski

Quoted 44 lines · Josh Poimboeuf

Quoted 28 lines · Andy Lutomirski

Quoted 19 lines · Josh Poimboeuf

Quoted 9 lines · Andy Lutomirski

Quoted 5 lines · Josh Poimboeuf

Quoted 2 lines

I suppose we could try to rejigger the code so that rbp points to pt_regs or similar.

I think we should avoid doing something like that because it would break gdb and all the other unwinders who don't know about it.

How so?

Quoted 7 lines

Currently, rbp in the entry code is meaningless. I'm suggesting that, when we do, for example, 'call \do_sym' in idtentry, we point rbp to the pt_regs. Currently it points to something stale (which the dump_stack code might be relying on. Hmm.) But it's probably also safe to assume that if you unwind to the 'call \do_sym', then pt_regs is the next thing on the stack, so just doing the section thing would work.

Quoted 5 lines

Yes, rbp is meaningless on the entry from user space. But if an in-kernel interrupt occurs (e.g. page fault, preemption) and you have nested entry, rbp keeps its old value, right? So the unwinder can walk past the nested entry frame and keep going until it gets to the original entry.

Yes.

Quoted 8 lines

It would be nice if we could do better, though, and actually notice the pt_regs and identify the entry. For example, I'd love to see "page fault, RIP=xyz" printed in the middle of a stack dump on a crash.

Also, I think that just following rbp links will lose the actual function that took the page fault (or whatever function pt_regs->ip actually points to).

Hm. I think we could fix all that in a more standard way. Whenever a new pt_regs frame gets saved on entry, we could also create a new stack frame which points to a fake kernel_entry() function. That would tell the unwinder there's a pt_regs frame without otherwise breaking frame pointers across the frame.

Quoted 4 lines

Then I guess we wouldn't need my other solution of putting the idt entries in a special section.

How does that sound?

Let me try to understand.

The normal call sequence is call; push %rbp; mov %rsp, %rbp. So rbp points to (prev rbp, prev rip) on the stack, and you can follow the chain back. Right now, on a user access page fault or similar, we have rbp (probably) pointing to the interrupted frame, and the interrupted rip isn't saved anywhere that a naive unwinder can find it. (It's in pt_regs, but the rbp chain skips right over that.)

We could change the entry code so that an interrupt / idtentry does:

push pt_regs
push kernel_entry
push %rbp
mov %rsp, %rbp
call handler
pop %rbp
addq $8, %rsp

Quoted 6 lines

or similar. That would make it appear that the actual C handler was caused by a dummy function "kernel_entry". Now the unwinder would get to kernel_entry, but it *still* wouldn't find its way to the calling frame, which only solves part of the problem. We could at least teach the unwinder how kernel_entry works and let it decode pt_regs to continue unwinding. This would be nice, and I think it could work.

Yeah, that's about what I had in mind.

Quoted 4 lines

I think I like this, except that, if it used a separate section, it could potentially be faster, as, for each actual entry type, the offset from the C handler frame to pt_regs is a foregone conclusion.

Hm, this I don't really follow. It's true that the unwinder can easily find RIP from pt_regs, which will always be a known offset from the kernel_entry pointer on the stack. But why would having the entry code in a separate section make that faster?

Quoted 7 lines

But this is pretty simple and performance is already abysmal in most handlers.

There's an added benefit to using a separate section, though: we could also annotate the calls with what type of entry they were so the unwinder could print it out nicely.

Yeah, that could be a nice feature... but doesn't printing the name of the C handler pretty much already give that information?

In any case, once we have a working DWARF unwinder, I think it will show the name of the idt entry anyway.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Have you looked at my vdso unwinding test at all? If we could do something similar for the kernel, IMO it would make testing much more pleasant.

Quoted 7 lines

Quoted 3 lines

I found it, but I'm not sure what it would mean to do something similar for the kernel. Do you mean doing something like an NMI sampling-based approach where we periodically do a random stack sanity check?

I was imagining something a little more strict: single-step interesting parts of the kernel and make sure that each step unwinds correctly. That could detect missing frames and similar.

Interesting idea, though I wonder how hard it would be to reliably distinguish a missing frame from the case where gcc decides to inline a function.

Another idea to detect missing frames: for each return address on the stack, ensure there's a corresponding "call <func>" instruction immediately preceding the return location, where <func> matches what's on the stack.

Quoted 3 lines

Another idea to detect missing frames: for each return address on the stack, ensure there's a corresponding "call <func>" instruction immediately preceding the return location, where <func> matches what's on the stack.

Hmm, interesting.

I hope your plans include rewriting the current stack unwinder completely. The thing in print_context_stack is (a) hard-to-understand and hard-to-modify crap and (b) is called in a loop from another file using totally ridiculous conventions.

Quoted 70 lines · Josh Poimboeuf

Quoted 69 lines · Andy Lutomirski

Quoted 44 lines · Josh Poimboeuf

Quoted 28 lines · Andy Lutomirski

Quoted 19 lines · Josh Poimboeuf

Quoted 9 lines · Andy Lutomirski

Quoted 5 lines · Josh Poimboeuf

Quoted 2 lines

I suppose we could try to rejigger the code so that rbp points to pt_regs or similar.

I think we should avoid doing something like that because it would break gdb and all the other unwinders who don't know about it.

How so?

Quoted 7 lines

Currently, rbp in the entry code is meaningless. I'm suggesting that, when we do, for example, 'call \do_sym' in idtentry, we point rbp to the pt_regs. Currently it points to something stale (which the dump_stack code might be relying on. Hmm.) But it's probably also safe to assume that if you unwind to the 'call \do_sym', then pt_regs is the next thing on the stack, so just doing the section thing would work.

Quoted 5 lines

Yes, rbp is meaningless on the entry from user space. But if an in-kernel interrupt occurs (e.g. page fault, preemption) and you have nested entry, rbp keeps its old value, right? So the unwinder can walk past the nested entry frame and keep going until it gets to the original entry.

Yes.

Quoted 8 lines

It would be nice if we could do better, though, and actually notice the pt_regs and identify the entry. For example, I'd love to see "page fault, RIP=xyz" printed in the middle of a stack dump on a crash.

Also, I think that just following rbp links will lose the actual function that took the page fault (or whatever function pt_regs->ip actually points to).

Hm. I think we could fix all that in a more standard way. Whenever a new pt_regs frame gets saved on entry, we could also create a new stack frame which points to a fake kernel_entry() function. That would tell the unwinder there's a pt_regs frame without otherwise breaking frame pointers across the frame.

Quoted 4 lines

Then I guess we wouldn't need my other solution of putting the idt entries in a special section.

How does that sound?

Let me try to understand.

The normal call sequence is call; push %rbp; mov %rsp, %rbp. So rbp points to (prev rbp, prev rip) on the stack, and you can follow the chain back. Right now, on a user access page fault or similar, we have rbp (probably) pointing to the interrupted frame, and the interrupted rip isn't saved anywhere that a naive unwinder can find it. (It's in pt_regs, but the rbp chain skips right over that.)

We could change the entry code so that an interrupt / idtentry does:

push pt_regs
push kernel_entry
push %rbp
mov %rsp, %rbp
call handler
pop %rbp
addq $8, %rsp

Quoted 8 lines

Quoted 6 lines

or similar. That would make it appear that the actual C handler was caused by a dummy function "kernel_entry". Now the unwinder would get to kernel_entry, but it *still* wouldn't find its way to the calling frame, which only solves part of the problem. We could at least teach the unwinder how kernel_entry works and let it decode pt_regs to continue unwinding. This would be nice, and I think it could work.

Yeah, that's about what I had in mind.

FWIW, I just tried this:

static bool is_entry_text(unsigned long addr)

{
    return addr >= (unsigned long)__entry_text_start &&
        addr < (unsigned long)__entry_text_end;
}

it works. So the entry code is already annotated reasonably well :)

I just hacked it up here:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=stack&id=085eacfe0edfc18768e48340084415dba9a6bd21

and it seems to work, at least for page faults. A better implementation would print out the entire contents of pt_regs so that people reading the stack trace will know the registers at the time of the exception, which might be helpful.

Quoted 8 lines

Quoted 3 lines

I think I like this, except that, if it used a separate section, it could potentially be faster, as, for each actual entry type, the offset from the C handler frame to pt_regs is a foregone conclusion.

Hm, this I don't really follow. It's true that the unwinder can easily find RIP from pt_regs, which will always be a known offset from the kernel_entry pointer on the stack. But why would having the entry code in a separate section make that faster?

It doesn't make the unwinder faster -- it makes the entry code faster.

I hope your plans include rewriting the current stack unwinder completely. The thing in print_context_stack is (a) hard-to-understand and hard-to-modify crap and (b) is called in a loop from another file using totally ridiculous conventions.

FWIW, I just tried this:

static bool is_entry_text(unsigned long addr)

{
    return addr >= (unsigned long)__entry_text_start &&
        addr < (unsigned long)__entry_text_end;
}

it works. So the entry code is already annotated reasonably well :)

I just hacked it up here:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=stack&id=085eacfe0edfc18768e48340084415dba9a6bd21

and it seems to work, at least for page faults. A better implementation would print out the entire contents of pt_regs so that people reading the stack trace will know the registers at the time of the exception, which might be helpful.

Quoted 14 lines

FWIW, I just tried this:

static bool is_entry_text(unsigned long addr)

{
    return addr >= (unsigned long)__entry_text_start &&
        addr < (unsigned long)__entry_text_end;
}

it works. So the entry code is already annotated reasonably well :)

I just hacked it up here:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=stack&id=085eacfe0edfc18768e48340084415dba9a6bd21

Quoted 4 lines

and it seems to work, at least for page faults. A better implementation would print out the entire contents of pt_regs so that people reading the stack trace will know the registers at the time of the exception, which might be helpful.

Sorry for being dense, but how do you distinguish here between a "real" kernel entry, that pushes pt_regs, and any "non-entry" function call that passes pt_regs around?

Quoted 19 lines · Jiri Kosina

Quoted 18 lines

FWIW, I just tried this:

static bool is_entry_text(unsigned long addr)

{
    return addr >= (unsigned long)__entry_text_start &&
        addr < (unsigned long)__entry_text_end;
}

it works. So the entry code is already annotated reasonably well :)

I just hacked it up here:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=stack&id=085eacfe0edfc18768e48340084415dba9a6bd21

and it seems to work, at least for page faults. A better implementation would print out the entire contents of pt_regs so that people reading the stack trace will know the registers at the time of the exception, which might be helpful.

Quoted 3 lines

Sorry for being dense, but how do you distinguish here between a "real" kernel entry, that pushes pt_regs, and any "non-entry" function call that passes pt_regs around?

Umm, actually, the more tricky part is the other way around -- how do you make sure that whenever you are calling out from a code between __entry_text_start and __entry_text_end, pt_regs will be at the place you're looking for it? How's that guaranteed?

Thanks,

--
Jiri Kosina
SUSE Labs

Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for powerpc. The bit getting set indicates the thread has a pending patch which needs to be applied when the thread exits the kernel.

The bit is included in the _TIF_USER_WORK_MASK macro so that do_notify_resume() and klp_patch_task() get called when the bit is set.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/powerpc/include/asm/thread_info.h | 4 +++-
 arch/powerpc/kernel/signal.c           | 4 ++++
 2 files changed, 7 insertions(+), 1 deletion(-)

arch/powerpc/include/asm/thread_info.h +3 -1

index 8febc3f..df262ca 100644
@@ -88,6 +88,7 @@ static inline struct thread_info *current_thread_info(void)
 					   TIF_NEED_RESCHED */
 #define TIF_32BIT		4	/* 32 bit binary */
 #define TIF_RESTORE_TM		5	/* need to restore TM FP/VEC/VSX */
+#define TIF_PATCH_PENDING	6	/* pending live patching update */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SINGLESTEP		8	/* singlestepping active */
 #define TIF_NOHZ		9	/* in adaptive nohz mode */

@@ -111,6 +112,7 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
 #define _TIF_32BIT		(1<<TIF_32BIT)
 #define _TIF_RESTORE_TM		(1<<TIF_RESTORE_TM)
+#define _TIF_PATCH_PENDING	(1<<TIF_PATCH_PENDING)
 #define _TIF_SYSCALL_AUDIT	(1<<TIF_SYSCALL_AUDIT)
 #define _TIF_SINGLESTEP		(1<<TIF_SINGLESTEP)
 #define _TIF_SECCOMP		(1<<TIF_SECCOMP)

@@ -127,7 +129,7 @@ static inline struct thread_info *current_thread_info(void)
 
 #define _TIF_USER_WORK_MASK	(_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
 				 _TIF_NOTIFY_RESUME | _TIF_UPROBE | \
-				 _TIF_RESTORE_TM)
+				 _TIF_RESTORE_TM | _TIF_PATCH_PENDING)
 #define _TIF_PERSYSCALL_MASK	(_TIF_RESTOREALL|_TIF_NOERROR)
 
 /* Bits in local_flags */

arch/powerpc/kernel/signal.c +1 -0

index cb64d6f..844497b 100644
@@ -14,6 +14,7 @@
 #include <linux/uprobes.h>
 #include <linux/key.h>
 #include <linux/context_tracking.h>
+#include <linux/livepatch.h>
 #include <asm/hw_breakpoint.h>
 #include <asm/uaccess.h>
 #include <asm/unistd.h>

arch/powerpc/kernel/signal.c +0 -0

 	user_enter();
 }
 

Signature · 1 line

2.4.11

Once we have a consistency model, patches and their objects will be enabled and disabled at different times. For example, when a patch is disabled, its loaded objects' funcs can remain registered with ftrace indefinitely until the unpatching operation is complete and they're no longer in use.

It's less confusing if we give them different names: patches can be enabled or disabled; objects (and their funcs) can be patched or unpatched:

- Enabled means that a patch is logically enabled (but not necessarily fully applied).

- Patched means that an object's funcs are registered with ftrace and added to the klp_ops func stack.

Also, since these states are binary, represent them with booleans instead of ints.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 include/linux/livepatch.h | 17 ++++-------
 kernel/livepatch/core.c   | 72 +++++++++++++++++++++++------------------------
 2 files changed, 42 insertions(+), 47 deletions(-)

kernel/livepatch/core.c +0 -0

index 6ea6880..2b59230 100644

Move functions related to the actual patching of functions and objects into a new patch.c file.

kernel/livepatch/patch.c new file +0 -0

new file mode 100644
index 0000000..782fbb5

Quoted 59 lines · Josh Poimboeuf

Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for powerpc. The bit getting set indicates the thread has a pending patch which needs to be applied when the thread exits the kernel.

The bit is included in the _TIF_USER_WORK_MASK macro so that do_notify_resume() and klp_patch_task() get called when the bit is set.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/powerpc/include/asm/thread_info.h | 4 +++-
 arch/powerpc/kernel/signal.c           | 4 ++++
 2 files changed, 7 insertions(+), 1 deletion(-)

arch/powerpc/include/asm/thread_info.h +3 -1

index 8febc3f..df262ca 100644
@@ -88,6 +88,7 @@ static inline struct thread_info *current_thread_info(void)
 					   TIF_NEED_RESCHED */
 #define TIF_32BIT		4	/* 32 bit binary */
 #define TIF_RESTORE_TM		5	/* need to restore TM FP/VEC/VSX */
+#define TIF_PATCH_PENDING	6	/* pending live patching update */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SINGLESTEP		8	/* singlestepping active */
 #define TIF_NOHZ		9	/* in adaptive nohz mode */

@@ -111,6 +112,7 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
 #define _TIF_32BIT		(1<<TIF_32BIT)
 #define _TIF_RESTORE_TM		(1<<TIF_RESTORE_TM)
+#define _TIF_PATCH_PENDING	(1<<TIF_PATCH_PENDING)
 #define _TIF_SYSCALL_AUDIT	(1<<TIF_SYSCALL_AUDIT)
 #define _TIF_SINGLESTEP		(1<<TIF_SINGLESTEP)
 #define _TIF_SECCOMP		(1<<TIF_SECCOMP)

@@ -127,7 +129,7 @@ static inline struct thread_info *current_thread_info(void)
 
 #define _TIF_USER_WORK_MASK	(_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
 				 _TIF_NOTIFY_RESUME | _TIF_UPROBE | \
-				 _TIF_RESTORE_TM)
+				 _TIF_RESTORE_TM | _TIF_PATCH_PENDING)
 #define _TIF_PERSYSCALL_MASK	(_TIF_RESTOREALL|_TIF_NOERROR)
 
 /* Bits in local_flags */

arch/powerpc/kernel/signal.c +1 -0

index cb64d6f..844497b 100644
@@ -14,6 +14,7 @@
 #include <linux/uprobes.h>
 #include <linux/key.h>
 #include <linux/context_tracking.h>
+#include <linux/livepatch.h>
 #include <asm/hw_breakpoint.h>
 #include <asm/uaccess.h>
 #include <asm/unistd.h>

@@ -159,6 +160,9 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
 		tracehook_notify_resume(regs);
 	}
 

arch/powerpc/kernel/signal.c void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags) · line 165

Quoted 3 lines

arch/powerpc/kernel/signal.c +3 -0

+	if (thread_info_flags & _TIF_PATCH_PENDING)
+		klp_patch_task(current);
+

JFYI, if we later add the fake signal to speed up migration of sleeping task, we would need to move this up before calling do_signal(). It would help to avoid cycling here twice. Mirek surely knows more details about it.

Quoted 30 lines · Josh Poimboeuf

Once we have a consistency model, patches and their objects will be enabled and disabled at different times. For example, when a patch is disabled, its loaded objects' funcs can remain registered with ftrace indefinitely until the unpatching operation is complete and they're no longer in use.

It's less confusing if we give them different names: patches can be enabled or disabled; objects (and their funcs) can be patched or unpatched:

- Enabled means that a patch is logically enabled (but not necessarily fully applied).

- Patched means that an object's funcs are registered with ftrace and added to the klp_ops func stack.

Also, since these states are binary, represent them with booleans instead of ints.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 include/linux/livepatch.h | 17 ++++-------
 kernel/livepatch/core.c   | 72 +++++++++++++++++++++++------------------------
 2 files changed, 42 insertions(+), 47 deletions(-)

kernel/livepatch/core.c +0 -0

index 6ea6880..2b59230 100644

kernel/livepatch/core.c static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, · line 626

Quoted 6 lines

kernel/livepatch/core.c +1 -1

@@ -622,20 +622,20 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (ret)
 		return -EINVAL;
 
-	if (val != KLP_DISABLED && val != KLP_ENABLED)
+	if (val > 1)
 		return -EINVAL;

It would be cleaner to get "val" via kstrtobool(). It guarantees that the value is true or false. Another nice win is that it accepts Y/y/1/N/n/0 as the input.

kernel/livepatch/core.c static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, · line 632

Quoted 6 lines

kernel/livepatch/core.c +1 -1

 	patch = container_of(kobj, struct klp_patch, kobj);
 
 	mutex_lock(&klp_mutex);
 
-	if (val == patch->state) {
+	if (patch->enabled == val) {

Also this check will be cleaner if "val" is a boolean.

Change livepatch to use a basic per-task consistency model. This is the foundation which will eventually enable us to patch those ~10% of security patches which change function or data semantics. This is the biggest remaining piece needed to make livepatch more generally useful.

kernel/fork.c +1 -0

@@ -76,6 +76,7 @@
 #include <linux/compiler.h>
 #include <linux/sysctl.h>
 #include <linux/kcov.h>
+#include <linux/livepatch.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

kernel/fork.c +1 -0

+
 	spin_lock(&current->sighand->siglock);
 
 	/*

kernel/livepatch/transition.c new file +12 -0

new file mode 100644
index 0000000..92819bb
+/*
+ * This function can be called in the middle of an existing transition to
+ * reverse the direction of the target patch state.  This can be done to
+ * effectively cancel an existing enable or disable operation if there are any
+ * tasks which are stuck in the initial patch state.
+ */
+void klp_reverse_transition(void)
+{
+	struct klp_patch *patch = klp_transition_patch;
+
+	klp_target_state = !klp_target_state;
+

Change livepatch to use a basic per-task consistency model. This is the foundation which will eventually enable us to patch those ~10% of security patches which change function or data semantics. This is the biggest remaining piece needed to make livepatch more generally useful.

kernel/livepatch/patch.c +2 -0

index 782fbb5..b3b8639 100644
@@ -29,6 +29,7 @@
 #include <linux/bug.h>
 #include <linux/printk.h>
 #include "patch.h"
+#include "transition.h"
 
 static LIST_HEAD(klp_ops);
 

@@ -58,11 +59,42 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	ops = container_of(fops, struct klp_ops, fops);
 
 	rcu_read_lock();
+
 	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
 				      stack_node);

kernel/livepatch/patch.c +14 -0

+		if (current->patch_state == KLP_UNPATCHED) {
+			/*
+			 * Use the previously patched version of the function.
+			 * If no previous patches exist, use the original
+			 * function.
+			 */
+			func = list_entry_rcu(func->stack_node.next,
+					      struct klp_func, stack_node);
+
+			if (&func->stack_node == &ops->func_stack)
+				goto unlock;
+		}
+	}
+
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	rcu_read_unlock();

kernel/livepatch/transition.c new file +6 -0

new file mode 100644
index 0000000..92819bb
+/*
+ * klp_patch_task() - change the patched state of a task
+ * @task:	The task to change
+ *
+ * Switches the patched state of the task to the set of functions in the target
+ * patch state.

kernel/livepatch/transition.c new file +35 -0

+/*
+ * Initialize the global target patch state and all tasks to the initial patch
+ * state, and initialize all function transition states to true in preparation
+ * for patching or unpatching.
+ */
+void klp_init_transition(struct klp_patch *patch, int state)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+	struct klp_object *obj;
+	struct klp_func *func;
+	int initial_state = !state;
+
+	klp_transition_patch = patch;
+
+	/*
+	 * If the patch can be applied or reverted immediately, skip the
+	 * per-task transitions.
+	 */
+	if (patch->immediate)
+		return;
+
+	/*
+	 * Initialize all tasks to the initial patch state to prepare them for
+	 * switching to the target state.
+	 */
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task)
+		task->patch_state = initial_state;
+	read_unlock(&tasklist_lock);
+
+	/*
+	 * Ditto for the idle "swapper" tasks.
+	 */
+	get_online_cpus();

kernel/livepatch/transition.c new file +16 -0

+	/*
+	 * Set the func transition states so klp_ftrace_handler() will know to
+	 * switch to the transition logic.
+	 *
+	 * When patching, the funcs aren't yet in the func_stack and will be
+	 * made visible to the ftrace handler shortly by the calls to
+	 * klp_patch_object().
+	 *
+	 * When unpatching, the funcs are already in the func_stack and so are
+	 * already visible to the ftrace handler.
+	 */
+	klp_for_each_object(patch, obj)
+		klp_for_each_func(obj, func)
+			func->transition = true;
+
+	/*

kernel/livepatch/transition.c new file +20 -0

+	smp_wmb();
+}
+
+/*
+ * Start the transition to the specified target patch state so tasks can begin
+ * switching to it.
+ */
+void klp_start_transition(void)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	pr_notice("'%s': %s...\n", klp_transition_patch->mod->name,
+		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
+	/*
+	 * If the patch can be applied or reverted immediately, skip the
+	 * per-task transitions.
+	 */
+	if (klp_transition_patch->immediate)

kernel/livepatch/transition.c new file +21 -0

+	read_unlock(&tasklist_lock);
+
+	/*
+	 * Ditto for the idle "swapper" tasks, though they never cross the
+	 * syscall barrier.  Instead they switch over in cpu_idle_loop().
+	 */
+	get_online_cpus();
+	for_each_online_cpu(cpu)
+		set_tsk_thread_flag(idle_task(cpu), TIF_PATCH_PENDING);
+	put_online_cpus();
+}
+
+/*
+ * This function can be called in the middle of an existing transition to
+ * reverse the direction of the target patch state.  This can be done to
+ * effectively cancel an existing enable or disable operation if there are any
+ * tasks which are stuck in the initial patch state.
+ */
+void klp_reverse_transition(void)
+{
+	struct klp_patch *patch = klp_transition_patch;

kernel/livepatch/transition.c line 147

Quoted 6 lines

kernel/livepatch/transition.c new file +6 -0

+	 * This barrier also ensures that if another CPU goes through the
+	 * syscall barrier, sees the TIF_PATCH_PENDING writes in
+	 * klp_start_transition(), and calls klp_patch_task(), it also sees the
+	 * above write to the target state.  Otherwise it can put the task in
+	 * the wrong universe.
+	 */

By other words, it makes sure that klp_patch_task() will assign the right patch_state. Where klp_patch_task() could not be called before we set TIF_PATCH_PENDING in klp_start_transition().

Review

Quoted 2 lines

(quoted diff) +2 -0

+	smp_wmb();
+}

		 *
		 * Note that the task must never be migrated to the target
		 * state when being inside this ftrace handler.
		 */

We might want to move the second paragraph on top of the function. It is a basic and important fact. It actually explains why the first read barrier is not needed when the patch is being disabled.

Change livepatch to use a basic per-task consistency model. This is the foundation which will eventually enable us to patch those ~10% of security patches which change function or data semantics. This is the biggest remaining piece needed to make livepatch more generally useful.

kernel/livepatch/transition.c new file +20 -0

new file mode 100644
index 0000000..92819bb
+/*
+ * klp_patch_task() - change the patched state of a task
+ * @task:	The task to change
+ *
+ * Switches the patched state of the task to the set of functions in the target
+ * patch state.
+ */
+void klp_patch_task(struct task_struct *task)
+{
+	clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
+
+	/*
+	 * The corresponding write barriers are in klp_init_transition() and
+	 * klp_reverse_transition().  See the comments there for an explanation.
+	 */
+	smp_rmb();
+
+	task->patch_state = klp_target_state;
+}
+

kernel/sched/idle.c +1 -0

index bd12c6c..60d633f 100644
@@ -9,6 +9,7 @@
 #include <linux/mm.h>
 #include <linux/stackprotector.h>
 #include <linux/suspend.h>
+#include <linux/livepatch.h>
 
 #include <asm/tlb.h>
 

kernel/sched/idle.c static void cpu_idle_loop(void) · line 273

Quoted 4 lines

kernel/sched/idle.c +3 -0

+
+		if (unlikely(klp_patch_pending(current)))
+			klp_patch_task(current);
 	}

Some more ideas from the world of crazy races. I was shaking my head if this was safe or not.

The problem might be if the task get rescheduled between the check for the pending stuff

or inside the klp_patch_task() function.

Quoted 16 lines · Josh Poimboeuf

Change livepatch to use a basic per-task consistency model. This is the foundation which will eventually enable us to patch those ~10% of security patches which change function or data semantics. This is the biggest remaining piece needed to make livepatch more generally useful.

kernel/fork.c +1 -0

@@ -76,6 +76,7 @@
 #include <linux/compiler.h>
 #include <linux/sysctl.h>
 #include <linux/kcov.h>
+#include <linux/livepatch.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1590

Quoted 4 lines

kernel/fork.c +1 -0

@@ -1586,6 +1587,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		p->parent_exec_id = current->self_exec_id;
 	}
 
+	klp_copy_process(p);

I am in doubts here. We copy the state from the parent here. It means that the new process might still need to be converted. But at the same point print_context_stack_reliable() returns zero without printing any stack trace when TIF_FORK flag is set. It means that a freshly forked task might get be converted immediately. I seems that boot operations are always done when copy_process() is called. But they are contradicting each other.