Re: [RFC PATCH 5/6] livepatch: Remove obsolete per-object callbacks

On Fri, May 15, 2026 at 6:40 AM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] Out-of-bounds ELF relocations causing memory corruption
> - [High] Silent failure of livepatch state callbacks due to missing initialization
> - [Low] Build failures due to unmigrated selftests and samples
> --
>
> commit deb761bd65e9a96a95798282bde72ddb362e1c0d
> Author: Yafang Shao <[email protected]>
>
> livepatch: Remove obsolete per-object callbacks
>
> This commit removes obsolete per-object callbacks from the livepatch framework
> in favor of per-state callbacks. It drops the callback fields from the klp
> object structures and updates objtool and the livepatch core scripts.
>
> > All selftests have been migrated to the new per-state callbacks, making the
> > per-object callbacks redundant.
>
> Is this description accurate?

Selftests are untouched in this RFC. I'll revise the commit log accordingly.

>
> The callbacks field was removed from struct klp_object, but if modules like
> samples/livepatch/livepatch-callbacks-demo.c or the selftests still use it
> in their klp_object declarations, won't this cause build failures?
>
> > diff --git a/scripts/livepatch/init.c b/scripts/livepatch/init.c
> > index 9265e0e504e0c..570366b9268c4 100644
> > --- a/scripts/livepatch/init.c
> > +++ b/scripts/livepatch/init.c
> > @@ -63,8 +63,6 @@ static int __init livepatch_mod_init(void)
> >
> >               obj->name = obj_ext->name;
> >               obj->funcs = funcs;
> > -
> > -             memcpy(&obj->callbacks, &obj_ext->callbacks, sizeof(struct klp_callbacks));
> >       }
> >
> >       patch->mod = THIS_MODULE;
>
> Will the state callbacks be executed when the patch is enabled?
>
> Looking at livepatch_mod_init() after these changes, patch->states is never
> initialized:
>
> scripts/livepatch/init.c:livepatch_mod_init() {
>         ...
>         patch->mod = THIS_MODULE;
>         patch->objs = objs;
>
>         /* TODO patch->states */
>
>         patch->replace_set = KLP_REPLACE_TAG;
>         return klp_enable_patch(patch);
> }
>
> When klp_states_pre_patch() iterates over patch->states, won't it silently
> skip the uninitialized callbacks?

Since these callbacks have been moved to klp_state, they must be
initialized first. Note that the KLP_*_PATCH_CALLBACK macros are
broken after patch #6; I will fix this in the next version.

>
> > diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> > index 4d1f9e9977eb9..6e2762de3d512 100644
> > --- a/tools/objtool/klp-diff.c
> > +++ b/tools/objtool/klp-diff.c
> [ ... ]
> > @@ -1566,8 +1566,8 @@ static int create_klp_sections(struct elfs *e)
> >               reloc = find_reloc_by_dest(e->out, sym->sec, sym->offset);
> >
> >               if (!elf_create_reloc(e->out, obj_sec,
> > -                                   offsetof(struct klp_object_ext, callbacks) +
> > -                                   offsetof(struct klp_callbacks, pre_patch),
> > +                                   offsetof(struct klp_state_ext, callbacks) +
> > +                                   offsetof(struct klp_state_callbacks, pre_patch),
> >                                     reloc->sym, reloc_addend(reloc), R_ABS64))
> >                       return -1;
> >       }
>
> Does this result in out-of-bounds memory writes during ELF relocation?
>
> Earlier in create_klp_sections(), obj_sec is allocated using the size of
> struct klp_object_ext, which is 24 bytes:
>
> tools/objtool/klp-diff.c:create_klp_sections() {
>         size_t obj_size  = sizeof(struct klp_object_ext);
>         ...
>         obj_sec  = elf_create_section_pair(e->out, KLP_OBJECTS_SEC, obj_size, 0, 0);
>
> However, the new relocations are created using offsets from a completely
> different structure, struct klp_state_ext.
>
> Because offsetof(struct klp_state_ext, callbacks) is 16, and the callback
> offsets go up to 24, this applies relocations at offsets 16, 24, 32, and 40
> within obj_sec.
>
> Won't the relocation at offset 16 corrupt klp_object_ext.nr_funcs, and the
> subsequent relocations write entirely out of bounds of the 24-byte obj_sec
> buffer?

Nice catch. I will fix it in the next version.

-- 
Regards
Yafang