On Wed 2025-10-08 08:27:45, Josh Poimboeuf wrote:
> On Wed, Oct 08, 2025 at 04:01:50PM +0200, Petr Mladek wrote:
> > On Wed 2025-09-17 09:03:59, Josh Poimboeuf wrote:
> > > +static int read_exports(void)
> > > +{
> > > + const char *symvers = "Module.symvers";
> > > + char line[1024], *path = NULL;
> > > + unsigned int line_num = 1;
> > > + FILE *file;
> > > +
> > > + file = fopen(symvers, "r");
> > > + if (!file) {
> > > + path = top_level_dir(symvers);
> > > + if (!path) {
> > > + ERROR("can't open '%s', \"objtool diff\" should be run
> > > from the kernel tree", symvers);
> > > + return -1;
> > > + }
> > > +
> > > + file = fopen(path, "r");
> > > + if (!file) {
> > > + ERROR_GLIBC("fopen");
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > + while (fgets(line, 1024, file)) {
> >
> > Nit: It might be more safe to replace 1024 with sizeof(line).
> > It might be fixed later in a separate patch.
>
> Indeed.
>
> > > +/*
> > > + * Klp relocations aren't allowed for __jump_table and
> > > .static_call_sites if
> > > + * the referenced symbol lives in a kernel module, because such klp
> > > relocs may
> > > + * be applied after static branch/call init, resulting in code
> > > corruption.
> > > + *
> > > + * Validate a special section entry to avoid that. Note that an inert
> > > + * tracepoint is harmless enough, in that case just skip the entry and
> > > print a
> > > + * warning. Otherwise, return an error.
> > > + *
> > > + * This is only a temporary limitation which will be fixed when
> > > livepatch adds
> > > + * support for submodules: fully self-contained modules which are
> > > embedded in
> > > + * the top-level livepatch module's data and which can be loaded on
> > > demand when
> > > + * their corresponding to-be-patched module gets loaded. Then klp
> > > relocs can
> > > + * be retired.
> >
> > I wonder how temporary this is ;-) The comment looks optimistic. I am
> > just curious. Do you have any plans to implement the support for
> > the submodules... ?
>
> I actually already have a working POC for that, but didn't want to make
> the patch set even longer ;-)
Sure.
> It was surprisingly easy and straightforward to implement.
I am curious ;-)
> > PS: To make some expectations. I am not doing a deep review.
> > I am just looking at the patchset to see how far and mature
> > it is. And I just comment what catches my eye.
> >
> > My first impression is that it is already in a pretty good state.
> > And I do not see any big problem there. Well, some documentation
> > would be fine ;-)
> >
> > What are your plans, please?
>
> >From my perspective, it's testing well and in a good enough state for
> merging soon (after the merge window?), if there aren't any objections
> to that.
>
> There will be more patches to come, like the submodules and other arch
> support. And of course there will be bugs discovered by broader
> testing. But I think this is a good foundation to begin with.
>
> And the sooner we can get people using this, the sooner we can start
> deprecating kpatch-build, which would be really nice.
Sounds reasonable and I am fine with it. I have one more question
before I give my ack ;-)
I wonder about the patchset which better integrate callbacks with shadow
variables and state API, see
https://lore.kernel.org/r/[email protected]
I think that it should not be that big problem to update it on top
of this patchset. It would require:
+ moving declarations from livepatch.h to livepatch_external.h
+ updating the macros in livepatch_helpers.h
+ update callback-related code in create_klp_sections()
Or do you expect bigger problems, please?
Best Regards,
Petr
