On Tue 2023-01-24 11:21:39, Seth Forshee wrote: > On Tue, Jan 24, 2023 at 03:17:43PM +0100, Petr Mladek wrote: > > On Fri 2023-01-20 16:12:22, Seth Forshee (DigitalOcean) wrote: > > > 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. > > > > > > > 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@xxxxxxxxxx> > > > --- > > > 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. 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. 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. 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. Back to the original problem. I still consider calling klp_update_patch_state(current) in vhost_worker() safe. Best Regards, Petr
