Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Josh Poimboeuf <jpoimboe@kernel.org>]

On Tue, May 07, 2024 at 10:03:59PM +0800, Yafang Shao wrote:
> On Tue, May 7, 2024 at 10:35 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > User space needs to be polling for the transition to complete so it can
> > reverse the patch if it stalls.  Otherwise the patch could stall forever
> > and go unnoticed.
> >
> > Can't user space just unload the replaced module after it detects the
> > completed transition?
> 
> Are you referring to polling the
> "/sys/kernel/livepatch/XXX/transition"? The challenge lies in the
> uncertainty regarding which livepatches will be replaced and how many.
> Even if we can poll the transition status, there's no guarantee that a
> livepatch will be replaced by this operation.

If klp_patch.replace is set on the new patch then it will replace all
previous patches.

If the replaced patches remain in /sys/livepatch then you can simply
unload all the patches with enabled == 0, after the new patch succeeds
(enabled == 1 && transition == 0).

> > I'm not sure I see the benefit in complicating the kernel and possibly
> > introducing bugs, when unloading the module from user space seems to be
> > a perfectly valid option.
> >
> > Also, an error returned by delete_module() to the kernel would be
> > ignored and the module might remain in memory forever without being
> > noticed.
> 
> As Petr pointed out, we can enhance the functionality by checking the
> return value and providing informative error messages. This aligns
> with the user experience when deleting a module; if deletion fails,
> users have the option to try again. Similarly, if error messages are
> displayed, users can manually remove the module if needed.

Calling delete_module() from the kernel means there's no syscall with
which to return an error back to the user.

-- 
Josh