| Message ID | 20220310150834.98815-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | livepatch: further fixes | expand |
On 10.03.22 16:08, Roger Pau Monne wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > The current handling of altinstructions sections by the livepatch > tools is incorrect, as on Xen those sections are part of .init and > thus discarded after load. Correctly handle them by just ignoring, as > it's done with other .init related sections. > > While there also add .data.ro_after_init section as a read-only > section and introduce some syntactic sugar for comparing section > names. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > I assume this handling of .altinstr* sections was inherited from Linux > where the sections are not discarded after load in order to apply > alternative patching to the loaded modules after boot. > --- > common.c | 7 +++++-- > create-diff-object.c | 26 -------------------------- > 2 files changed, 5 insertions(+), 28 deletions(-) > > diff --git a/common.c b/common.c > index 68a71f7..a148d8a 100644 > --- a/common.c > +++ b/common.c > @@ -249,19 +249,22 @@ int is_text_section(struct section *sec) > (sec->sh.sh_flags & SHF_EXECINSTR)); > } > > +#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1) > int is_rodata_section(struct section *sec) > { > return sec->sh.sh_type == SHT_PROGBITS && > !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) && > - !strncmp(sec->name, ".rodata", 7); > + (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init")); > } > > int is_init_section(struct section *sec) > { > return sec->sh.sh_type == SHT_PROGBITS && > (sec->sh.sh_flags & SHF_ALLOC) && > - !strncmp(sec->name, ".init", 5); > + (SEC_MATCH(".init") || SEC_MATCH(".text.startup") || > + SEC_MATCH(".altinstr") || SEC_MATCH(".ctors")); > } > +#undef SEC_MATCH > > int is_debug_section(struct section *sec) > { > diff --git a/create-diff-object.c b/create-diff-object.c > index a516670..ec2afb4 100644 > --- a/create-diff-object.c > +++ b/create-diff-object.c > @@ -995,19 +995,6 @@ static int ex_table_group_size(struct kpatch_elf *kelf, int offset) > return size; > } > > -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) > -{ > - static int size = 0; > - char *str; > - if (!size) { > - str = getenv("ALT_STRUCT_SIZE"); > - size = str ? atoi(str) : 12; > - } > - > - log_debug("altinstr_size=%d\n", size); > - return size; > -} > - > static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset) > { > static int size = 0; > @@ -1021,11 +1008,6 @@ static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset) > return size; > } > > -static int undefined_group_size(struct kpatch_elf *kelf, int offset) > -{ > - return 0; > -} > - > /* > * The rela groups in the .fixup section vary in size. The beginning of each > * .fixup rela group is referenced by the .ex_table section. To find the size > @@ -1099,14 +1081,6 @@ static struct special_section special_sections[] = { > .name = ".ex_table", > .group_size = ex_table_group_size, > }, > - { > - .name = ".altinstructions", > - .group_size = altinstructions_group_size, > - }, > - { > - .name = ".altinstr_replacement", > - .group_size = undefined_group_size, > - }, > { > .name = ".livepatch.hooks.load", > .group_size = livepatch_hooks_group_size, > -- > 2.34.1 > Confirming, this solves the altsection issue I reported via https://lore.kernel.org/xen-devel/b74a68b038c31df4bb94a5b5e87453f5a249cfe2.1646753657.git.doebel@amazon.de/ Reviewed-by: Bjoern Doebel <doebel@amazon.de> Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On 10.03.2022 16:08, Roger Pau Monne wrote: > --- a/common.c > +++ b/common.c > @@ -249,19 +249,22 @@ int is_text_section(struct section *sec) > (sec->sh.sh_flags & SHF_EXECINSTR)); > } > > +#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1) > int is_rodata_section(struct section *sec) > { > return sec->sh.sh_type == SHT_PROGBITS && > !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) && > - !strncmp(sec->name, ".rodata", 7); > + (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init")); > } > > int is_init_section(struct section *sec) > { > return sec->sh.sh_type == SHT_PROGBITS && > (sec->sh.sh_flags & SHF_ALLOC) && > - !strncmp(sec->name, ".init", 5); > + (SEC_MATCH(".init") || SEC_MATCH(".text.startup") || > + SEC_MATCH(".altinstr") || SEC_MATCH(".ctors")); Having dealt with this recently - what about .init_array? Modern gcc prefers that over .ctors. Of course the question is whether either really needs dealing with here - these sections, to my knowledge, appear only with gcov support enabled. Not sure that's a case where livepatching is actually expected to be used. Jan
On Fri, Mar 11, 2022 at 08:35:18AM +0100, Jan Beulich wrote: > On 10.03.2022 16:08, Roger Pau Monne wrote: > > --- a/common.c > > +++ b/common.c > > @@ -249,19 +249,22 @@ int is_text_section(struct section *sec) > > (sec->sh.sh_flags & SHF_EXECINSTR)); > > } > > > > +#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1) > > int is_rodata_section(struct section *sec) > > { > > return sec->sh.sh_type == SHT_PROGBITS && > > !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) && > > - !strncmp(sec->name, ".rodata", 7); > > + (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init")); > > } > > > > int is_init_section(struct section *sec) > > { > > return sec->sh.sh_type == SHT_PROGBITS && > > (sec->sh.sh_flags & SHF_ALLOC) && > > - !strncmp(sec->name, ".init", 5); > > + (SEC_MATCH(".init") || SEC_MATCH(".text.startup") || > > + SEC_MATCH(".altinstr") || SEC_MATCH(".ctors")); > > Having dealt with this recently - what about .init_array? Modern gcc > prefers that over .ctors. Of course the question is whether either > really needs dealing with here - these sections, to my knowledge, > appear only with gcov support enabled. Not sure that's a case where > livepatching is actually expected to be used. .init_array will match the .init comparison, and thus is already handled. Regarding .ctors, it's certainly an .init section, so it doesn't hurt to get added here in any case? (regardless of us only knowing it being used for code coverage so far) Thanks, Roger.
On 11.03.2022 09:12, Roger Pau Monné wrote: > On Fri, Mar 11, 2022 at 08:35:18AM +0100, Jan Beulich wrote: >> On 10.03.2022 16:08, Roger Pau Monne wrote: >>> --- a/common.c >>> +++ b/common.c >>> @@ -249,19 +249,22 @@ int is_text_section(struct section *sec) >>> (sec->sh.sh_flags & SHF_EXECINSTR)); >>> } >>> >>> +#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1) >>> int is_rodata_section(struct section *sec) >>> { >>> return sec->sh.sh_type == SHT_PROGBITS && >>> !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) && >>> - !strncmp(sec->name, ".rodata", 7); >>> + (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init")); >>> } >>> >>> int is_init_section(struct section *sec) >>> { >>> return sec->sh.sh_type == SHT_PROGBITS && >>> (sec->sh.sh_flags & SHF_ALLOC) && >>> - !strncmp(sec->name, ".init", 5); >>> + (SEC_MATCH(".init") || SEC_MATCH(".text.startup") || >>> + SEC_MATCH(".altinstr") || SEC_MATCH(".ctors")); >> >> Having dealt with this recently - what about .init_array? Modern gcc >> prefers that over .ctors. Of course the question is whether either >> really needs dealing with here - these sections, to my knowledge, >> appear only with gcov support enabled. Not sure that's a case where >> livepatching is actually expected to be used. > > .init_array will match the .init comparison, and thus is already > handled. Oh, I guess I should have looked at what SEC_MATCH() actually does. > Regarding .ctors, it's certainly an .init section, so it doesn't hurt > to get added here in any case? (regardless of us only knowing it being > used for code coverage so far) It certainly doesn't hurt, sure. Jan
On Thu, Mar 10, 2022 at 04:08:34PM +0100, Roger Pau Monne wrote: > The current handling of altinstructions sections by the livepatch > tools is incorrect, as on Xen those sections are part of .init and > thus discarded after load. Correctly handle them by just ignoring, as > it's done with other .init related sections. I think my logic here is wrong. While looking at something else I realized that livepatch does support '.altinstructions', as it needs to be able to patch the contents of the payload that is being loaded - hence ignoring any '.altintr*' section at patch generation is not OK. I have to withdraw this patch, will likely repost with the other sections that do need to be ignored. Thanks, Roger.
diff --git a/common.c b/common.c index 68a71f7..a148d8a 100644 --- a/common.c +++ b/common.c @@ -249,19 +249,22 @@ int is_text_section(struct section *sec) (sec->sh.sh_flags & SHF_EXECINSTR)); } +#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1) int is_rodata_section(struct section *sec) { return sec->sh.sh_type == SHT_PROGBITS && !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) && - !strncmp(sec->name, ".rodata", 7); + (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init")); } int is_init_section(struct section *sec) { return sec->sh.sh_type == SHT_PROGBITS && (sec->sh.sh_flags & SHF_ALLOC) && - !strncmp(sec->name, ".init", 5); + (SEC_MATCH(".init") || SEC_MATCH(".text.startup") || + SEC_MATCH(".altinstr") || SEC_MATCH(".ctors")); } +#undef SEC_MATCH int is_debug_section(struct section *sec) { diff --git a/create-diff-object.c b/create-diff-object.c index a516670..ec2afb4 100644 --- a/create-diff-object.c +++ b/create-diff-object.c @@ -995,19 +995,6 @@ static int ex_table_group_size(struct kpatch_elf *kelf, int offset) return size; } -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) -{ - static int size = 0; - char *str; - if (!size) { - str = getenv("ALT_STRUCT_SIZE"); - size = str ? atoi(str) : 12; - } - - log_debug("altinstr_size=%d\n", size); - return size; -} - static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset) { static int size = 0; @@ -1021,11 +1008,6 @@ static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset) return size; } -static int undefined_group_size(struct kpatch_elf *kelf, int offset) -{ - return 0; -} - /* * The rela groups in the .fixup section vary in size. The beginning of each * .fixup rela group is referenced by the .ex_table section. To find the size @@ -1099,14 +1081,6 @@ static struct special_section special_sections[] = { .name = ".ex_table", .group_size = ex_table_group_size, }, - { - .name = ".altinstructions", - .group_size = altinstructions_group_size, - }, - { - .name = ".altinstr_replacement", - .group_size = undefined_group_size, - }, { .name = ".livepatch.hooks.load", .group_size = livepatch_hooks_group_size,
The current handling of altinstructions sections by the livepatch tools is incorrect, as on Xen those sections are part of .init and thus discarded after load. Correctly handle them by just ignoring, as it's done with other .init related sections. While there also add .data.ro_after_init section as a read-only section and introduce some syntactic sugar for comparing section names. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- I assume this handling of .altinstr* sections was inherited from Linux where the sections are not discarded after load in order to apply alternative patching to the loaded modules after boot. --- common.c | 7 +++++-- create-diff-object.c | 26 -------------------------- 2 files changed, 5 insertions(+), 28 deletions(-)