[PATCH] x86/kvm: replace TASK_UNINTERRUPTIBLE with TASK_KILLABLE

[PATCH] x86/kvm: replace TASK_UNINTERRUPTIBLE with TASK_KILLABLE

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

There doesn't seem to be any advantage to having a *completely*
uninterruptible task here. For most users, allowing a task to respond
to the SIGKILL interrupt signal (all other signals are ignored just like
TASK_UNINTERRUPTIBLE) will not impact them at all.

However, for the rare edge-cases where a task becomes stuck, maybe due to
snapshot corruption or some other similarly unrecoverable error, it
is *much* more convenient for a user to be able to have the additional
option of nuking that task with SIGKILL, rather than annoying them by
forcing them to reboot in order to remove the immortal process.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index bc1a27280c4bf77899..7d4faee962e0c2e3c1 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -154,8 +154,8 @@ void kvm_async_pf_task_wait(u32 token, int interrupt_kernel)
 
 	for (;;) {
 		if (!n.halted)
-			prepare_to_swait(&n.wq, &wait, TASK_UNINTERRUPTIBLE);
-		if (hlist_unhashed(&n.link))
+			prepare_to_swait(&n.wq, &wait, TASK_KILLABLE);
+		if (hlist_unhashed(&n.link) || fatal_signal_pending(current))
 			break;
 
 		rcu_irq_exit();
-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] x86/kvm: replace TASK_UNINTERRUPTIBLE with TASK_KILLABLE

[Radim Krčmář]

2018-03-16 00:57-1000, Joey Pabalinas:
> There doesn't seem to be any advantage to having a *completely*
> uninterruptible task here. For most users, allowing a task to respond
> to the SIGKILL interrupt signal (all other signals are ignored just like
> TASK_UNINTERRUPTIBLE) will not impact them at all.
> 
> However, for the rare edge-cases where a task becomes stuck, maybe due to
> snapshot corruption or some other similarly unrecoverable error, it
> is *much* more convenient for a user to be able to have the additional
> option of nuking that task with SIGKILL, rather than annoying them by
> forcing them to reboot in order to remove the immortal process.
> 
> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
> 
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index bc1a27280c4bf77899..7d4faee962e0c2e3c1 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -154,8 +154,8 @@ void kvm_async_pf_task_wait(u32 token, int interrupt_kernel)
>  
>  	for (;;) {
>  		if (!n.halted)
> -			prepare_to_swait(&n.wq, &wait, TASK_UNINTERRUPTIBLE);
> -		if (hlist_unhashed(&n.link))
> +			prepare_to_swait(&n.wq, &wait, TASK_KILLABLE);

Makes sense, but it's not as easy:

> +		if (hlist_unhashed(&n.link) || fatal_signal_pending(current))

Removing the waiter from the list is currently waker's job, but the
entry is stack-allocated by waiter;  simply breaking there would cause
use after free on the waker's side.

We could take the lock again near the end of kvm_async_pf_task_wait to
remove the entry and use a variable (instead of hlist_unhashed) to
signal that the wakeup happened.

And there is another problem as well: If the waiting task is killed
before the wakeup arrives, the waker allocates a dummy entry that is not
going to be consumed by a future waiter, leading to a leak that could
potentially deplete the whole memory.

A solution to that could use a list of waiters that were killed before
the wakeup, so the normal working case wouldn't regress.

Doing that to handle snapshot corruption is definitely not worth it --
restoring from a snapshot should do apf_task_wake_all, so the corruption
would have to be very precise.

I remember we had a bug where tasks were getting stuck when running
nested and maybe there are going to be other cases to excuse the change.
I'm slightly against changing the behavior as it's pretty hard to test,
but I can be easily convinced with a well reasoned patch,

thanks!

Re: [PATCH] x86/kvm: replace TASK_UNINTERRUPTIBLE with TASK_KILLABLE

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]

On Thu, Mar 29, 2018 at 11:41:20PM +0200, Radim Krčmář wrote:
> I remember we had a bug where tasks were getting stuck when running
> nested and maybe there are going to be other cases to excuse the change.
> I'm slightly against changing the behavior as it's pretty hard to test,
> but I can be easily convinced with a well reasoned patch,
> 
> thanks!

Yes, that was quite a thorough explanation, thank you for that :). Took
me a while to read through all of your reply as well as look through the
accompanying code. I can definitely see that, yeah, this is quite a bit
more complicated that it appears at first glance. This will take a lot
more thought before tackling, and I am unsure if honestl it may be a bit
over my head as well, but I suppose I will see, haha.

Regardless, I did learn a lot, so thank you for taking the time to make
such a detailed reply!

If I do end up seeing something I can improve will definitely cook up
a patch and send it in, thanks again!

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]