kpatch: dynamic kernel patching

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied.

Quoted 4 lines · Josh Poimboeuf

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied.

How does that work for tail calls?

call foo
foo:

      ...
      jmp bar

bar:

      ... code executing ...

When you backtrace you will see foo, but you are running in bar.

Note that tail calls can be indirect, so they cannot be resolved statically.

CONFIG_DEBUG_INFO usually disables tail calls, but not supporting it would seem like a large limitation, as the cost can be high.

It wouldn't surprise me if there are some similar special cases that can even happen with them disabled.

In theory you could read LBRs, but even that may miss some extreme cases.

When bar returns, would it skip foo and go straight back to foo's caller? If so, then it should be safe to patch foo after it jumps to bar.

Quoted 3 lines

When bar returns, would it skip foo and go straight back to foo's caller? If so, then it should be safe to patch foo after it jumps to bar.

foo is no problem, you see it in the backtrace.
But you don't see bar.

Quoted 6 lines · Andi Kleen

Quoted 3 lines

When bar returns, would it skip foo and go straight back to foo's caller? If so, then it should be safe to patch foo after it jumps to bar.

foo is no problem, you see it in the backtrace.
But you don't see bar.

Sorry, I missed your point the first time. Good question.

stop_machine schedules a high priority thread on each CPU, which means every other task will be waiting in a schedule() call (assuming a non-preemptible kernel). In my local kernel, a quick grep of the disassembly doesn't show any jumps to schedule:

  $ egrep 'j.*<.*>' vmlinux.asm |grep -v '\+' |grep schedule
  ffffffff816b89b5:	e9 e2 fe ff ff       	jmpq   ffffffff816b889c <retint_with_reschedule>
  ffffffff816b8cec:	75 1e                	jne    ffffffff816b8d0c <paranoid_schedule>

But yes, that would be a problem if any tail call jumps to schedule() ever showed up. We may need to detect that case in our patch generation tooling and fail to create the patch module binary if the patch affects a function which does this.

Quoted 3 lines

When bar returns, would it skip foo and go straight back to foo's caller? If so, then it should be safe to patch foo after it jumps to bar.

foo is no problem, you see it in the backtrace.
But you don't see bar.

Since Jiri posted the kGraft patches [1], I wanted to share an alternative live patching solution called kpatch, which is something we've been working on at Red Hat for quite a while.

kpatch vs kGraft
----------------

I think the biggest difference between kpatch and kGraft is how they ensure that the patch is applied atomically and safely.

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

Quoted 3 lines · Josh Poimboeuf

Since Jiri posted the kGraft patches [1], I wanted to share an alternative live patching solution called kpatch, which is something we've been working on at Red Hat for quite a while.

Hi Josh, Seth,

thanks a lot for following up to our RFC with your submission, I am pretty sure this will help to energize the discussion and will provoke ideas for further improvements.

[ ... snip ... ]

Quoted 11 lines

kpatch vs kGraft
----------------

I think the biggest difference between kpatch and kGraft is how they ensure that the patch is applied atomically and safely.

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

Completely agreed with your comparative analysis, thanks for a nice summary.

Additional thing that I believe is important to add here: with the "stop-machine / check all tasks" aproach, there might be situations where you'll always fail to patch the system; if there is a long-time sleeper in the patched callchain, such a single sleeper is enough to make the patching of the whole system impossible.

With the lazy/gradual aproach implemented in kGraft, the whole system is gradually moving towards "fully patched" state and once all the sleepers blocking the process wake up, it ultimately converges to the fully patched state.

Thanks,

Signature · 2 lines

Jiri Kosina
SUSE Labs

kpatch vs kGraft
----------------

I think the biggest difference between kpatch and kGraft is how they ensure that the patch is applied atomically and safely.

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

Instead, kGraft uses per-task consistency: each task either sees the old version or the new version of the function. This gives a consistent view with respect to functions, but _not_ data, because the old and new functions are allowed to run simultaneously and share data. This could be dangerous if a patch changes how a function uses a data structure. The new function could make a data change that the old function wasn't expecting.

Quoted 12 lines · Josh Poimboeuf

kpatch vs kGraft
----------------

I think the biggest difference between kpatch and kGraft is how they ensure that the patch is applied atomically and safely.

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

Quoted 7 lines

Instead, kGraft uses per-task consistency: each task either sees the old version or the new version of the function. This gives a consistent view with respect to functions, but _not_ data, because the old and new functions are allowed to run simultaneously and share data. This could be dangerous if a patch changes how a function uses a data structure. The new function could make a data change that the old function wasn't expecting.

Please correct me if I am wrong, but with kPatch, you are also unable to do a "flip and forget" switch between functions that expect different format of in-memory data without performing a non-trivial all-memory lookup to find structures in question and perfoming corresponding transformations.

What we can do with kGraft si to perform the patching in two steps

(1) redirect to a temporary band-aid function that can handle both semantics of the data (persumably in highly sub-optimal way)
(2) patching in (1) succeeds completely (kGraft claims victory), start a new round of patching with redirect to the final function which expects only the new semantics

This basically implies that both aproaches need "human inspection" in this respect anyway.

Quoted 12 lines · Josh Poimboeuf

kpatch vs kGraft
----------------

I think the biggest difference between kpatch and kGraft is how they ensure that the patch is applied atomically and safely.

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

Quoted 7 lines

Instead, kGraft uses per-task consistency: each task either sees the old version or the new version of the function. This gives a consistent view with respect to functions, but _not_ data, because the old and new functions are allowed to run simultaneously and share data. This could be dangerous if a patch changes how a function uses a data structure. The new function could make a data change that the old function wasn't expecting.

Please correct me if I am wrong, but with kPatch, you are also unable to do a "flip and forget" switch between functions that expect different format of in-memory data without performing a non-trivial all-memory lookup to find structures in question and perfoming corresponding transformations.

What we can do with kGraft si to perform the patching in two steps

(1) redirect to a temporary band-aid function that can handle both semantics of the data (persumably in highly sub-optimal way)
(2) patching in (1) succeeds completely (kGraft claims victory), start a new round of patching with redirect to the final function which expects only the new semantics

This basically implies that both aproaches need "human inspection" in this respect anyway.

how would you know that all instances of the datastructure in memory have= been touched? just because all tasks have run and are outside the function in question doesn't tell you data structures have been converted. You have n= o way of knowing when (or if) the next call to the modified function will take place on any potential in-memory structure.

Quoted 6 lines · David Lang

how would you know that all instances of the datastructure in memory have= been touched? just because all tasks have run and are outside the function in question doesn't tell you data structures have been converted. You have n= o way of knowing when (or if) the next call to the modified function will take place on any potential in-memory structure.

The problem you are trying to avoid here is functions expecting to read "v2" format of the data from memory, while there are still tasks that are unpredictably writing "v1" format of the data to the memory.

There are several ways to attack this problem:

- stop the whole system, convert all the existing data structures to new format (which might potentially be non-trivial, mostly because you have to *know* where all the data structures have been allocated), apply patch, resume operation [ksplice, probably kpatch in future]
- restrict the data format to be backwards compatible [to be done manually during patch creation, currently what kGraft needs to do in such case]
- have a proxy code which can read both "v1" and "v2" formats, and writes back in the same format it has seen the data structure on input

- once all the *code* has been converted, it still has to understand "v1" and "v2", but it can now start writing out "v2" format only [possible with kGraft, not implemented in automated fashion]

Quoted 15 lines · Jiri Kosina

Quoted 6 lines · David Lang

how would you know that all instances of the datastructure in memory have= been touched? just because all tasks have run and are outside the function in question doesn't tell you data structures have been converted. You have n= o way of knowing when (or if) the next call to the modified function will take place on any potential in-memory structure.

The problem you are trying to avoid here is functions expecting to read "v2" format of the data from memory, while there are still tasks that are unpredictably writing "v1" format of the data to the memory.

There are several ways to attack this problem:

Quoted 9 lines

- stop the whole system, convert all the existing data structures to new format (which might potentially be non-trivial, mostly because you have to *know* where all the data structures have been allocated), apply patch, resume operation [ksplice, probably kpatch in future]
- restrict the data format to be backwards compatible [to be done manually during patch creation, currently what kGraft needs to do in such case]
- have a proxy code which can read both "v1" and "v2" formats, and writes back in the same format it has seen the data structure on input

doesn't this have the same problem of finding all the data?

Quoted 3 lines

- once all the *code* has been converted, it still has to understand "v1" and "v2", but it can now start writing out "v2" format only [possible with kGraft, not implemented in automated fashion]

which is a varient of the second one in that all the data needs to be tagged with a version so that it can be converted.

I don't see that 'stop the world' ends up being much better for this.

it does avoid any possibility of v1 code reading v2 data, but it doesn't help in avoiding v2 code reading v1 data some time (potentially much) later.

Quoted 6 lines · David Lang

how would you know that all instances of the datastructure in memory have= been touched? just because all tasks have run and are outside the function in question doesn't tell you data structures have been converted. You have n= o way of knowing when (or if) the next call to the modified function will take place on any potential in-memory structure.

The problem you are trying to avoid here is functions expecting to read "v2" format of the data from memory, while there are still tasks that are unpredictably writing "v1" format of the data to the memory.

There are several ways to attack this problem:

- stop the whole system, convert all the existing data structures to new format (which might potentially be non-trivial, mostly because you have to *know* where all the data structures have been allocated), apply patch, resume operation [ksplice, probably kpatch in future]
- restrict the data format to be backwards compatible [to be done manually during patch creation, currently what kGraft needs to do in such case]
- have a proxy code which can read both "v1" and "v2" formats, and writes back in the same format it has seen the data structure on input
- once all the *code* has been converted, it still has to understand "v1" and "v2", but it can now start writing out "v2" format only [possible with kGraft, not implemented in automated fashion]

Ideas are of course more than welcome.

So what I'm curious about, what is the actual 'in the field' distro experience, about the type of live-patches that get pushed with urgency?

My guess would be that the overwhelming majority of live-patches don't change data structures - and hence the right initial model would be to ensure (via tooling, and via review) that 'v1' and 'v2' data is exactly the same.

The most abstract way to 'live patch' a kernel is to do a checkpoint save on all application state, to reboot the kernel, boot into the patched kernel and then restore all application state seemlessly.

Quoted 28 lines · Jiri Kosina

Quoted 6 lines · David Lang

how would you know that all instances of the datastructure in memory have= been touched? just because all tasks have run and are outside the function in question doesn't tell you data structures have been converted. You have n= o way of knowing when (or if) the next call to the modified function will take place on any potential in-memory structure.

The problem you are trying to avoid here is functions expecting to read "v2" format of the data from memory, while there are still tasks that are unpredictably writing "v1" format of the data to the memory.

There are several ways to attack this problem:

- stop the whole system, convert all the existing data structures to new format (which might potentially be non-trivial, mostly because you have to *know* where all the data structures have been allocated), apply patch, resume operation [ksplice, probably kpatch in future]
- restrict the data format to be backwards compatible [to be done manually during patch creation, currently what kGraft needs to do in such case]
- have a proxy code which can read both "v1" and "v2" formats, and writes back in the same format it has seen the data structure on input
- once all the *code* has been converted, it still has to understand "v1" and "v2", but it can now start writing out "v2" format only [possible with kGraft, not implemented in automated fashion]

Quoted 1 line

Ideas are of course more than welcome.

So what I'm curious about, what is the actual 'in the field' distro experience, about the type of live-patches that get pushed with urgency?

My guess would be that the overwhelming majority of live-patches don't change data structures - and hence the right initial model would be to ensure (via tooling, and via review) that 'v1' and 'v2' data is exactly the same.

Quoted 33 lines · Ingo Molnar

Quoted 28 lines · Jiri Kosina

Quoted 6 lines · David Lang

how would you know that all instances of the datastructure in memory have= been touched? just because all tasks have run and are outside the function in question doesn't tell you data structures have been converted. You have n= o way of knowing when (or if) the next call to the modified function will take place on any potential in-memory structure.

The problem you are trying to avoid here is functions expecting to read "v2" format of the data from memory, while there are still tasks that are unpredictably writing "v1" format of the data to the memory.

There are several ways to attack this problem:

- stop the whole system, convert all the existing data structures to new format (which might potentially be non-trivial, mostly because you have to *know* where all the data structures have been allocated), apply patch, resume operation [ksplice, probably kpatch in future]
- restrict the data format to be backwards compatible [to be done manually during patch creation, currently what kGraft needs to do in such case]
- have a proxy code which can read both "v1" and "v2" formats, and writes back in the same format it has seen the data structure on input
- once all the *code* has been converted, it still has to understand "v1" and "v2", but it can now start writing out "v2" format only [possible with kGraft, not implemented in automated fashion]

Quoted 1 line

Ideas are of course more than welcome.

Quoted 8 lines

So what I'm curious about, what is the actual 'in the field' distro experience, about the type of live-patches that get pushed with urgency?

My guess would be that the overwhelming majority of live-patches don't change data structures - and hence the right initial model would be to ensure (via tooling, and via review) that 'v1' and 'v2' data is exactly the same.

Yes, in general we want to avoid data changes. In practice, we expect most patches to be small, localized security fixes, so it shouldn't be an issue in most cases.

Currently the kpatch tooling detects any compile-time changes to static data and refuses to build the patch module in that case.

But there's no way to programmatically detect changes to dynamic data. Which is why the user always has to be very careful when selecting a patch.

Quoted 50 lines · Josh Poimboeuf

Quoted 33 lines · Ingo Molnar

Quoted 28 lines · Jiri Kosina

Quoted 6 lines · David Lang

how would you know that all instances of the datastructure in memory have= been touched? just because all tasks have run and are outside the function in question doesn't tell you data structures have been converted. You have n= o way of knowing when (or if) the next call to the modified function will take place on any potential in-memory structure.

The problem you are trying to avoid here is functions expecting to read "v2" format of the data from memory, while there are still tasks that are unpredictably writing "v1" format of the data to the memory.

There are several ways to attack this problem:

- stop the whole system, convert all the existing data structures to new format (which might potentially be non-trivial, mostly because you have to *know* where all the data structures have been allocated), apply patch, resume operation [ksplice, probably kpatch in future]
- restrict the data format to be backwards compatible [to be done manually during patch creation, currently what kGraft needs to do in such case]
- have a proxy code which can read both "v1" and "v2" formats, and writes back in the same format it has seen the data structure on input
- once all the *code* has been converted, it still has to understand "v1" and "v2", but it can now start writing out "v2" format only [possible with kGraft, not implemented in automated fashion]

Quoted 1 line

Ideas are of course more than welcome.

Quoted 8 lines

So what I'm curious about, what is the actual 'in the field' distro experience, about the type of live-patches that get pushed with urgency?

My guess would be that the overwhelming majority of live-patches don't change data structures - and hence the right initial model would be to ensure (via tooling, and via review) that 'v1' and 'v2' data is exactly the same.

Yes, in general we want to avoid data changes. In practice, we expect most patches to be small, localized security fixes, so it shouldn't be an issue in most cases.

Currently the kpatch tooling detects any compile-time changes to static data and refuses to build the patch module in that case.

Quoted 3 lines

But there's no way to programmatically detect changes to dynamic data. Which is why the user always has to be very careful when selecting a patch.

And since this is about the system kernel it's dead easy to mess up a new kernel function and make the system unbootable - so it's not like 'be careful' isn't something implied already.

On Wed, 7 May 2014, Ingo Molnar wrote:

It's possible to have two versions of code that each work independently, but that you can't switch between easily on the fly.

If the new code assumes a lock is held that the old code didn't take, then when you switch, you are eventually going to hit a case where the new code trys to release a lock it doesn't hold.

detecting all possible cases progromatically seems close to impossible.

but this means that there are two categories of patches

1. patches that are safe to put in a kernel that you are going to boot from

2. patches that are able to be applied on the fly

and the tool isn't going to be able to tell you which category the patch is in. It can identify some of the items that make it unlikely or impossible for the patch to belong to #2, but don't rely on the tool catching all of them

Quoted 52 lines · Ingo Molnar

Quoted 50 lines · Josh Poimboeuf

Quoted 33 lines · Ingo Molnar

Quoted 28 lines · Jiri Kosina

Quoted 6 lines · David Lang

how would you know that all instances of the datastructure in memory have= been touched? just because all tasks have run and are outside the function in question doesn't tell you data structures have been converted. You have n= o way of knowing when (or if) the next call to the modified function will take place on any potential in-memory structure.

The problem you are trying to avoid here is functions expecting to read "v2" format of the data from memory, while there are still tasks that are unpredictably writing "v1" format of the data to the memory.

There are several ways to attack this problem:

- stop the whole system, convert all the existing data structures to new format (which might potentially be non-trivial, mostly because you have to *know* where all the data structures have been allocated), apply patch, resume operation [ksplice, probably kpatch in future]
- restrict the data format to be backwards compatible [to be done manually during patch creation, currently what kGraft needs to do in such case]
- have a proxy code which can read both "v1" and "v2" formats, and writes back in the same format it has seen the data structure on input
- once all the *code* has been converted, it still has to understand "v1" and "v2", but it can now start writing out "v2" format only [possible with kGraft, not implemented in automated fashion]

Quoted 1 line

Ideas are of course more than welcome.

Quoted 8 lines

So what I'm curious about, what is the actual 'in the field' distro experience, about the type of live-patches that get pushed with urgency?

My guess would be that the overwhelming majority of live-patches don't change data structures - and hence the right initial model would be to ensure (via tooling, and via review) that 'v1' and 'v2' data is exactly the same.

Yes, in general we want to avoid data changes. In practice, we expect most patches to be small, localized security fixes, so it shouldn't be an issue in most cases.

Currently the kpatch tooling detects any compile-time changes to static data and refuses to build the patch module in that case.

Quoted 7 lines

Quoted 3 lines

But there's no way to programmatically detect changes to dynamic data. Which is why the user always has to be very careful when selecting a patch.

And since this is about the system kernel it's dead easy to mess up a new kernel function and make the system unbootable - so it's not like 'be careful' isn't something implied already.

It's possible to have two versions of code that each work independently, but that you can't switch between easily on the fly.

If the new code assumes a lock is held that the old code didn't take, then when you switch, you are eventually going to hit a case where the new code trys to release a lock it doesn't hold.

detecting all possible cases progromatically seems close to impossible.

but this means that there are two categories of patches

1. patches that are safe to put in a kernel that you are going to boot from

2. patches that are able to be applied on the fly

and the tool isn't going to be able to tell you which category the patch is in. It can identify some of the items that make it unlikely or impossible for the patch to belong to #2, but don't rely on the tool catching all of them

[...]

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

Quoted 8 lines · Josh Poimboeuf

[...]

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

When live patching the kernel, how about achieving an even 'cleaner' state for all tasks in the system: to freeze all tasks, as the suspend and hibernation code (and kexec) does, via freeze_processes()?

That means no tasks in the system have any real kernel execution state, and there's also no problem with long-sleeping tasks, as freeze_processes() is supposed to be fast as well.

I.e. go for the most conservative live patching state first, and relax it only once the initial model is upstream and is working robustly.

Quoted 20 lines · Ingo Molnar

Quoted 8 lines · Josh Poimboeuf

[...]

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

When live patching the kernel, how about achieving an even 'cleaner' state for all tasks in the system: to freeze all tasks, as the suspend and hibernation code (and kexec) does, via freeze_processes()?

That means no tasks in the system have any real kernel execution state, and there's also no problem with long-sleeping tasks, as freeze_processes() is supposed to be fast as well.

Quoted 2 lines

I.e. go for the most conservative live patching state first, and relax it only once the initial model is upstream and is working robustly.

I had considered doing this before, but the problem I found is that many kernel threads are unfreezable. So we wouldn't be able to check whether its safe to replace any functions in use by those kernel threads.

Quoted 21 lines · Josh Poimboeuf

Quoted 20 lines · Ingo Molnar

Quoted 8 lines · Josh Poimboeuf

[...]

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

When live patching the kernel, how about achieving an even 'cleaner' state for all tasks in the system: to freeze all tasks, as the suspend and hibernation code (and kexec) does, via freeze_processes()?

That means no tasks in the system have any real kernel execution state, and there's also no problem with long-sleeping tasks, as freeze_processes() is supposed to be fast as well.

Quoted 7 lines

Quoted 2 lines

I.e. go for the most conservative live patching state first, and relax it only once the initial model is upstream and is working robustly.

I had considered doing this before, but the problem I found is that many kernel threads are unfreezable. So we wouldn't be able to check whether its safe to replace any functions in use by those kernel threads.

OTOH many kernel threads are parkable. Which achieves kind of similar desired behaviour: the kernel threads then aren't running.

And in fact we could implement freezing on top of park for kthreads.

But unfortunately there are still quite some of them which don't support parking.

Quoted 35 lines · Frederic Weisbecker

Quoted 21 lines · Josh Poimboeuf

Quoted 20 lines · Ingo Molnar

Quoted 8 lines · Josh Poimboeuf

[...]

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

When live patching the kernel, how about achieving an even 'cleaner' state for all tasks in the system: to freeze all tasks, as the suspend and hibernation code (and kexec) does, via freeze_processes()?

That means no tasks in the system have any real kernel execution state, and there's also no problem with long-sleeping tasks, as freeze_processes() is supposed to be fast as well.

Quoted 7 lines

Quoted 2 lines

I.e. go for the most conservative live patching state first, and relax it only once the initial model is upstream and is working robustly.

I had considered doing this before, but the problem I found is that many kernel threads are unfreezable. So we wouldn't be able to check whether its safe to replace any functions in use by those kernel threads.

OTOH many kernel threads are parkable. Which achieves kind of similar desired behaviour: the kernel threads then aren't running.

And in fact we could implement freezing on top of park for kthreads.

Quoted 2 lines

But unfortunately there are still quite some of them which don't support parking.

Well, if distros are moving towards live patching (and they are!), then it looks rather necessary to me that something scary as flipping out live kernel instructions with substantially different code should be as safe as possible, and only then fast.

If a kernel refuses to patch with certain threads running, that will drive those kernel threads being fixed and such. It's a deterministic, recoverable, reportable bug situation, so fixing it should be fast.

We learned these robustness lessons the hard way with kprobes and ftrace dynamic code patching... which are utterly simple compared to live kernel patching!

Thanks,

Ingo

Quoted 7 lines · Ingo Molnar

If a kernel refuses to patch with certain threads running, that will drive those kernel threads being fixed and such. It's a deterministic, recoverable, reportable bug situation, so fixing it should be fast.

We learned these robustness lessons the hard way with kprobes and ftrace dynamic code patching... which are utterly simple compared to live kernel patching!

Yeah, agreed. More rationale behind: we want to put the kthreads into semantic sleeps, not just random sleeping point. This way we lower the chances to execute new code messing up living state that is expecting old code after random preemption or sleeping points.

But by semantic sleeps I mean more than just explicit calls to schedule() as opposed to preemption points.
It also implies shutting down as well the living states handled by the kthread such that some sort of re-initialization of the state is also needed when the kthread gets back to run.

And that's exactly what good implementations of kthread park provide.

Consider kernel/watchdog.c as an example: when we park the lockup detector kthread, it disables the perf event and the hrtimer before it goes to actually park and sleep. When the kthread is later unparked, the kthread restarts the hrtimer and the perf event.

If we live patch code that has obscure relations with perf or hrtimer here, we lower a lot the chances for a crash when the watchdog kthread is parked.

So I'm in favour of parking all possible kthreads before live patching. Freezing alone doesn't provide the same state shutdown than parking.

Now since parking looks more widely implemented than kthread freezing, we could even think about implementing kthread freezing using parking as backend.

Live patching is a very sensitive and risky operation, and from a kernel standpoint we should make it as safe as we reasonably can. But we can't do much about careless users. Ultimately the risk is in the hands of the user and their choice of patches. They need to absolutely understand all the implications of patching a particular function. If the patch changes the way a function interacts with some external data, then they're starting to tempt fate and they need to be extra careful. This care needs to be taken for *all* kernel functions, not just for the few that are called from kernel threads.

Quoted 9 lines · On Tue, 6 May 2014 07:12:11 -0500 Josh Poimboeuf <[email protected]> wrote:

Live patching is a very sensitive and risky operation, and from a kernel standpoint we should make it as safe as we reasonably can. But we can't do much about careless users. Ultimately the risk is in the hands of the user and their choice of patches. They need to absolutely understand all the implications of patching a particular function. If the patch changes the way a function interacts with some external data, then they're starting to tempt fate and they need to be extra careful. This care needs to be taken for *all* kernel functions, not just for the few that are called from kernel threads.

Ideally the kpatch tools should be able to somewhat prevent users from doing damage. Or at least make them type a sentence that says:

I know what I'm doing and will not blame anyone but myself if this kills the system along with all my puppies and kittens.

I'm guessing that kpatch needs to be marketed that a distro or "hired help" will be creating the patch and the admin only needs to "trust" the one that gave them the kpatch module to load. All the testing/checking that the module works will be done by kernel developers and not by any "users".

Quoted 27 lines · Frederic Weisbecker

Quoted 7 lines · Ingo Molnar

If a kernel refuses to patch with certain threads running, that will drive those kernel threads being fixed and such. It's a deterministic, recoverable, reportable bug situation, so fixing it should be fast.

We learned these robustness lessons the hard way with kprobes and ftrace dynamic code patching... which are utterly simple compared to live kernel patching!

Yeah, agreed. More rationale behind: we want to put the kthreads into semantic sleeps, not just random sleeping point. This way we lower the chances to execute new code messing up living state that is expecting old code after random preemption or sleeping points.

But by semantic sleeps I mean more than just explicit calls to schedule() as opposed to preemption points.
It also implies shutting down as well the living states handled by the kthread such that some sort of re-initialization of the state is also needed when the kthread gets back to run.

And that's exactly what good implementations of kthread park provide.

Consider kernel/watchdog.c as an example: when we park the lockup detector kthread, it disables the perf event and the hrtimer before it goes to actually park and sleep. When the kthread is later unparked, the kthread restarts the hrtimer and the perf event.

Quoted 8 lines

If we live patch code that has obscure relations with perf or hrtimer here, we lower a lot the chances for a crash when the watchdog kthread is parked.

So I'm in favour of parking all possible kthreads before live patching. Freezing alone doesn't provide the same state shutdown than parking.

Now since parking looks more widely implemented than kthread freezing, we could even think about implementing kthread freezing using parking as backend.

The vast majority of kernel threads on my system don't seem to know anything about parking or freezing. I see one kthread function which calls kthread_should_park(), which is smpboot_thread_fn(), used for ksoftirqd/*, migration/* and watchdog/*. But there are many other kthread functions which seem to be parking ignorant, including:

cpu_idle_loop
kthreadd
rcu_gp_kthread
worker_thread
rescuer_thread
devtmpfsd
hub_thread
kswapd
ksm_scan_thread
khugepaged
fsnotify_mark_destroy
scsi_error_handler
kauditd_thread
kjournald2
irq_thread
rfcomm_run

Maybe we could modify all these thread functions (and probably more) to be park and/or freezer capable. But really it wouldn't make much of a difference IMO. It would only protect careless users from a tiny percentage of all possible havoc that a careless user could create.

Live patching is a very sensitive and risky operation, and from a kernel standpoint we should make it as safe as we reasonably can. But we can't do much about careless users. Ultimately the risk is in the hands of the user and their choice of patches. They need to absolutely understand all the implications of patching a particular function.

If the patch changes the way a function interacts with some external data, then they're starting to tempt fate and they need to be extra careful. This care needs to be taken for *all* kernel functions, not just for the few that are called from kernel threads.

Also, the top level kernel thread functions (like those listed above) will never be patchable anyway, because we never patch an in-use function (these functions are always in the threads' backtraces). This further diminishes the benefit of parking/freezing kernel threads.

Quoted 43 lines · Josh Poimboeuf

Quoted 27 lines · Frederic Weisbecker

Quoted 7 lines · Ingo Molnar

If a kernel refuses to patch with certain threads running, that will drive those kernel threads being fixed and such. It's a deterministic, recoverable, reportable bug situation, so fixing it should be fast.

We learned these robustness lessons the hard way with kprobes and ftrace dynamic code patching... which are utterly simple compared to live kernel patching!

Yeah, agreed. More rationale behind: we want to put the kthreads into semantic sleeps, not just random sleeping point. This way we lower the chances to execute new code messing up living state that is expecting old code after random preemption or sleeping points.

But by semantic sleeps I mean more than just explicit calls to schedule() as opposed to preemption points.
It also implies shutting down as well the living states handled by the kthread such that some sort of re-initialization of the state is also needed when the kthread gets back to run.

And that's exactly what good implementations of kthread park provide.

Consider kernel/watchdog.c as an example: when we park the lockup detector kthread, it disables the perf event and the hrtimer before it goes to actually park and sleep. When the kthread is later unparked, the kthread restarts the hrtimer and the perf event.

Quoted 8 lines

If we live patch code that has obscure relations with perf or hrtimer here, we lower a lot the chances for a crash when the watchdog kthread is parked.

So I'm in favour of parking all possible kthreads before live patching. Freezing alone doesn't provide the same state shutdown than parking.

Now since parking looks more widely implemented than kthread freezing, we could even think about implementing kthread freezing using parking as backend.

The vast majority of kernel threads on my system don't seem to know anything about parking or freezing. I see one kthread function which calls kthread_should_park(), which is smpboot_thread_fn(), used for ksoftirqd/*, migration/* and watchdog/*. But there are many other kthread functions which seem to be parking ignorant, including:

Quoted 16 lines

cpu_idle_loop
kthreadd
rcu_gp_kthread
worker_thread
rescuer_thread
devtmpfsd
hub_thread
kswapd
ksm_scan_thread
khugepaged
fsnotify_mark_destroy
scsi_error_handler
kauditd_thread
kjournald2
irq_thread
rfcomm_run

Yeah I now realize that only very few of them can park. Only infiniband, rcu, stop_machine and the watchdog...

But that's still a good direction to take if we want to make the kernel step by step more robust against live patching.

Quoted 11 lines

Maybe we could modify all these thread functions (and probably more) to be park and/or freezer capable. But really it wouldn't make much of a difference IMO. It would only protect careless users from a tiny percentage of all possible havoc that a careless user could create.

Live patching is a very sensitive and risky operation, and from a kernel standpoint we should make it as safe as we reasonably can. But we can't do much about careless users. Ultimately the risk is in the hands of the user and their choice of patches. They need to absolutely understand all the implications of patching a particular function.

Kernel developers will be a tiny minority of users of live patching.

Very few sysadmins know about kernel internals. You can't really ask them to judge if a patch is reasonably live patchable or not.

It's possible to appreciate a patch wrt. its size, or the fact that it came through a stable tree, so it's at worst mildly invasive.

The only thing that could make live patching safe is that a community eventually builds around it and carefully inspect patches in a stable release then provide a selection of safely live patchable pieces.

Other than that, expect people to do crazy things.

Quoted 4 lines

If the patch changes the way a function interacts with some external data, then they're starting to tempt fate and they need to be extra careful. This care needs to be taken for *all* kernel functions, not just for the few that are called from kernel threads.

It's actually very hard to tell if a given function isn't called by any kernel thread.

Quoted 4 lines

Also, the top level kernel thread functions (like those listed above) will never be patchable anyway, because we never patch an in-use function (these functions are always in the threads' backtraces). This further diminishes the benefit of parking/freezing kernel threads.

You're right to do some basic prevention. It's the bare minimum.

But keep in mind it's a tiny protection if you consider the top level function is only a small part of what is called by a kernel thread. And there is still a risk that the patched part is incompatible with previously executed code. For example when a patched function depends on a patched initialization but we executed the old version of the initialization already and we are going to execute the new patched function. It's actually a common pattern. And careful implementation of kthreads parking can help. But this is something that we can do step by step.

Ah this reminds me when we chased kprobes dangerous spots and we tried to declare __kprobes the functions which were too dangerous to hot patch.

We eventually gave up because it was impossible to fix everything. And that was only for kprobes!

So you can never tell if a given patch will impact a given kthread.

Quoted 7 lines

Ah this reminds me when we chased kprobes dangerous spots and we tried to declare __kprobes the functions which were too dangerous to hot patch.

We eventually gave up because it was impossible to fix everything. And that was only for kprobes!

Quoted 2 lines

So you can never tell if a given patch will impact a given kthread.

If the user (or the person creating the patch for them) doesn't understand all impacts of the patch, they have no business patching their kernel with it.

Quoted 7 lines · Josh Poimboeuf

Quoted 7 lines

Ah this reminds me when we chased kprobes dangerous spots and we tried to declare __kprobes the functions which were too dangerous to hot patch.

We eventually gave up because it was impossible to fix everything. And that was only for kprobes!

Quoted 6 lines

Quoted 2 lines

So you can never tell if a given patch will impact a given kthread.

If the user (or the person creating the patch for them) doesn't understand all impacts of the patch, they have no business patching their kernel with it.

I think what is being somewhat lost is this discussion is the distinction between:

1) is the patch safe
2) is the _live patching_ safe

It's really two different things. We should absolutely strive for live patching to be safe under all circumstances, as long as the patch being fed to it is safe in itself when building a new kernel the old fashioned way.

I.e. it's natural that a kernel can be messed up via a patch, but this subsystem should absolutely make sure that it will safely reject totally fine patches that are unsafe to live patch.

Quoted 28 lines · Ingo Molnar

Quoted 7 lines · Josh Poimboeuf

Quoted 7 lines

Ah this reminds me when we chased kprobes dangerous spots and we tried to declare __kprobes the functions which were too dangerous to hot patch.

We eventually gave up because it was impossible to fix everything. And that was only for kprobes!

Quoted 6 lines

Quoted 2 lines

So you can never tell if a given patch will impact a given kthread.

If the user (or the person creating the patch for them) doesn't understand all impacts of the patch, they have no business patching their kernel with it.

I think what is being somewhat lost is this discussion is the distinction between:

1) is the patch safe
2) is the _live patching_ safe

It's really two different things. We should absolutely strive for live patching to be safe under all circumstances, as long as the patch being fed to it is safe in itself when building a new kernel the old fashioned way.

Quoted 3 lines

I.e. it's natural that a kernel can be messed up via a patch, but this subsystem should absolutely make sure that it will safely reject totally fine patches that are unsafe to live patch.

Thanks, that's a very succinct way to put it. They are indeed two different things, but at the same time they're interrelated: determining whether a patch is safe requires making assumptions about how it will be applied.

Live patching does not enter into this question, ever. The correctness of a patch to the source does not depend on 'live patching' considerations in any way, shape or form.

Any mechanism that tries to blur these lines is broken by design.

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

"We think/hope it won't blow up in most cases and we automated some checks halfways" or "the user must know what he is doing" is really not something that I think is a good concept for something as fragile as live patching.

Quoted 29 lines · Josh Poimboeuf

Quoted 28 lines · Ingo Molnar

Quoted 7 lines · Josh Poimboeuf

Quoted 7 lines

Ah this reminds me when we chased kprobes dangerous spots and we tried to declare __kprobes the functions which were too dangerous to hot patch.

We eventually gave up because it was impossible to fix everything. And that was only for kprobes!

Quoted 6 lines

Quoted 2 lines

So you can never tell if a given patch will impact a given kthread.

If the user (or the person creating the patch for them) doesn't understand all impacts of the patch, they have no business patching their kernel with it.

I think what is being somewhat lost is this discussion is the distinction between:

1) is the patch safe
2) is the _live patching_ safe

It's really two different things. We should absolutely strive for live patching to be safe under all circumstances, as long as the patch being fed to it is safe in itself when building a new kernel the old fashioned way.

Quoted 8 lines

Quoted 3 lines

I.e. it's natural that a kernel can be messed up via a patch, but this subsystem should absolutely make sure that it will safely reject totally fine patches that are unsafe to live patch.

Thanks, that's a very succinct way to put it. They are indeed two different things, but at the same time they're interrelated: determining whether a patch is safe requires making assumptions about how it will be applied.

No!

A patch to the kernel source is 'safe' if it results in a correctly patched kernel source. Full stop!

Live patching does not enter into this question, ever. The correctness of a patch to the source does not depend on 'live patching' considerations in any way, shape or form.

Any mechanism that tries to blur these lines is broken by design.

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

It's similar to how kprobes (or ftrace) will safely reject or perform a live patching of the kernel.

So for example, there's this recent upstream kernel fix:

3ca9e5d36afb agp: info leak in agpioc_info_wrap()

which fixes an information leak. The 'patch' is Git commit 3ca9e5d36afb (i.e. it patches a very specific incoming kernel source tree that results in a specific outgoing source tree), and we know it's safe and correct.

Any live patching subsystem must make sure that if this patch is live-patched, that this attempt is either rejected safely or performed safely.

"We think/hope it won't blow up in most cases and we automated some checks halfways" or "the user must know what he is doing" is really not something that I think is a good concept for something as fragile as live patching.

Quoted 68 lines · Ingo Molnar

Quoted 29 lines · Josh Poimboeuf

Quoted 28 lines · Ingo Molnar

Quoted 7 lines · Josh Poimboeuf

Quoted 7 lines

Ah this reminds me when we chased kprobes dangerous spots and we tried to declare __kprobes the functions which were too dangerous to hot patch.

We eventually gave up because it was impossible to fix everything. And that was only for kprobes!

Quoted 6 lines

Quoted 2 lines

So you can never tell if a given patch will impact a given kthread.

If the user (or the person creating the patch for them) doesn't understand all impacts of the patch, they have no business patching their kernel with it.

I think what is being somewhat lost is this discussion is the distinction between:

1) is the patch safe
2) is the _live patching_ safe

It's really two different things. We should absolutely strive for live patching to be safe under all circumstances, as long as the patch being fed to it is safe in itself when building a new kernel the old fashioned way.

Quoted 8 lines

Quoted 3 lines

I.e. it's natural that a kernel can be messed up via a patch, but this subsystem should absolutely make sure that it will safely reject totally fine patches that are unsafe to live patch.

Thanks, that's a very succinct way to put it. They are indeed two different things, but at the same time they're interrelated: determining whether a patch is safe requires making assumptions about how it will be applied.

No!

A patch to the kernel source is 'safe' if it results in a correctly patched kernel source. Full stop!

Live patching does not enter into this question, ever. The correctness of a patch to the source does not depend on 'live patching' considerations in any way, shape or form.

Any mechanism that tries to blur these lines is broken by design.

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

It's similar to how kprobes (or ftrace) will safely reject or perform a live patching of the kernel.

So for example, there's this recent upstream kernel fix:

3ca9e5d36afb agp: info leak in agpioc_info_wrap()

which fixes an information leak. The 'patch' is Git commit 3ca9e5d36afb (i.e. it patches a very specific incoming kernel source tree that results in a specific outgoing source tree), and we know it's safe and correct.

Quoted 8 lines

Any live patching subsystem must make sure that if this patch is live-patched, that this attempt is either rejected safely or performed safely.

"We think/hope it won't blow up in most cases and we automated some checks halfways" or "the user must know what he is doing" is really not something that I think is a good concept for something as fragile as live patching.

In that case you will have to reject any kernel patch that changes any memory structure, because it's impossible as a general rule to say that changing memory structures is going to be safe (or even possible) to change.

that includes any access to memory that moves around a lock

Quoted 43 lines

Quoted 29 lines

No!

A patch to the kernel source is 'safe' if it results in a correctly patched kernel source. Full stop!

Live patching does not enter into this question, ever. The correctness of a patch to the source does not depend on 'live patching' considerations in any way, shape or form.

Any mechanism that tries to blur these lines is broken by design.

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

It's similar to how kprobes (or ftrace) will safely reject or perform a live patching of the kernel.

So for example, there's this recent upstream kernel fix:

3ca9e5d36afb agp: info leak in agpioc_info_wrap()

which fixes an information leak. The 'patch' is Git commit 3ca9e5d36afb (i.e. it patches a very specific incoming kernel source tree that results in a specific outgoing source tree), and we know it's safe and correct.

Quoted 8 lines

Any live patching subsystem must make sure that if this patch is live-patched, that this attempt is either rejected safely or performed safely.

"We think/hope it won't blow up in most cases and we automated some checks halfways" or "the user must know what he is doing" is really not something that I think is a good concept for something as fragile as live patching.

In that case you will have to reject any kernel patch that changes any memory structure, because it's impossible as a general rule to say that changing memory structures is going to be safe (or even possible) to change.

Quoted 1 line

that includes any access to memory that moves around a lock

Initially restricting it to such patches would be a good beginning - most of the security fixes are just failed checks, i.e. they don't typically even change any external (not on stack) memory structure, right?

Quoted 43 lines · Ingo Molnar

Quoted 43 lines

Quoted 29 lines

No!

A patch to the kernel source is 'safe' if it results in a correctly patched kernel source. Full stop!

Live patching does not enter into this question, ever. The correctness of a patch to the source does not depend on 'live patching' considerations in any way, shape or form.

Any mechanism that tries to blur these lines is broken by design.

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

It's similar to how kprobes (or ftrace) will safely reject or perform a live patching of the kernel.

So for example, there's this recent upstream kernel fix:

3ca9e5d36afb agp: info leak in agpioc_info_wrap()

which fixes an information leak. The 'patch' is Git commit 3ca9e5d36afb (i.e. it patches a very specific incoming kernel source tree that results in a specific outgoing source tree), and we know it's safe and correct.

Quoted 8 lines

Any live patching subsystem must make sure that if this patch is live-patched, that this attempt is either rejected safely or performed safely.

"We think/hope it won't blow up in most cases and we automated some checks halfways" or "the user must know what he is doing" is really not something that I think is a good concept for something as fragile as live patching.

In that case you will have to reject any kernel patch that changes any memory structure, because it's impossible as a general rule to say that changing memory structures is going to be safe (or even possible) to change.

Quoted 6 lines

Quoted 1 line

that includes any access to memory that moves around a lock

Initially restricting it to such patches would be a good beginning - most of the security fixes are just failed checks, i.e. they don't typically even change any external (not on stack) memory structure, right?

in terms of hit-patching kernels you are correct.

but that's a far cry from what it sounded like you were demanding (that it must handle any kernel patch)

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

Quoted 52 lines · David Lang

Quoted 49 lines · Ingo Molnar

Quoted 38 lines

Quoted 29 lines

No!

A patch to the kernel source is 'safe' if it results in a correctly patched kernel source. Full stop!

Live patching does not enter into this question, ever. The correctness of a patch to the source does not depend on 'live patching' considerations in any way, shape or form.

Any mechanism that tries to blur these lines is broken by design.

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

It's similar to how kprobes (or ftrace) will safely reject or perform a live patching of the kernel.

So for example, there's this recent upstream kernel fix:

3ca9e5d36afb agp: info leak in agpioc_info_wrap()

which fixes an information leak. The 'patch' is Git commit 3ca9e5d36afb (i.e. it patches a very specific incoming kernel source tree that results in a specific outgoing source tree), and we know it's safe and correct.

Quoted 8 lines

Any live patching subsystem must make sure that if this patch is live-patched, that this attempt is either rejected safely or performed safely.

"We think/hope it won't blow up in most cases and we automated some checks halfways" or "the user must know what he is doing" is really not something that I think is a good concept for something as fragile as live patching.

Quoted 6 lines

In that case you will have to reject any kernel patch that changes any memory structure, because it's impossible as a general rule to say that changing memory structures is going to be safe (or even possible) to change.

that includes any access to memory that moves around a lock

Initially restricting it to such patches would be a good beginning - most of the security fixes are just failed checks, i.e. they don't typically even change any external (not on stack) memory structure, right?

Quoted 4 lines

in terms of hit-patching kernels you are correct.

but that's a far cry from what it sounded like you were demanding (that it must handle any kernel patch)

No, I was not demanding that at all, my suggestion was:

Quoted 4 lines

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

Note the 'if'. It could start simple and stupid, and only allow cases where we know the patch must be trivially safe (because it does not do much in terms of disturbing globally visible state). That needs some tooling help, but apparently tooling help is in place already.

And then we can complicate it from there - but have a reasonably robust starting point that we _know_ works (as long as the implementation is correct).

Quoted 52 lines · David Lang

Quoted 43 lines · Ingo Molnar

Quoted 43 lines

Quoted 29 lines

No!

A patch to the kernel source is 'safe' if it results in a correctly patched kernel source. Full stop!

Live patching does not enter into this question, ever. The correctness of a patch to the source does not depend on 'live patching' considerations in any way, shape or form.

Any mechanism that tries to blur these lines is broken by design.

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

It's similar to how kprobes (or ftrace) will safely reject or perform a live patching of the kernel.

So for example, there's this recent upstream kernel fix:

3ca9e5d36afb agp: info leak in agpioc_info_wrap()

which fixes an information leak. The 'patch' is Git commit 3ca9e5d36afb (i.e. it patches a very specific incoming kernel source tree that results in a specific outgoing source tree), and we know it's safe and correct.

Quoted 8 lines

Any live patching subsystem must make sure that if this patch is live-patched, that this attempt is either rejected safely or performed safely.

"We think/hope it won't blow up in most cases and we automated some checks halfways" or "the user must know what he is doing" is really not something that I think is a good concept for something as fragile as live patching.

In that case you will have to reject any kernel patch that changes any memory structure, because it's impossible as a general rule to say that changing memory structures is going to be safe (or even possible) to change.

Quoted 6 lines

Quoted 1 line

that includes any access to memory that moves around a lock

Initially restricting it to such patches would be a good beginning - most of the security fixes are just failed checks, i.e. they don't typically even change any external (not on stack) memory structure, right?

Quoted 4 lines

in terms of hit-patching kernels you are correct.

but that's a far cry from what it sounded like you were demanding (that it must handle any kernel patch)

No, I was not demanding that at all, my suggestion was:

Quoted 4 lines

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

Note the 'if'. It could start simple and stupid, and only allow cases where we know the patch must be trivially safe (because it does not do much in terms of disturbing globally visible state). That needs some tooling help, but apparently tooling help is in place already.

And then we can complicate it from there - but have a reasonably robust starting point that we _know_ works (as long as the implementation is correct).

Quoted 7 lines

No, I was not demanding that at all, my suggestion was:

Quoted 4 lines

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

Quoted 8 lines

Note the 'if'. It could start simple and stupid, and only allow cases where we know the patch must be trivially safe (because it does not do much in terms of disturbing globally visible state). That needs some tooling help, but apparently tooling help is in place already.

And then we can complicate it from there - but have a reasonably robust starting point that we _know_ works (as long as the implementation is correct).

I really wonder if detecting a "trivially safe" patch is even possible.

Where do you draw the line with the following patches?

- add a call to another function which modifies global data

- add an early return or a goto which changes the way the function modifies (or no longer modifies) global data

- touch a local stack variable which results in global data being modified later in the function

- return a different value which causes the function's caller to modify data

Quoted 4 lines

in terms of hit-patching kernels you are correct.

but that's a far cry from what it sounded like you were demanding (that it must handle any kernel patch)

No, I was not demanding that at all, my suggestion was:

Quoted 4 lines

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

Note the 'if'. It could start simple and stupid, and only allow cases where we know the patch must be trivially safe (because it does not do much in terms of disturbing globally visible state). That needs some tooling help, but apparently tooling help is in place already.

Hi!

Quoted 12 lines

Quoted 4 lines

in terms of hit-patching kernels you are correct.

but that's a far cry from what it sounded like you were demanding (that it must handle any kernel patch)

No, I was not demanding that at all, my suggestion was:

Quoted 4 lines

My claim is that if a patch is correct/safe in the old fashioned way, then a fundamental principle is that a live patching subsystem must either safely apply, or safely reject the live patching attempt, independently from any user input.

Quoted 5 lines

Note the 'if'. It could start simple and stupid, and only allow cases where we know the patch must be trivially safe (because it does not do much in terms of disturbing globally visible state). That needs some tooling help, but apparently tooling help is in place already.

Actually, even if patch is very trivial, it will be difficult to determine if it is safe. Consider adding error check:

int
do_something(void)
{
#if 0
if (1)

    return -1;

#endif
return 0;
}

void
main(void)
{
if (do_something())

    printf("error happened\n");
}

Imagine changing that #if 0 to #if 1. But gcc at -O3 already optimized out the error message. So... do we compile whole second kernel and compare the binaries? I think I seen remark "don't try to do binary compares" somewhere...

Quoted 35 lines · Frederic Weisbecker

Quoted 21 lines · Josh Poimboeuf

Quoted 20 lines · Ingo Molnar

Quoted 8 lines · Josh Poimboeuf

[...]

kpatch checks the backtraces of all tasks in stop_machine() to ensure that no instances of the old function are running when the new function is applied. I think the biggest downside of this approach is that stop_machine() has to idle all other CPUs during the patching process, so it inserts a small amount of latency (a few ms on an idle system).

When live patching the kernel, how about achieving an even 'cleaner' state for all tasks in the system: to freeze all tasks, as the suspend and hibernation code (and kexec) does, via freeze_processes()?

That means no tasks in the system have any real kernel execution state, and there's also no problem with long-sleeping tasks, as freeze_processes() is supposed to be fast as well.

Quoted 7 lines

Quoted 2 lines

I.e. go for the most conservative live patching state first, and relax it only once the initial model is upstream and is working robustly.

I had considered doing this before, but the problem I found is that many kernel threads are unfreezable. So we wouldn't be able to check whether its safe to replace any functions in use by those kernel threads.

OTOH many kernel threads are parkable. Which achieves kind of similar desired behaviour: the kernel threads then aren't running.

And in fact we could implement freezing on top of park for kthreads.

Quoted 2 lines

But unfortunately there are still quite some of them which don't support parking.

Well, if distros are moving towards live patching (and they are!), then it looks rather necessary to me that something scary as flipping out live kernel instructions with substantially different code should be as safe as possible, and only then fast.

If a kernel refuses to patch with certain threads running, that will drive those kernel threads being fixed and such. It's a deterministic, recoverable, reportable bug situation, so fixing it should be fast.

We learned these robustness lessons the hard way with kprobes and ftrace dynamic code patching... which are utterly simple compared to live kernel patching!

However, I also think if users can accept such freezing wait-time, it means they can also accept kexec based "checkpoint-restart" patching. So, I think the final goal of the kpatch will be live patching without stopping the machine. I'm discussing the issue on github #138, but that is off-topic. :)

Quoted 6 lines · On Tue, 06 May 2014 20:45:50 +0900 Masami Hiramatsu…

However, I also think if users can accept such freezing wait-time, it means they can also accept kexec based "checkpoint-restart" patching. So, I think the final goal of the kpatch will be live patching without stopping the machine. I'm discussing the issue on github #138, but that is off-topic. :)

I agree with Ingo too. Being conservative at first is the right approach here. We should start out with a stop_machine making sure that everything is sane before we continue. Sure, that's not much different than a kexec, but lets take things one step at a time.

ftrace did the stop_machine (and still does for some archs), and slowly moved to a more efficient method. kpatch/kgraft should follow suit.

Quoted 5 lines

However, I also think if users can accept such freezing wait-time, it means they can also accept kexec based "checkpoint-restart" patching. So, I think the final goal of the kpatch will be live patching without stopping the machine. I'm discussing the issue on github #138, but that is off-topic. :)

I agree with Ingo too. Being conservative at first is the right approach here. We should start out with a stop_machine making sure that everything is sane before we continue. Sure, that's not much different than a kexec, but lets take things one step at a time.

ftrace did the stop_machine (and still does for some archs), and slowly moved to a more efficient method. kpatch/kgraft should follow suit.

Quoted 6 lines · Steven Rostedt

Quoted 5 lines

However, I also think if users can accept such freezing wait-time, it means they can also accept kexec based "checkpoint-restart" patching. So, I think the final goal of the kpatch will be live patching without stopping the machine. I'm discussing the issue on github #138, but that is off-topic. :)

Quoted 7 lines

I agree with Ingo too. Being conservative at first is the right approach here. We should start out with a stop_machine making sure that everything is sane before we continue. Sure, that's not much different than a kexec, but lets take things one step at a time.

ftrace did the stop_machine (and still does for some archs), and slowly moved to a more efficient method. kpatch/kgraft should follow suit.

I don't really agree here.

I actually believe that "lazy" switching kgraft is doing provides a little bit more in the sense of consistency than stop_machine()-based aproach.

Consider this scenario:

	void foo()
	{
		for (i=0; i<10000; i++) {
			bar(i);
			something_else(i);
		}
	}

Let's say you want to live-patch bar(). With stop_machine()-based aproach, you can easily end-up with old bar() and new bar() being called in two consecutive iterations before the loop is even exited, right? (especially on preemptible kernel, or if something_else() goes to sleep).

With lazy-switching implemented in kgraft, this can never happen.

So I'd like to ask for a little bit more explanation why you think the stop_machine()-based patching provides more sanity/consistency assurance than the lazy switching we're doing.

Thanks a lot,

Signature · 2 lines

Jiri Kosina
SUSE Labs

Quoted 10 lines

Consider this scenario:

	void foo()
	{
		for (i=0; i<10000; i++) {
			bar(i);
			something_else(i);
		}
	}

Quoted 4 lines

Let's say you want to live-patch bar(). With stop_machine()-based aproach, you can easily end-up with old bar() and new bar() being called in two consecutive iterations before the loop is even exited, right? (especially on preemptible kernel, or if something_else() goes to sleep).

Can you clarify why this would be a problem? Is it because the new bar() changed some data semantics which confused foo() or something_else()?

Quoted 10 lines · Josh Poimboeuf

Quoted 10 lines

Consider this scenario:

	void foo()
	{
		for (i=0; i<10000; i++) {
			bar(i);
			something_else(i);
		}
	}

Quoted 8 lines

Quoted 4 lines

Let's say you want to live-patch bar(). With stop_machine()-based aproach, you can easily end-up with old bar() and new bar() being called in two consecutive iterations before the loop is even exited, right? (especially on preemptible kernel, or if something_else() goes to sleep).

Can you clarify why this would be a problem? Is it because the new bar() changed some data semantics which confused foo() or something_else()?

I guess the example I used wasn't really completely illustrative, sorry for that. But I guess this has been answered later in the thread already; the thing is that you don't have a complete callgraph available (at least I believe you don't ...?), so you don't really know where your patched function will be called from, and thus you can't change function arguments or return value semantics; with lazy aproach, you can do that.

Quoted 6 lines · Steven Rostedt

Quoted 5 lines

However, I also think if users can accept such freezing wait-time, it means they can also accept kexec based "checkpoint-restart" patching. So, I think the final goal of the kpatch will be live patching without stopping the machine. I'm discussing the issue on github #138, but that is off-topic. :)

Quoted 7 lines

I agree with Ingo too. Being conservative at first is the right approach here. We should start out with a stop_machine making sure that everything is sane before we continue. Sure, that's not much different than a kexec, but lets take things one step at a time.

ftrace did the stop_machine (and still does for some archs), and slowly moved to a more efficient method. kpatch/kgraft should follow suit.

I don't really agree here.

I actually believe that "lazy" switching kgraft is doing provides a little bit more in the sense of consistency than stop_machine()-based aproach.

Consider this scenario:

	void foo()
	{
		for (i=0; i<10000; i++) {
			bar(i);
			something_else(i);
		}
	}

Let's say you want to live-patch bar(). With stop_machine()-based aproach, you can easily end-up with old bar() and new bar() being called in two consecutive iterations before the loop is even exited, right? (especially on preemptible kernel, or if something_else() goes to sleep).

With lazy-switching implemented in kgraft, this can never happen.

So I'd like to ask for a little bit more explanation why you think the stop_machine()-based patching provides more sanity/consistency assurance than the lazy switching we're doing.

Thanks a lot,

Quoted 23 lines · Jiri Kosina

Quoted 6 lines · Steven Rostedt

Quoted 5 lines

However, I also think if users can accept such freezing wait-time, it means they can also accept kexec based "checkpoint-restart" patching. So, I think the final goal of the kpatch will be live patching without stopping the machine. I'm discussing the issue on github #138, but that is off-topic. :)

Quoted 7 lines

I agree with Ingo too. Being conservative at first is the right approach here. We should start out with a stop_machine making sure that everything is sane before we continue. Sure, that's not much different than a kexec, but lets take things one step at a time.

ftrace did the stop_machine (and still does for some archs), and slowly moved to a more efficient method. kpatch/kgraft should follow suit.

I don't really agree here.

I actually believe that "lazy" switching kgraft is doing provides a little bit more in the sense of consistency than stop_machine()-based aproach.

Consider this scenario:

Quoted 7 lines

	void foo()
	{
		for (i=0; i<10000; i++) {
			bar(i);
			something_else(i);
		}
	}

In this case, I'd recommend you to add foo() to replacing target as dummy. Then, kpatch can ensure foo() is actually not running. :)

Quoted 6 lines

Let's say you want to live-patch bar(). With stop_machine()-based aproach, you can easily end-up with old bar() and new bar() being called in two consecutive iterations before the loop is even exited, right? (especially on preemptible kernel, or if something_else() goes to sleep).

With lazy-switching implemented in kgraft, this can never happen.

And I guess similar thing may happen with kgraft. If old function and new function share a non-auto variable and they modify it different way, the result will be unexpected by the mutual interference.

Quoted 2 lines

ftrace did the stop_machine (and still does for some archs), and slowly moved to a more efficient method. kpatch/kgraft should follow suit.

I don't really agree here.

I actually believe that "lazy" switching kgraft is doing provides a little bit more in the sense of consistency than stop_machine()-based aproach.

Consider this scenario:

	void foo()
	{
		for (i=0; i<10000; i++) {
			bar(i);
			something_else(i);
		}
	}

Let's say you want to live-patch bar(). With stop_machine()-based aproach, you can easily end-up with old bar() and new bar() being called in two consecutive iterations before the loop is even exited, right? (especially on preemptible kernel, or if something_else() goes to sleep).

With lazy-switching implemented in kgraft, this can never happen.

So I'd like to ask for a little bit more explanation why you think the stop_machine()-based patching provides more sanity/consistency assurance than the lazy switching we're doing.

Quoted 5 lines

With lazy-switching implemented in kgraft, this can never happen.

So I'd like to ask for a little bit more explanation why you think the stop_machine()-based patching provides more sanity/consistency assurance than the lazy switching we're doing.

Here's what I'm more concerned with. With "lazy" switching you can have two tasks running two different versions of bar(). What happens if the locking of data within bar changes? Say the data was protected incorrectly with mutex(X) and you now need to protect it with mutex(Y).

With stop machine, you can make sure everyone is out of bar() and all tasks will use the same mutex to access the data. But with a lazy approach, one task can be protecting the data with mutex(X) and the other with mutex(Y) causing both tasks to be accessing the data at the same time.

*That* is what I'm more concerned about.

That's true, and we come back to what has been said at the very beginning for both aproaches -- you can't really get away without manual human inspection of the patches that are being applied.

The case you have outlined is indeed problematic for the "lazy switching" aproach, and can be worked around (interim function, which takes both mutexes in well defined order, for example).

You can construct a broken locking scenario for stop_machine() aproach as well -- consider a case when you are exchaing a function which changes the locking order of two locks/mutexes. How do you deal with the rest of the code where the locks are being acquired, but not through the functions you've exchanged?

So again -- there is no disagreement, I believe, about the fact that "live patches" can't be reliably auto-generated, and human inspection will always be necessary. Given the intended use-case (serious CVEs mostly, handled by distro vendors), this is fine.

Why I still favor the stop_machine approach, is because the method of patching is a bit simpler that way. A "lazy" approach will be more complex and more likely to be buggy. The thing I'm arguing here is not the end result being a problem, but the implementation of the patching itself causing bugs.

I rather have a "lazy" approach,

but like ftrace and its breakpoint method, the stop_machine approach is the simpler way to make sure the patching works before we try to optimize it.