Re: [PATCH v4 51/63] objtool/klp: Introduce klp diff subcommand for diffing object files

[Petr Mladek <pmladek@suse.com>]

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/20250115082431.5550-1-pmladek@suse.com

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