NMI handler live patching about kpatch HOT 5 CLOSED

Here is the new activeness safety check design that I'm thinking about:

• kpatch_register doesn't use stop_machine any more, and just calls kpatch_apply directly.
• kpatch_apply_patch protects the activeness safety check and updates to the function list with a write_lock_irqsave(&kpatch_lock).
• The ftrace handler's reading of the function list will be protected with a read_lock(&kpatch_lock). This means that even if the trace handler is called during the activeness safety check, it will wait for the check to complete and will not cause it to fail.
• The activeness safety check includes:
  • For tasks not currently running on a CPU, check their backtraces
  • For a task currently running on a CPU, run the backtrace check for that task with stop_one_cpu_nowait() on that CPU, so that the task is removed from the CPU before the backtrace check.
  • The tricky part here is ensuring that the entirety of each backtrace check occurs while the task is not running, otherwise we could get a corrupt stack or a false positive. This may need some coordination with the scheduler (or using something like task_rq_lock(), which is currently only an internal scheduler function).

The for_each_thread loop doesn't have to be atomic, because any future entries into kpatched functions will be paused by the read lock.

If the write lock is taken, and an NMI happens on a CPU other than the writer, and the NMI handler tries to call a kpatched function, it will spin on the kpatch_lock until the writer releases the lock.

If an NMI happens on the CPU with the writer (i.e. NMI interrupts the activeness safety and func list update), and the NMI handler tries to call a kpatched function, we will have to detect that scenario (in NMI and read_trylock fails) and panic.

The goal of this new design is to avoid using stop_machine if at all possible.

from kpatch.

@jpoimboe Hmm, why would you like to avoid using stop_machine and use lock? I don't think it's a good idea to dance with schedulers.
Instead, here I have another option to support NMI with stop_machine. This will involves CAS(compare and swap) but no lock.

1. prepare an atomic conflict_counter. And add an updating flag in kpatch_func.
2. when registering the patch, updating flag on each new kpatch_func set true.
3. right before stop_machine, clear the conflict_counter.(Actually, a trick is here, run a stop_machine ensures all running interrupt(including NMI) handlers exit until stop_machine callback runs)
4. in the stop_machine, after checked activeness and copied new kpatch_func list, do atomic CAS on conflict_counter to set 2 if it is 0.
5. if the CAS failed and conflict_counter should be 1, there may be at least one concurrently running NMI handler hits the new kpatch_func, this patch should be failed. Rollback and return -EBUSY
6. if the CAS succeeded, clear updating flag of all new kpatch_func and return 0.

On the other hand, the kpatch_ftrace_handler() does;

1. search kpatch_func by ip address
2. if failed to find kpatch_func, and if in_nmi() is true, goto step 5.
3. if we hits an updating kpatch_func and conflict_counter is not 2, goto step 5.
4. modify regs->ip and return
5. do atomic CAS on conflict_counter to set 1 if it is 0.
6. if the CAS is succeeded or the conflict_counter is 1, return without modifying regs->ip
7. if the CAS is failed and the conflict_counter is 2, go back to step 1 to retry

With this method, you can avoid using locks and no need to PANIC on patching too. :)

from kpatch.

Oops, I missed push the close button... sorry.

from kpatch.

I want to avoid using stop_machine, if possible, because it can introduce a lot of latency into the system, especially for systems with many CPUs. Also I've heard rumors that stop_machine is planned to be removed from the kernel someday.

However, I agree that my non-stop_machine design has problems: it relies on locking the scheduler, it creates a small possibility of panic on NMI, and it requires modifications to the kernel.

For now, I think your proposal is the way to go. I can work on implementing it, but I'll be at the Collaboration Summit next week, so it might be a few weeks. So feel free to submit a pull request ;-)

If we get a lot of complaints about latencies, or if stop_machine gets removed from the kernel, we'll have to revisit this issue someday.

from kpatch.

Fixed with #108.

from kpatch.