[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aOeqt32wQhB5jAD-@pathway.suse.cz>
Date: Thu, 9 Oct 2025 14:29:43 +0200
From: Petr Mladek <pmladek@...e.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
Miroslav Benes <mbenes@...e.cz>,
Joe Lawrence <joe.lawrence@...hat.com>,
live-patching@...r.kernel.org, Song Liu <song@...nel.org>,
laokz <laokz@...mail.com>, Jiri Kosina <jikos@...nel.org>,
Marcos Paulo de Souza <mpdesouza@...e.com>,
Weinan Liu <wnliu@...gle.com>,
Fazla Mehrab <a.mehrab@...edance.com>,
Chen Zhongjin <chenzhongjin@...wei.com>,
Puranjay Mohan <puranjay@...nel.org>,
Dylan Hatch <dylanbhatch@...gle.com>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v4 51/63] objtool/klp: Introduce klp diff subcommand for
diffing object files
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
Powered by blists - more mailing lists