| Message ID | 20190808123527.8340-1-wipawel@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 8/8/19 1:35 PM, Pawel Wieczorkiewicz wrote: > Handle .livepatch.hooks* and .altinstr_replacement sections as the > special sections with assigned group_size resolution function. > By default each .livepatch.hooks* sections' entry is 8 bytes long (a > pointer). The .altinstr_replacement section has undefined group_size. > > Allow to specify different .livepatch.hooks* section entry size using > shell environment variable HOOK_STRUCT_SIZE. > > Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de> > Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com> > Reviewed-by: Bjoern Doebel <doebel@amazon.de> > Reviewed-by: Norbert Manthey <nmanthey@amazon.de> > --- > v2: > * Applied suggestions from Ross and neccessary changes enforced by > previous patch of the series: > - fixed indentation > - used log_debug() instead of printf() > - added aux. function undefined_group_size() returning 0 for a > undefined group_size > - added .altinstr_replacement to the special_sections array and > fixed its group_size to undefined (0). > --- > create-diff-object.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/create-diff-object.c b/create-diff-object.c > index c6183c3..8365af0 100644 > --- a/create-diff-object.c > +++ b/create-diff-object.c > @@ -995,6 +995,24 @@ static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) > return size; > } > > +static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset) > +{ > + static int size = 0; > + char *str; > + if (!size) { > + str = getenv("HOOK_STRUCT_SIZE"); > + size = str ? atoi(str) : 8; > + } > + > + log_debug("livepatch_hooks_size=%d\n", size); > + 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 > @@ -1072,6 +1090,18 @@ static struct special_section special_sections[] = { > .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, > + }, > + { > + .name = ".livepatch.hooks.unload", > + .group_size = livepatch_hooks_group_size, > + }, > {}, > }; > > Unless I'm misunderstanding something, I can't see how kpatch_regenerate_special_section would work with .altinstr_replacement having a group size of 0. It looks to me like the for loop in that function would become an infinite loop (due to incrementing by group_size) and should_keep_rela_group would always return false. Regards,
On 16. Aug 2019, at 11:40, Ross Lagerwall <ross.lagerwall@citrix.com<mailto:ross.lagerwall@citrix.com>> wrote:
On 8/8/19 1:35 PM, Pawel Wieczorkiewicz wrote:
…snip...
* 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
@@ -1072,6 +1090,18 @@ static struct special_section special_sections[] = {
.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,
+ },
+ {
+ .name = ".livepatch.hooks.unload",
+ .group_size = livepatch_hooks_group_size,
+ },
{},
};
Unless I'm misunderstanding something, I can't see how kpatch_regenerate_special_section would work with .altinstr_replacement having a group size of 0. It looks to me like the for loop in that function would become an infinite loop (due to incrementing by group_size) and should_keep_rela_group would always return false.
AFAICS, the group_size 0 sections are never actually processed by the kpatch_regenerate_special_section().
They are not RELA sections and the following check excludes them from this processing:
static void kpatch_process_special_sections(struct kpatch_elf *kelf)
{
[…]
for (special = special_sections; special->name; special++) {
[…]
sec = sec->rela;
if (!sec)
continue;
kpatch_regenerate_special_section(kelf, special, sec);
}
Nevertheless, you are right. It does not make much sense to rely on this assumption.
I will add explicit checks to kpatch_regenerate_special_section() and friends dealing with group_size 0 sections.
Regards,
--
Ross Lagerwall
Best,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
On 8/16/19 1:06 PM, Wieczorkiewicz, Pawel wrote: > >> On 16. Aug 2019, at 11:40, Ross Lagerwall <ross.lagerwall@citrix.com >> <mailto:ross.lagerwall@citrix.com>> wrote: >> >> On 8/8/19 1:35 PM, Pawel Wieczorkiewicz wrote: >>> > > …snip... > >>> * 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 >>> @@ -1072,6 +1090,18 @@ static struct special_section >>> special_sections[] = { >>> .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, >>> +}, >>> +{ >>> +.name= ".livepatch.hooks.unload", >>> +.group_size= livepatch_hooks_group_size, >>> +}, >>> {}, >>> }; >>> >> >> Unless I'm misunderstanding something, I can't see how >> kpatch_regenerate_special_section would work with >> .altinstr_replacement having a group size of 0. It looks to me like >> the for loop in that function would become an infinite loop (due to >> incrementing by group_size) and should_keep_rela_group would always >> return false. >> > > AFAICS, the group_size 0 sections are never actually processed by the > kpatch_regenerate_special_section(). > They are not RELA sections and the following check excludes them from > this processing: > OK, that makes sense. Thanks,
diff --git a/create-diff-object.c b/create-diff-object.c index c6183c3..8365af0 100644 --- a/create-diff-object.c +++ b/create-diff-object.c @@ -995,6 +995,24 @@ static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) return size; } +static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset) +{ + static int size = 0; + char *str; + if (!size) { + str = getenv("HOOK_STRUCT_SIZE"); + size = str ? atoi(str) : 8; + } + + log_debug("livepatch_hooks_size=%d\n", size); + 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 @@ -1072,6 +1090,18 @@ static struct special_section special_sections[] = { .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, + }, + { + .name = ".livepatch.hooks.unload", + .group_size = livepatch_hooks_group_size, + }, {}, };