Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
Hi Joe, On Tue, Aug 20, 2019 at 1:02 AM Joe Lawrence wrote: > > On 8/19/19 3:31 AM, Miroslav Benes wrote: > > On Mon, 19 Aug 2019, Masahiro Yamada wrote: > > > >> > >> I can review this series from the build system point of view, > >> but I am not familiar enough with live-patching itself. > >> > >> Some possibilities: > >> > >> [1] Merge this series thru the live-patch tree after the > >> kbuild base patches land. > >> This requires one extra development cycle (targeting for 5.5-rc1) > >> but I think this is the official way if you do not rush into it. > > > > I'd prefer this option. There is no real rush and I think we can wait one > > extra development cycle. > > Agreed. I'm in no hurry and was only curious about the kbuild changes > that this patchset is now dependent on -- how to note them for other > reviewers or anyone wishing to test. > > > Joe, could you submit one more revision with all the recent changes (once > > kbuild improvements settle down), please? We should take a look at the > > whole thing one more time? What do you think? > > > > Definitely, yes. I occasionally force a push to: > https://github.com/joe-lawrence/linux/tree/klp-convert-v5-expanded > > as I've been updating and collecting feedback from v4. Once updates > settle, I'll send out a new v5 set. > > -- Joe When you send v5, please squash the following clean-up too: diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal index 89eaef0d3efc..9e77246d84e3 100644 --- a/scripts/Makefile.modfinal +++ b/scripts/Makefile.modfinal @@ -47,7 +47,7 @@ targets += $(modules) $(modules:.ko=.mod.o) # Live Patch # --- -$(modules-klp:.ko=.tmp.ko): %.tmp.ko: %.o %.mod.o Symbols.list FORCE +%.tmp.ko: %.o %.mod.o Symbols.list FORCE +$(call if_changed,ld_ko_o) quiet_cmd_klp_convert = KLP $@ Thanks. -- Best Regards Masahiro Yamada
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
> > How is this feature supposed to work for external modules? > > > > klp-convert receives: > > "symbols from vmlinux" + "symbols from no-klp in-tree modules" > > + "symbols from no-klp external modules" ?? > > > > I don't think that this use-case has been previously thought out (Miroslav, > correct me if I'm wrong here.) > > I did just run an external build of a copy of > samples/livepatch/livepatch-annotated-sample.c: > > - modules.livepatch is generated in external dir > - klp-convert is invoked for the livepatch module > - the external livepatch module successfully loads > > But that was only testing external livepatch modules. > > I don't know if we need/want to support general external modules supplementing > Symbols.list, at least for the initial klp-convert commit. I suppose external > livepatch modules would then need to specify additional Symbols.list(s) files > somehow as well. I think we discussed it briefly and decided to postpone it for later improvements. External modules are not so important in my opinion. > > > > BTW, 'Symbols.list' sounds like a file to list out symbols > > for generic purposes, but in fact, the > > file format is very specific for the klp-convert tool. > > Perhaps, is it better to rename it so it infers > > this is for livepatching? What do you think? > > > > I don't know if the "Symbols.list" name and leading uppercase was based on any > convention, but something like symbols.klp would be fine with me. symbols.klp looks ok Miroslav
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On 8/19/19 3:31 AM, Miroslav Benes wrote: On Mon, 19 Aug 2019, Masahiro Yamada wrote: I can review this series from the build system point of view, but I am not familiar enough with live-patching itself. Some possibilities: [1] Merge this series thru the live-patch tree after the kbuild base patches land. This requires one extra development cycle (targeting for 5.5-rc1) but I think this is the official way if you do not rush into it. I'd prefer this option. There is no real rush and I think we can wait one extra development cycle. Agreed. I'm in no hurry and was only curious about the kbuild changes that this patchset is now dependent on -- how to note them for other reviewers or anyone wishing to test. Joe, could you submit one more revision with all the recent changes (once kbuild improvements settle down), please? We should take a look at the whole thing one more time? What do you think? Definitely, yes. I occasionally force a push to: https://github.com/joe-lawrence/linux/tree/klp-convert-v5-expanded as I've been updating and collecting feedback from v4. Once updates settle, I'll send out a new v5 set. -- Joe
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On 8/18/19 11:50 PM, Masahiro Yamada wrote: Hi Joe, On Sat, Aug 17, 2019 at 4:01 AM Joe Lawrence wrote: I didn't realize that we're supposed to be able to still build external modules after "make clean". If that's the case, then one might want to build an external klp-module after doing that. Yes. 'make clean' must keep all the build artifacts needed for building external modules. With that in mind, shouldn't Symbols.list to persist until mrproper? And I think modules-livepatch could go away during clean, what do you think? -- Joe Symbols.list should be kept by the time mrproper is run. So, please add it to MRROPER_FILES instead of CLEAN_FILES. modules.livepatch is a temporary file, so you can add it to CLEAN_FILES. OK, I'll add those to their respective lists. How is this feature supposed to work for external modules? klp-convert receives: "symbols from vmlinux" + "symbols from no-klp in-tree modules" + "symbols from no-klp external modules" ?? I don't think that this use-case has been previously thought out (Miroslav, correct me if I'm wrong here.) I did just run an external build of a copy of samples/livepatch/livepatch-annotated-sample.c: - modules.livepatch is generated in external dir - klp-convert is invoked for the livepatch module - the external livepatch module successfully loads But that was only testing external livepatch modules. I don't know if we need/want to support general external modules supplementing Symbols.list, at least for the initial klp-convert commit. I suppose external livepatch modules would then need to specify additional Symbols.list(s) files somehow as well. BTW, 'Symbols.list' sounds like a file to list out symbols for generic purposes, but in fact, the file format is very specific for the klp-convert tool. Perhaps, is it better to rename it so it infers this is for livepatching? What do you think? I don't know if the "Symbols.list" name and leading uppercase was based on any convention, but something like symbols.klp would be fine with me. Thanks, -- Joe
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On Mon, 19 Aug 2019, Masahiro Yamada wrote: > On Fri, Aug 16, 2019 at 9:43 PM Joe Lawrence wrote: > > > > On 8/16/19 4:19 AM, Miroslav Benes wrote: > > > Hi, > > > > > >> I cleaned up the build system, and pushed it based on my > > >> kbuild tree. > > >> > > >> Please see: > > >> > > >> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git > > >> klp-cleanup > > > > > > This indeed looks much simpler and cleaner (as far as I can judge with my > > > limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch, > > > "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and > > > work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy > > > module which is then livepatched by lib/livepatch/test_klp_convert1.c). > > > > > > > Yeah, Masahiro this is great, thanks for reworking this! > > > > I did tweak one module like Miroslav mentioned and I think a few of the > > newly generated files need to be cleaned up as part of "make clean", but > > all said, this is a nice improvement. > > > > Are you targeting the next merge window for your kbuild branch? (This > > appears to be what the klp-cleanup branch was based on.) > > > I can review this series from the build system point of view, > but I am not familiar enough with live-patching itself. > > Some possibilities: > > [1] Merge this series thru the live-patch tree after the > kbuild base patches land. > This requires one extra development cycle (targeting for 5.5-rc1) > but I think this is the official way if you do not rush into it. I'd prefer this option. There is no real rush and I think we can wait one extra development cycle. Joe, could you submit one more revision with all the recent changes (once kbuild improvements settle down), please? We should take a look at the whole thing one more time? What do you think? > [2] Create an immutable branch in kbuild tree, and the live-patch > tree will use it as the basis for queuing this series. > We will have to coordinate the pull request order, but > we can merge this feature for 5.4-rc1 > > [3] Apply this series to my kbuild tree, with proper Acked-by > from the livepatch maintainers. > I am a little bit uncomfortable with applying patches I > do not understand, though... Thanks Miroslav
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
Hi Joe, On Sat, Aug 17, 2019 at 4:01 AM Joe Lawrence wrote: > > On 8/16/19 8:43 AM, Joe Lawrence wrote: > > On 8/16/19 4:19 AM, Miroslav Benes wrote: > >> Hi, > >> > >>> I cleaned up the build system, and pushed it based on my > >>> kbuild tree. > >>> > >>> Please see: > >>> > >>> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git > >>> klp-cleanup > >> > >> This indeed looks much simpler and cleaner (as far as I can judge with my > >> limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch, > >> "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and > >> work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy > >> module which is then livepatched by lib/livepatch/test_klp_convert1.c). > >> > > > > Yeah, Masahiro this is great, thanks for reworking this! > > > > I did tweak one module like Miroslav mentioned and I think a few of the > > newly generated files need to be cleaned up as part of "make clean", but > > all said, this is a nice improvement. > > > > Well actually, now I see this comment in the top-level Makefile: > > # Cleaning is done on three levels. > > # make clean Delete most generated files > > #Leave enough to build external modules > > # make mrproper Delete the current configuration, and all generated > files > # make distclean Remove editor backup files, patch leftover files and > the like > > I didn't realize that we're supposed to be able to still build external > modules after "make clean". If that's the case, then one might want to > build an external klp-module after doing that. Yes. 'make clean' must keep all the build artifacts needed for building external modules. > With that in mind, shouldn't Symbols.list to persist until mrproper? > And I think modules-livepatch could go away during clean, what do you think? > > -- Joe Symbols.list should be kept by the time mrproper is run. So, please add it to MRROPER_FILES instead of CLEAN_FILES. modules.livepatch is a temporary file, so you can add it to CLEAN_FILES. How is this feature supposed to work for external modules? klp-convert receives: "symbols from vmlinux" + "symbols from no-klp in-tree modules" + "symbols from no-klp external modules" ?? BTW, 'Symbols.list' sounds like a file to list out symbols for generic purposes, but in fact, the file format is very specific for the klp-convert tool. Perhaps, is it better to rename it so it infers this is for livepatching? What do you think? -- Best Regards Masahiro Yamada
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On Fri, Aug 16, 2019 at 9:43 PM Joe Lawrence wrote: > > On 8/16/19 4:19 AM, Miroslav Benes wrote: > > Hi, > > > >> I cleaned up the build system, and pushed it based on my > >> kbuild tree. > >> > >> Please see: > >> > >> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git > >> klp-cleanup > > > > This indeed looks much simpler and cleaner (as far as I can judge with my > > limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch, > > "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and > > work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy > > module which is then livepatched by lib/livepatch/test_klp_convert1.c). > > > > Yeah, Masahiro this is great, thanks for reworking this! > > I did tweak one module like Miroslav mentioned and I think a few of the > newly generated files need to be cleaned up as part of "make clean", but > all said, this is a nice improvement. > > Are you targeting the next merge window for your kbuild branch? (This > appears to be what the klp-cleanup branch was based on.) I can review this series from the build system point of view, but I am not familiar enough with live-patching itself. Some possibilities: [1] Merge this series thru the live-patch tree after the kbuild base patches land. This requires one extra development cycle (targeting for 5.5-rc1) but I think this is the official way if you do not rush into it. [2] Create an immutable branch in kbuild tree, and the live-patch tree will use it as the basis for queuing this series. We will have to coordinate the pull request order, but we can merge this feature for 5.4-rc1 [3] Apply this series to my kbuild tree, with proper Acked-by from the livepatch maintainers. I am a little bit uncomfortable with applying patches I do not understand, though... -- Best Regards Masahiro Yamada
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On 8/16/19 8:43 AM, Joe Lawrence wrote: On 8/16/19 4:19 AM, Miroslav Benes wrote: Hi, I cleaned up the build system, and pushed it based on my kbuild tree. Please see: git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git klp-cleanup This indeed looks much simpler and cleaner (as far as I can judge with my limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch, "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy module which is then livepatched by lib/livepatch/test_klp_convert1.c). Yeah, Masahiro this is great, thanks for reworking this! I did tweak one module like Miroslav mentioned and I think a few of the newly generated files need to be cleaned up as part of "make clean", but all said, this is a nice improvement. Well actually, now I see this comment in the top-level Makefile: # Cleaning is done on three levels. # make clean Delete most generated files #Leave enough to build external modules # make mrproper Delete the current configuration, and all generated files # make distclean Remove editor backup files, patch leftover files and the like I didn't realize that we're supposed to be able to still build external modules after "make clean". If that's the case, then one might want to build an external klp-module after doing that. With that in mind, shouldn't Symbols.list to persist until mrproper? And I think modules-livepatch could go away during clean, what do you think? -- Joe
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On 8/16/19 4:19 AM, Miroslav Benes wrote: Hi, I cleaned up the build system, and pushed it based on my kbuild tree. Please see: git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git klp-cleanup This indeed looks much simpler and cleaner (as far as I can judge with my limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch, "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy module which is then livepatched by lib/livepatch/test_klp_convert1.c). Yeah, Masahiro this is great, thanks for reworking this! I did tweak one module like Miroslav mentioned and I think a few of the newly generated files need to be cleaned up as part of "make clean", but all said, this is a nice improvement. Are you targeting the next merge window for your kbuild branch? (This appears to be what the klp-cleanup branch was based on.) Thanks, -- Joe
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
Hi, > I cleaned up the build system, and pushed it based on my > kbuild tree. > > Please see: > > git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git > klp-cleanup This indeed looks much simpler and cleaner (as far as I can judge with my limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch, "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy module which is then livepatched by lib/livepatch/test_klp_convert1.c). Thanks a lot! Miroslav
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
Hi Joe, On Tue, Aug 13, 2019 at 12:56 AM Joe Lawrence wrote: > > On Wed, Jul 31, 2019 at 02:58:27PM +0900, Masahiro Yamada wrote: > > Hi Joe, > > > > > > On Thu, May 9, 2019 at 11:39 PM Joe Lawrence > > wrote: > > > > > > From: Miroslav Benes > > > > > > Currently, livepatch infrastructure in the kernel relies on > > > MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the > > > kernel module loader knows a module is indeed livepatch module and can > > > behave accordingly. > > > > > > klp-convert, on the other hand relies on LIVEPATCH_* statement in the > > > module's Makefile for exactly the same reason. > > > > > > Remove dependency on modinfo and generate MODULE_INFO flag > > > automatically in modpost when LIVEPATCH_* is defined in the module's > > > Makefile. Generate a list of all built livepatch modules based on > > > the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give > > > this list as an argument for modpost which will use it to identify > > > livepatch modules. > > > > > > As MODULE_INFO is no longer needed, remove it. > > > > > > I do not understand this patch. > > This makes the implementation so complicated. > > > > I think MODULE_INFO(livepatch, "Y") is cleaner than > > LIVEPATCH_* in Makefile. > > > > > > How about this approach? > > > > > > [1] Make modpost generate the list of livepatch modules. > > (livepatch-modules) > > > > [2] Generate Symbols.list in scripts/Makefile.modpost > > (vmlinux + modules excluding livepatch-modules) > > > > [3] Run klp-convert for modules in livepatch-modules. > > > > > > If you do this, you can remove most of the build system hacks > > can't you? > > > > > > I attached an example implementation for [1]. > > > > Please check whether this works. > > > > Hi Masahiro, > > I tested and step [1] that you attached did create the livepatch-modules > as expected. Thanks for that example, it does look cleaner that what > we had in the patchset. > > I'm admittedly out of my element with kbuild changes, but here are my > naive attempts at steps [2] and [3]... > > > [step 2] generate Symbols.list - I tacked this on as a dependency of the > $(modules:.ko=.mod.o), but there is probably a better more logical place > to put it. Also used grep -Fxv to exclude the livepatch-modules list > from the modules.order list of modules to process. > > -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- > > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost > index 3eca7fccadd4..5409bbc212bb 100644 > --- a/scripts/Makefile.modpost > +++ b/scripts/Makefile.modpost > @@ -111,7 +111,23 @@ quiet_cmd_cc_o_c = CC $@ >cmd_cc_o_c = $(CC) $(c_flags) $(KBUILD_CFLAGS_MODULE) $(CFLAGS_MODULE) > \ >-c -o $@ $< > > -$(modules:.ko=.mod.o): %.mod.o: %.mod.c FORCE > +quiet_cmd_klp_map = KLP Symbols.list > +SLIST = $(objtree)/Symbols.list > + > +define cmd_symbols_list > + $(shell echo "klp-convert-symbol-data.0.1" > $(objtree)/Symbols.list) > \ > + $(shell echo "*vmlinux" >> $(objtree)/Symbols.list) > \ > + $(shell nm -f posix $(objtree)/vmlinux | cut -d\ -f1 >> > $(objtree)/Symbols.list) \ > + $(foreach ko, $(sort $(shell grep -Fxv -f livepatch-modules > modules.order)),\ > + $(shell echo "*$(shell basename -s .ko $(ko))" >> > $(objtree)/Symbols.list) \ > + $(shell nm -f posix $(patsubst %.ko,%.o,$(ko)) | cut -d\ -f1 > >> $(objtree)/Symbols.list)) > +endef All the $(shell ...) calls are pointless. $(shell echo "hello" > Symbols.list) is equivalent to echo "hello" > Symbols.list > + > +Symbols.list: __modpost > + $(if $(CONFIG_LIVEPATCH), $(call cmd,symbols_list)) > + > + > +$(modules:.ko=.mod.o): %.mod.o: %.mod.c Symbols.list FORCE > $(call if_changed_dep,cc_o_c) > > targets += $(modules:.ko=.mod.o) > -- > 2.18.1 > > -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- > > > > [step 3] klp-convert the livepatch-modules - more or less what existed > in the patchset already, however used the grep -Fx trick to process only > modules found in livepatch-modules file: > > -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- > > diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include > index 73e80b917f12..f085644c2b97 100644 > --- a/scripts/Kbuild.include > +++ b/scripts/Kbuild.include > @@ -223,6 +223,8 @@ endif > # (needed for the shell) > make-cmd = $(call escsq,$(subst $(pound),$$(pound),$(subst > $$,,$(cmd_$(1) > > +save-cmd = printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd > + > # Find any prerequisites that is newer than target or that does not exist. > # PHONY targets skipped in both cases. > any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard > $^),$^) > @@ -230,7 +232,7 @@ any-prereq = $(filter-out $(PHONY),$?)$(filter-out >
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On Wed, 31 Jul 2019, Masahiro Yamada wrote: > Hi Joe, > > > On Thu, May 9, 2019 at 11:39 PM Joe Lawrence wrote: > > > > From: Miroslav Benes > > > > Currently, livepatch infrastructure in the kernel relies on > > MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the > > kernel module loader knows a module is indeed livepatch module and can > > behave accordingly. > > > > klp-convert, on the other hand relies on LIVEPATCH_* statement in the > > module's Makefile for exactly the same reason. > > > > Remove dependency on modinfo and generate MODULE_INFO flag > > automatically in modpost when LIVEPATCH_* is defined in the module's > > Makefile. Generate a list of all built livepatch modules based on > > the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give > > this list as an argument for modpost which will use it to identify > > livepatch modules. > > > > As MODULE_INFO is no longer needed, remove it. > > > I do not understand this patch. > This makes the implementation so complicated. > > I think MODULE_INFO(livepatch, "Y") is cleaner than > LIVEPATCH_* in Makefile. > > > How about this approach? > > > [1] Make modpost generate the list of livepatch modules. > (livepatch-modules) > > [2] Generate Symbols.list in scripts/Makefile.modpost > (vmlinux + modules excluding livepatch-modules) > > [3] Run klp-convert for modules in livepatch-modules. > > > If you do this, you can remove most of the build system hacks > can't you? > > > I attached an example implementation for [1]. > > Please check whether this works. Yes, it sounds like a better approach. I've never liked LIVEPATCH_* in Makefile much, so I'm all for dropping it. Thanks Miroslav
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On Wed, Jul 31, 2019 at 02:58:27PM +0900, Masahiro Yamada wrote: > Hi Joe, > > > On Thu, May 9, 2019 at 11:39 PM Joe Lawrence wrote: > > > > From: Miroslav Benes > > > > Currently, livepatch infrastructure in the kernel relies on > > MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the > > kernel module loader knows a module is indeed livepatch module and can > > behave accordingly. > > > > klp-convert, on the other hand relies on LIVEPATCH_* statement in the > > module's Makefile for exactly the same reason. > > > > Remove dependency on modinfo and generate MODULE_INFO flag > > automatically in modpost when LIVEPATCH_* is defined in the module's > > Makefile. Generate a list of all built livepatch modules based on > > the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give > > this list as an argument for modpost which will use it to identify > > livepatch modules. > > > > As MODULE_INFO is no longer needed, remove it. > > > I do not understand this patch. > This makes the implementation so complicated. > > I think MODULE_INFO(livepatch, "Y") is cleaner than > LIVEPATCH_* in Makefile. > > > How about this approach? > > > [1] Make modpost generate the list of livepatch modules. > (livepatch-modules) > > [2] Generate Symbols.list in scripts/Makefile.modpost > (vmlinux + modules excluding livepatch-modules) > > [3] Run klp-convert for modules in livepatch-modules. > > > If you do this, you can remove most of the build system hacks > can't you? > > > I attached an example implementation for [1]. > > Please check whether this works. > Hi Masahiro, I tested and step [1] that you attached did create the livepatch-modules as expected. Thanks for that example, it does look cleaner that what we had in the patchset. I'm admittedly out of my element with kbuild changes, but here are my naive attempts at steps [2] and [3]... [step 2] generate Symbols.list - I tacked this on as a dependency of the $(modules:.ko=.mod.o), but there is probably a better more logical place to put it. Also used grep -Fxv to exclude the livepatch-modules list from the modules.order list of modules to process. -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index 3eca7fccadd4..5409bbc212bb 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -111,7 +111,23 @@ quiet_cmd_cc_o_c = CC $@ cmd_cc_o_c = $(CC) $(c_flags) $(KBUILD_CFLAGS_MODULE) $(CFLAGS_MODULE) \ -c -o $@ $< -$(modules:.ko=.mod.o): %.mod.o: %.mod.c FORCE +quiet_cmd_klp_map = KLP Symbols.list +SLIST = $(objtree)/Symbols.list + +define cmd_symbols_list + $(shell echo "klp-convert-symbol-data.0.1" > $(objtree)/Symbols.list) \ + $(shell echo "*vmlinux" >> $(objtree)/Symbols.list) \ + $(shell nm -f posix $(objtree)/vmlinux | cut -d\ -f1 >> $(objtree)/Symbols.list) \ + $(foreach ko, $(sort $(shell grep -Fxv -f livepatch-modules modules.order)),\ + $(shell echo "*$(shell basename -s .ko $(ko))" >> $(objtree)/Symbols.list) \ + $(shell nm -f posix $(patsubst %.ko,%.o,$(ko)) | cut -d\ -f1 >> $(objtree)/Symbols.list)) +endef + +Symbols.list: __modpost + $(if $(CONFIG_LIVEPATCH), $(call cmd,symbols_list)) + + +$(modules:.ko=.mod.o): %.mod.o: %.mod.c Symbols.list FORCE $(call if_changed_dep,cc_o_c) targets += $(modules:.ko=.mod.o) -- 2.18.1 -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- [step 3] klp-convert the livepatch-modules - more or less what existed in the patchset already, however used the grep -Fx trick to process only modules found in livepatch-modules file: -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index 73e80b917f12..f085644c2b97 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -223,6 +223,8 @@ endif # (needed for the shell) make-cmd = $(call escsq,$(subst $(pound),$$(pound),$(subst $$,,$(cmd_$(1) +save-cmd = printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd + # Find any prerequisites that is newer than target or that does not exist. # PHONY targets skipped in both cases. any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard $^),$^) @@ -230,7 +232,7 @@ any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard $^),$^) # Execute command if command has changed or prerequisite(s) are updated. if_changed = $(if $(any-prereq)$(cmd-check), \ $(cmd); \ - printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:) + $(save-cmd), @:) # Execute the command and also postprocess generated .d dependencies file. if_changed_dep = $(if
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
Hi Joe,
On Thu, May 9, 2019 at 11:39 PM Joe Lawrence wrote:
>
> From: Miroslav Benes
>
> Currently, livepatch infrastructure in the kernel relies on
> MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the
> kernel module loader knows a module is indeed livepatch module and can
> behave accordingly.
>
> klp-convert, on the other hand relies on LIVEPATCH_* statement in the
> module's Makefile for exactly the same reason.
>
> Remove dependency on modinfo and generate MODULE_INFO flag
> automatically in modpost when LIVEPATCH_* is defined in the module's
> Makefile. Generate a list of all built livepatch modules based on
> the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give
> this list as an argument for modpost which will use it to identify
> livepatch modules.
>
> As MODULE_INFO is no longer needed, remove it.
I do not understand this patch.
This makes the implementation so complicated.
I think MODULE_INFO(livepatch, "Y") is cleaner than
LIVEPATCH_* in Makefile.
How about this approach?
[1] Make modpost generate the list of livepatch modules.
(livepatch-modules)
[2] Generate Symbols.list in scripts/Makefile.modpost
(vmlinux + modules excluding livepatch-modules)
[3] Run klp-convert for modules in livepatch-modules.
If you do this, you can remove most of the build system hacks
can't you?
I attached an example implementation for [1].
Please check whether this works.
Thanks.
--
Best Regards
Masahiro Yamada
From 85571430aa12cd19a75cbc856da1092199876e6a Mon Sep 17 00:00:00 2001
From: Masahiro Yamada
Date: Wed, 31 Jul 2019 14:51:29 +0900
Subject: [PATCH] livepatch: make modpost generate the list of livepatch
modules
Reverse the livepatch-modules direction.
The modpost generates the livepatch-modules file instead of
Makefile feeding it to modpost.
The implementation just mimics write_dump().
Signed-off-by: Masahiro Yamada
---
scripts/Makefile.modpost | 3 ++-
scripts/mod/modpost.c| 28 ++--
scripts/mod/modpost.h| 1 +
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 92ed02d7cd5e..c884b7b709ca 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -56,7 +56,8 @@ MODPOST = scripts/mod/modpost \
$(if $(KBUILD_EXTMOD),$(addprefix -e ,$(KBUILD_EXTRA_SYMBOLS))) \
$(if $(KBUILD_EXTMOD),-o $(modulesymfile)) \
$(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E) \
- $(if $(KBUILD_MODPOST_WARN),-w)
+ $(if $(KBUILD_MODPOST_WARN),-w) \
+ $(if $(CONFIG_LIVEPATCH),-l livepatch-modules)
ifdef MODPOST_VMLINUX
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 3e6d36ddfcdf..e3f637f225e4 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1976,6 +1976,10 @@ static void read_symbols(const char *modname)
license = get_next_modinfo(, "license", license);
}
+ /* Livepatch modules have unresolved symbols resolved by klp-convert */
+ if (get_modinfo(, "livepatch"))
+ mod->livepatch = 1;
+
for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
symname = remove_dot(info.strtab + sym->st_name);
@@ -2118,7 +2122,7 @@ static int check_exports(struct module *mod)
const char *basename;
exp = find_symbol(s->name);
if (!exp || exp->module == mod) {
- if (have_vmlinux && !s->weak) {
+ if (have_vmlinux && !s->weak && !mod->livepatch) {
if (warn_unresolved) {
warn("\"%s\" [%s.ko] undefined!\n",
s->name, mod->name);
@@ -2429,6 +2433,20 @@ static void write_dump(const char *fname)
free(buf.p);
}
+static void write_livepatch_modules(const char *fname)
+{
+ struct buffer buf = { };
+ struct module *mod;
+
+ for (mod = modules; mod; mod = mod->next) {
+ if (mod->livepatch)
+ buf_printf(, "%s\n", mod->name);
+ }
+
+ write_if_changed(, fname);
+ free(buf.p);
+}
+
struct ext_sym_list {
struct ext_sym_list *next;
const char *file;
@@ -2440,13 +2458,14 @@ int main(int argc, char **argv)
struct buffer buf = { };
char *kernel_read = NULL, *module_read = NULL;
char *dump_write = NULL, *files_source = NULL;
+ char *livepatch_modules = NULL;
int opt;
int err;
int n;
struct ext_sym_list *extsym_iter;
struct ext_sym_list *extsym_start = NULL;
- while ((opt = getopt(argc, argv, "i:I:e:mnsT:o:awE")) != -1) {
+ while ((opt = getopt(argc, argv, "i:I:e:l:mnsT:o:awE")) != -1) {
switch (opt) {
case 'i':
kernel_read = optarg;
@@ -2463,6 +2482,9 @@ int main(int argc, char **argv)
extsym_iter->file = optarg;
extsym_start = extsym_iter;
break;
+ case 'l':
+ livepatch_modules = optarg;
+ break;
case 'm':
modversions = 1;
break;
@@ -2535,6 +2557,8 @@ int main(int argc, char **argv)
}
if (dump_write)
write_dump(dump_write);
+ if (livepatch_modules)
+ write_livepatch_modules(livepatch_modules);
if (sec_mismatch_count && sec_mismatch_fatal)
fatal("modpost: Section mismatches detected.\n"
"Set
