Petr Mladek <pmladek@xxxxxxxx> writes:
> On Tue 2022-05-03 12:49:34, Seth Forshee wrote:
>> A task can be livepatched only when it is sleeping or it exits to
>> userspace. This may happen infrequently for a heavily loaded vCPU task,
>> leading to livepatch transition failures.
>
> This is misleading.
>
> First, the problem is not a loaded CPU. The problem is that the
> task might spend very long time in the kernel when handling
> some syscall.
>
> Second, there is no timeout for the transition in the kernel code.
> It might take very long time but it will not fail.
>
>> Fake signals will be sent to tasks which fail patching via stack
>> checking. This will cause running vCPU tasks to exit guest mode, but
>> since no signal is pending they return to guest execution without
>> exiting to userspace. Fix this by treating a pending livepatch migration
>> like a pending signal, exiting to userspace with EINTR. This allows the
>> task to be patched, and userspace should re-excecute KVM_RUN to resume
>> guest execution.
>
> It seems that the patch works as expected but it is far from clear.
> And the above description helps only partially. Let me try to
> explain it for dummies like me ;-)
>
> <explanation>
> The problem was solved by sending a fake signal, see the commit
> 0b3d52790e1cfd6b80b826 ("livepatch: Remove signal sysfs attribute").
> It was achieved by calling signal_wake_up(). It set TIF_SIGPENDING
> and woke the task. It interrupted the syscall and the task was
> transitioned when leaving to the userspace.
>
> signal_wake_up() was later replaced by set_notify_signal(),
> see the commit 8df1947c71ee53c7e21 ("livepatch: Replace
> the fake signal sending with TIF_NOTIFY_SIGNAL infrastructure").
> The difference is that set_notify_signal() uses TIF_NOTIFY_SIGNAL
> instead of TIF_SIGPENDING.
>
> The effect is the same when running on a real hardware. The syscall
> gets interrupted and exit_to_user_mode_loop() is called where
> the livepatch state is updated (task migrated).
>
> But it works a different way in kvm where the task works are
> called in the guest mode and the task does not return into
> the user space in the host mode.
> </explanation>
>
> The solution provided by this patch is a bit weird, see below.
>
>
>> In my testing, systems where livepatching would timeout after 60 seconds
>> were able to load livepatches within a couple of seconds with this
>> change.
>>
>> Signed-off-by: Seth Forshee <sforshee@xxxxxxxxxxxxxxxx>
>> ---
>> Changes in v2:
>> - Added _TIF_SIGPENDING to XFER_TO_GUEST_MODE_WORK
>> - Reworded commit message and comments to avoid confusion around the
>> term "migrate"
>>
>> include/linux/entry-kvm.h | 4 ++--
>> kernel/entry/kvm.c | 7 ++++++-
>> 2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/entry-kvm.h b/include/linux/entry-kvm.h
>> index 6813171afccb..bf79e4cbb5a2 100644
>> --- a/include/linux/entry-kvm.h
>> +++ b/include/linux/entry-kvm.h
>> @@ -17,8 +17,8 @@
>> #endif
>>
>> #define XFER_TO_GUEST_MODE_WORK \
>> - (_TIF_NEED_RESCHED | _TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL | \
>> - _TIF_NOTIFY_RESUME | ARCH_XFER_TO_GUEST_MODE_WORK)
>> + (_TIF_NEED_RESCHED | _TIF_SIGPENDING | _TIF_PATCH_PENDING | \
>> + _TIF_NOTIFY_SIGNAL | _TIF_NOTIFY_RESUME | ARCH_XFER_TO_GUEST_MODE_WORK)
>>
>> struct kvm_vcpu;
>>
>> diff --git a/kernel/entry/kvm.c b/kernel/entry/kvm.c
>> index 9d09f489b60e..98439dfaa1a0 100644
>> --- a/kernel/entry/kvm.c
>> +++ b/kernel/entry/kvm.c
>> @@ -14,7 +14,12 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
>> task_work_run();
>> }
>>
>> - if (ti_work & _TIF_SIGPENDING) {
>> + /*
>> + * When a livepatch is pending, force an exit to userspace
>> + * as though a signal is pending to allow the task to be
>> + * patched.
>> + */
>> + if (ti_work & (_TIF_SIGPENDING | _TIF_PATCH_PENDING)) {
>> kvm_handle_signal_exit(vcpu);
>> return -EINTR;
>> }
>
> This looks strange:
>
> + klp_send_signals() calls set_notify_signal(task) that sets
> TIF_NOTIFY_SIGNAL
>
> + xfer_to_guest_mode_work() handles TIF_NOTIFY_SIGNAL by calling
> task_work_run().
>
> + This patch calls kvm_handle_signal_exit(vcpu) when
> _TIF_PATCH_PENDING is set. It probably causes the guest
> to call exit_to_user_mode_loop() because TIF_PATCH_PENDING
> bit is set. But neither TIF_NOTIFY_SIGNAL not TIF_NOTIFY_SIGNAL
> is set so that it works different way than on the real hardware.
>
>
> Question:
>
> Does xfer_to_guest_mode_work() interrupts the syscall running
> on the guest?
It looks like xfer_to_guest_mode_work runs like exit_to_user_mode_loop,
just before guest mode is invoked.
> If "yes" then we do not need to call kvm_handle_signal_exit(vcpu).
> It will be enough to call:
>
> if (ti_work & _TIF_PATCH_PENDING)
> klp_update_patch_state(current);
>
> If "no" then I do not understand why TIF_NOTIFY_SIGNAL interrupts
> the syscall on the real hardware and not in kvm.
>
> Anyway, we either should make sure that TIF_NOTIFY_SIGNAL has the same
> effect on the real hardware and in kvm. Or we need another interface
> for the fake signal used by livepatching.
The point of TIF_NOTIFY_SIGNAL is to break out of interruptible kernel
loops. Once out of the interruptible kernel loop the expectation is the
returns to user space and on it's way runs the exit_to_user_mode_loop or
is architecture specific equivalent.
> Adding Jens Axboe and Eric into Cc.
Yes. This is interesting.
Reading through the history of kernel/entry/kvm.c I believe
I made ``conservative'' changes that has not helped this situation.
Long story short at one point it was thought that _TIF_SIGPENDING
and _TIF_NOTIFY_SIGNAL could be separated and they could not.
Unfortunately the work to separate their handling has not been
completely undone.
In this case it appears that the only reason xfer_to_guest_mode_work
touches task_work_run is because of the separation work done by Jens
Axboe. I don't see any kvm specific reason for _TIF_NOTIFY_SIGNAL
and _TIF_SIGPENDING to be treated differently. Meanwhile my cleanups
elsewhere have made the unnecessary _TIF_NOTIFY_SIGNAL special case
bigger in xfer_to_guest_mode_work.
I suspect the first step in fixing things really should be just handling
_TIF_SIGPENDING and _TIF_NOTIFY_SIGNAL the same.
static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
{
do {
int ret;
if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) {
kvm_handle_signal_exit(vcpu);
return -EINTR;
}
if (ti_work & _TIF_NEED_RESCHED)
schedule();
if (ti_work & _TIF_NOTIFY_RESUME)
resume_user_mode_work(NULL);
ret = arch_xfer_to_guest_mode_handle_work(vcpu, ti_work);
if (ret)
return ret;
ti_work = read_thread_flags();
} while (ti_work & XFER_TO_GUEST_MODE_WORK || need_resched());
return 0;
}
That said I do expect adding support for the live patching into
xfer_to_guest_mode_work, like there is in exit_to_user_mode_loop, is
probably a good idea. That should prevent the live patching code from
needing to set TIF_NOTIFY_SIGNAL.
Something like:
Thomas Gleixner's patch to make _TIF_PATCH_PENDING always available.
#define XFER_TO_GUEST_MODE_WORK \
(_TIF_NEED_RESCHED | _TIF_SIGPENDING | _TIF_PATCH_PENDING | \
_TIF_NOTIFY_SIGNAL | _TIF_NOTIFY_RESUME | ARCH_XFER_TO_GUEST_MODE_WORK)
static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
{
do {
int ret;
if (ti_work & _TIF_PATCH_PENDING)
klp_update_patch_state(current);
if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) {
kvm_handle_signal_exit(vcpu);
return -EINTR;
}
if (ti_work & _TIF_NEED_RESCHED)
schedule();
if (ti_work & _TIF_NOTIFY_RESUME)
resume_user_mode_work(NULL);
ret = arch_xfer_to_guest_mode_handle_work(vcpu, ti_work);
if (ret)
return ret;
ti_work = read_thread_flags();
} while (ti_work & XFER_TO_GUEST_MODE_WORK || need_resched());
return 0;
}
If the kvm and the live patching folks could check my thinking that
would be great.
Eric
