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

Yafang Shao
2024-05-08T06:02:07Z

On Wed, May 8, 2024 at 1:16 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> 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.

A scenario exists wherein a user could simultaneously disable a loaded
livepatch, potentially resulting in it not being replaced by the new
patch. While theoretical, this possibility is not entirely
implausible.

>
> 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.

pr_error() can calso tell the user the error, right ?
If we must return an error to the user, probably we can use
klp_free_replaced_patches_sync() instead of
klp_free_replaced_patches_async().

Under what conditions might a kernel module of a disabled livepatch be
unable to be removed?

--
Regards
Yafang