Date:   Tue, 24 Jan 2023 11:21:39 -0600
From:   Seth Forshee <sforshee@kernel.org>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Jason Wang <jasowang@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jiri Kosina <jikos@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>,
        Joe Lawrence <joe.lawrence@redhat.com>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        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: <Y9ATo5FukOhphwqT@do-x1extreme>
References: <20230120-vhost-klp-switching-v1-0-7c2b65519c43@kernel.org>
 <20230120-vhost-klp-switching-v1-2-7c2b65519c43@kernel.org>
 <Y8/ohzRGcOiqsh69@alley>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y8/ohzRGcOiqsh69@alley>
Precedence: bulk

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@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?

Thanks,
Seth