Re: [PATCH 3/3] livepatch: Clean up klp_init_object_loaded() when fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Failing `klp_apply_object_relocs()` triggers spurious architectural warnings due to unconditional clearing of unapplied relocations in the newly added error path.

Pre-existing issues:
- [High] If `klp_pre_patch_callback()` or `klp_patch_object()` fails during `klp_module_coming()`, the successfully initialized state from `klp_init_object_loaded()` is leaked. The cleanup function `klp_cleanup_module_patches_limited()` intentionally skips the currently failing patch, leaving applied relocations in the livepatch module uncleared and causing subsequent load attempts of the target module to persistently fail.
--

commit a90c142b7a6f2c5551f83382ec1fcf15614ffbac
Author: Petr Mladek <pmladek@xxxxxxxx>

livepatch: Clean up klp_init_object_loaded() when fails

When a module is loaded, klp_module_coming() iterates over patches and
calls klp_init_object_loaded(). If initialization fails, it delegates
cleanup to klp_cleanup_module_patches_limited(). This commit updates
klp_free_object_loaded() to revert all and only changes made by
klp_init_object_loaded().

Closes: https://lore.kernel.org/r/20260823062313.1321B1F000E9@xxxxxxxxxxxxxxx

> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index a6796cd6b65fd..706a4a7193514 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c

[ ... ]

> @@ -916,11 +920,17 @@ static int klp_init_object_loaded(struct klp_patch *patch,
>  		if (!ret) {
>  			pr_err("kallsyms size lookup failed for '%s' replacement\n",
>  			       func->old_name);
> -			return -ENOENT;
> +			ret = -ENOENT;
> +			goto err;
>  		}
>  	}
>  
>  	return 0;
> +
> +err:
> +	klp_free_object_loaded(patch, obj);
> +
> +	return ret;
>  }

[Severity: Medium]
Does this unconditionally clear unapplied relocations?

When klp_init_object_loaded() fails (for example, if a symbol lookup fails
after klp_apply_object_relocs() succeeds, or if klp_apply_object_relocs()
itself fails), this newly added error path calls klp_free_object_loaded().

This unconditionally invokes klp_clear_object_relocs(), which iterates over
all .klp.rela sections and attempts to un-apply them via
clear_relocate_add().

However, for sections that were never applied, the target memory remains
unmodified (zeroed). On architectures like x86, clear_relocate_add() uses
memcmp() to verify that the memory contains the expected relocated value.
Since it does not, it prints an "Invalid relocation target" warning for
every unapplied relocation entry, resulting in significant spurious warning
spam in the kernel log.

[ ... ]

> @@ -1274,8 +1284,8 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
>  			klp_unpatch_object(obj);
>  
>  			klp_post_unpatch_callback(obj);
> -			klp_clear_object_relocs(patch, obj);
> -			klp_free_object_loaded(obj);
> +			klp_free_object_loaded(patch, obj);
> +			obj->mod = NULL;
>  			break;
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does this cleanup logic leak relocations
if klp_module_coming() fails after klp_init_object_loaded() succeeds?

If klp_pre_patch_callback() or klp_patch_object() fails during
klp_module_coming(), the successfully initialized state from
klp_init_object_loaded() is leaked.

The cleanup function intentionally skips the currently failing patch:

kernel/livepatch/core.c:klp_cleanup_module_patches_limited() {
	...
	klp_for_each_patch(patch) {
		if (patch == limit)
			break;
	...
}

This prevents its newly applied relocations from being cleared. Since these
relocations modify the livepatch module's memory, they persist even after
the target module's load attempt is aborted.

Upon a subsequent attempt to load the target module,
klp_apply_object_relocs() will attempt to apply the relocations again. On
architectures like x86, apply_relocate_add() verifies that the target
memory is zero; finding it non-zero, it returns -ENOEXEC. This causes
klp_module_coming() to fail, permanently preventing the target module
from being loaded as long as the livepatch is loaded.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830173343.52759-1-x90613@xxxxxxxxx?part=3