On Wed, Jan 25, 2023 at 12:34:26PM +0100, Petr Mladek wrote:

On Tue 2023-01-24 11:21:39, Seth Forshee wrote:

quoted

On Tue, Jan 24, 2023 at 03:17:43PM +0100, Petr Mladek wrote:

quoted

On Fri 2023-01-20 16:12:22, Seth Forshee (DigitalOcean) wrote:

quoted

Livepatch relies on stack checking of sleeping tasks to switch kthreads,
so a busy kthread can block a livepatch transition indefinitely. We've
seen this happen fairly often with busy vhost kthreads.

To be precise, it would be "indefinitely" only when the kthread never
sleeps.

But yes. I believe that the problem is real. It might be almost
impossible to livepatch some busy kthreads, especially when they
have a dedicated CPU.

quoted

Add a check to call klp_switch_current() from vhost_worker() when a
livepatch is pending. In testing this allowed vhost kthreads to switch
immediately when they had previously blocked livepatch transitions for
long periods of time.

Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>
---
 drivers/vhost/vhost.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index cbe72bfd2f1f..d8624f1f2d64 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -366,6 +367,9 @@ static int vhost_worker(void *data)
 			if (need_resched())
 				schedule();
 		}
+
+		if (unlikely(klp_patch_pending(current)))
+			klp_switch_current();

I suggest to use the following intead:

		if (unlikely(klp_patch_pending(current)))
			klp_update_patch_state(current);

We already use this in do_idle(). The reason is basically the same.
It is almost impossible to livepatch the idle task when a CPU is
very idle.

klp_update_patch_state(current) does not check the stack.
It switches the task immediately.

It should be safe because the kthread never leaves vhost_worker().
It means that the same kthread could never re-enter this function
and use the new code.

My knowledge of livepatching internals is fairly limited, so I'll accept
it if you say that it's safe to do it this way. But let me ask about one
scenario.

Let's say that a livepatch is loaded which replaces vhost_worker(). New
vhost worker threads are started which use the replacement function. Now
if the patch is disabled, these new worker threads would be switched
despite still running the code from the patch module, correct? Could the
module then be unloaded, freeing the memory containing the code these
kthreads are executing?

Great catch! Yes, this might theoretically happen.

The above scenario would require calling klp_update_patch_state() from
the code in the livepatch module. It is not possible at the moment because
this function is not exported for modules.

vhost can be built as a module, so in order to call
klp_update_patch_state() from vhost_worker() it would have to be
exported to modules.

Hmm, the same problem might be when we livepatch a function that calls
another function that calls klp_update_patch_state(). But in this case
it would be kthread() from kernel/kthread.c. It would affect any
running kthread. I doubt that anyone would seriously think about
livepatching this function.

Yes, there are clearly certain functions that are not safe/practical to
patch, and authors need to know what they are doing. Most kthread main()
functions probably qualify as impractical at best, at least without a
strategy to restart relevant kthreads.

But a livepatch transition will normally stall if patching these
functions when a relevant kthread is running (unless the patch is
forced), so a patch author who made a mistake should quickly notice.
vhost_worker() would behave differently.

A good enough solution might be to document this. Livepatches could
not be created blindly. There are more situations where the
livepatch is tricky or not possible at all.

I can add this if you like. Is Documentation/livepatch/livepatch.rst the
right place for this?

Crazy idea. We could prevent this problem even technically. A solution
would be to increment a per-process counter in klp_ftrace_handler() when a
function is redirected(). And klp_update_patch_state() might refuse
the migration when this counter is not zero. But it would require
to use a trampoline on return that would decrement the counter.
I am not sure if this is worth the complexity.

One the other hand, this counter might actually remove the need
of the reliable backtrace. It is possible that I miss something
or that it is not easy/possible to implement the return trampoline.

I agree this should work for unpatching, and even for patching a
function which is already patched.

Maybe I'm misunderstanding, but this would only work for unpatching or
patching an already-patched function, wouldn't it? Because the original
functions would not increment the counter so you would not know if tasks
still had those on their call stacks.

Back to the original problem. I still consider calling
klp_update_patch_state(current) in vhost_worker() safe.

Okay, I can send a v2 which does this, so long as it's okay to export
klp_update_patch_state() to modules.

Thanks,
Seth