From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C36EDC54EAA for ; Wed, 25 Jan 2023 16:57:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235758AbjAYQ5j (ORCPT ); Wed, 25 Jan 2023 11:57:39 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50196 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234965AbjAYQ5i (ORCPT ); Wed, 25 Jan 2023 11:57:38 -0500 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5B93A12589; Wed, 25 Jan 2023 08:57:35 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sin.source.kernel.org (Postfix) with ESMTPS id B218FCE20BB; Wed, 25 Jan 2023 16:57:33 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9590EC433D2; Wed, 25 Jan 2023 16:57:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1674665851; bh=RjMnbTbIfoWSxtx16I229CedhH4XH1ufxYZp+NhU/kI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mII9TYLC4I7Bo9OMKdjwOy7BL8aCb8EUXhnH7d/sqJylpcSI0SMWJjnYZI1RENIgQ /dIF+IMzGiNKLV8r/n8osTPXtim7NzYn0Uz7N0tbkJcEFziyyrDSm+1zIuHiN7DxQo LvZ+KdiGJziorG/TTUYKsV5D+dUTAc8T/ke1x0PmOY9fmNILRQd3Jx4ZpPxftSO/mP xe0pmGi0gq2nyvo054mrhaOFfM7di6K6dPJvJAeMaS8E6KBv4ij5kKbwL45Ij6eNIa ONOFmxIYmIlvb2kkLBvYPhY3SwwfK3Ej9i4vIp9FI0LGGYVv1aeVBoMpIIfUKL/fwG ZNBkXXaCy8wLw== Date: Wed, 25 Jan 2023 10:57:30 -0600 From: Seth Forshee To: Petr Mladek Cc: Jason Wang , "Michael S. Tsirkin" , Jiri Kosina , Miroslav Benes , Joe Lawrence , Josh Poimboeuf , virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, netdev@vger.kernel.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] vhost: check for pending livepatches from vhost worker kthreads Message-ID: References: <20230120-vhost-klp-switching-v1-0-7c2b65519c43@kernel.org> <20230120-vhost-klp-switching-v1-2-7c2b65519c43@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, Jan 25, 2023 at 12:34:26PM +0100, Petr Mladek wrote: > 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) > > > > --- > > > > 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