Re: [PATCH v4 4/5] livepatch: Clear relocations when klp_apply_object_relocs() fails

[Miroslav Benes <mbenes@suse.cz>]

On Fri, 23 Oct 2026, Petr Mladek wrote:

> On Fri 2026-09-18 15:59:59, Miroslav Benes wrote:
> > On Tue, 8 Sep 2026, Petr Mladek wrote:
> > 
> > > When a module is loaded, klp_module_coming() updates all enabled
> > > livepatches. If an error occurs, it delegates cleanup to
> > > klp_cleanup_module_patches_limited(). However, this cleanup loop skips
> > > the partially updated patch, leaving any changes made prior to failure
> > > unreverted.
> > > 
> > > One unhandled failure path occurs inside klp_apply_object_relocs(). On
> > > architectures like x86_64, apply_relocate_add() performs a verification
> > > step using memcmp() to check that memory contains the expected relocated
> > > or zeroed value. If relocations left behind by a failed patch are not
> > > cleared, subsequent patch operations or reloads can fail this validation.
> > > 
> > > Introduce klp_write_object_relocs_limited() to unwind and clear only the
> > > relocations that were successfully applied before klp_write_object_relocs()
> > > encountered an error.
> > > 
> > > There is no need to clear relocations for other objects in the failing
> > > patch because klp_module_coming() operates strictly on the specific
> > > module being loaded.
> > 
> > Hm, I spent some quite time on that and I am not sure if I deciphered 
> > everything correctly.
> > 
> > It seems to me that all error handling in those paths you are mentioning 
> > above is correct and the only problematic thing is that 
> > klp_apply_object_relocs() in klp_init_object_loaded() returns ret 
> > immediately which leaves partially applied relocations in case of an 
> > error. Following calls in klp_init_object_loaded() go through err: label 
> > where klp_free_object_loaded() is called which should be fine.
> > 
> > Is it correct?
> > 
> > Wouldn't be better to be somehow consistent and clean up right in the 
> > error path of klp_apply_object_relocs() call in klp_init_object_loaded() 
> > similarly to what is already there. After all we want to clean the object.
> 
> In fact, this is exactly what klp_write_object_relocs_limited() does.
> The new _limited() variant of the function is used just in two
> locations:
> 
>    1. It implements the original klp_write_object_relocs() where
>       it is called "unlimited".
> 
>    2. It is used recursively in its own error path.
> 
> I did this trick to avoid code duplication in the error path.
> 
> I agree that is is not obvious. I should mention it in the commit
> message. And probably even add a comment into the code.
> 
> Or would you prefer another solution, please?

I will admit that I do not understand what I meant back then. Your 
solution seems correct to me now.

Miroslav