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

Yafang Shao
2024-05-09T02:18:19Z

On Wed, May 8, 2024 at 3:03 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Wed, May 08, 2024 at 02:01:29PM +0800, Yafang Shao wrote:
> > On Wed, May 8, 2024 at 1:16 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > 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.
>
> Why does it matter whether it was replaced, or was disabled beforehand?
> Either way the end result is the same.

When users disable the livepatch, the corresponding kernel module may
sometimes be removed, while other times it remains intact. This
inconsistency has the potential to confuse users.

>
> > > > 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 ?
>
> The dmesg buffer isn't a reliable way to communicate errors to a user
> space process.
>
> > 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().
>
> It's async for a reason :-)
>
> > Under what conditions might a kernel module of a disabled livepatch be
> > unable to be removed?
>
> For example:
>
> - Some code grabbed a reference to it, or some module has a dependency
> on it
>
> - It has an init function but not an exit function
>
> - The module has already been removed due to some race
>
> - Some other unforeseen bug or race in the module exit path

This scenario is typical for a regular kernel module, but I'm
uncertain if the same applies to a disabled livepatch. Is there a
mechanism in place to ensure that the corresponding kernel module is
always removed?

--
Regards
Yafang