| Message ID | 20190416120716.26269-4-wipawel@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [livepatch-build-tools,part2,1/6] common: Add is_standard_section() helper function | expand |
On Tue, Apr 16, 2019 at 12:07:14PM +0000, Pawel Wieczorkiewicz wrote: > Hard-coding the special section group sizes is unreliable. Instead, > determine them dynamically by finding the related struct definitions > in the DWARF metadata. > > This is a livepatch backport of kpatch upstream commit [1]: > kpatch-build: detect special section group sizes 170449847136a48b19fc > > Xen only deals with alt_instr, bug_frame and exception_table_entry > structures, so sizes of these structers are obtained from xen-syms. > > This change is needed since with recent Xen the alt_instr structure > has changed size from 12 to 14 bytes. Oh this is so much better than the "solution" we coded. Thank you! Ross, will commit to repo unless you have concerns.. > > [1] https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56 > > Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de> > Reviewed-by: Bjoern Doebel <doebel@amazon.de> > Reviewed-by: Martin Mazein <amazein@amazon.de> > --- > create-diff-object.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-------- > livepatch-build | 23 ++++++++++++++++++++ > 2 files changed, 74 insertions(+), 9 deletions(-) > > diff --git a/create-diff-object.c b/create-diff-object.c > index 1e6e617..b0b4dcb 100644 > --- a/create-diff-object.c > +++ b/create-diff-object.c > @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf) > } > } > > -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { return 8; } > -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { return 8; } > -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { return 8; } > -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { return 16; } > -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return 8; } > -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) { return 12; } > +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset) > +{ > + static int size = 0; > + char *str; > + if (!size) { > + str = getenv("BUG_STRUCT_SIZE"); > + size = str ? atoi(str) : 8; > + } > + > + return size; > +} > + > +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) > +{ > + static int size = 0; > + char *str; > + if (!size) { > + str = getenv("BUG_STRUCT_SIZE"); > + size = str ? atoi(str) : 16; > + } > + > + return size; > +} > + > +static int ex_table_group_size(struct kpatch_elf *kelf, int offset) > +{ > + static int size = 0; > + char *str; > + if (!size) { > + str = getenv("EX_STRUCT_SIZE"); > + size = str ? atoi(str) : 8; > + } > + > + 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; > + } > + > + printf("altinstr_size=%d\n", size); > + return size; > +} > > /* > * The rela groups in the .fixup section vary in size. The beginning of each > @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf *kelf, int offset) > static struct special_section special_sections[] = { > { > .name = ".bug_frames.0", > - .group_size = bug_frames_0_group_size, > + .group_size = bug_frames_group_size, > }, > { > .name = ".bug_frames.1", > - .group_size = bug_frames_1_group_size, > + .group_size = bug_frames_group_size, > }, > { > .name = ".bug_frames.2", > - .group_size = bug_frames_2_group_size, > + .group_size = bug_frames_group_size, > }, > { > .name = ".bug_frames.3", > diff --git a/livepatch-build b/livepatch-build > index c057fa1..a6cae12 100755 > --- a/livepatch-build > +++ b/livepatch-build > @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}" > echo "================================================" > echo > > +if [ -f "$XENSYMS" ]; then > + echo "Reading special section data" > + SPECIAL_VARS=$(readelf -wi "$XENSYMS" | > + gawk --non-decimal-data ' > + BEGIN { a = b = e = 0 } > + a == 0 && /DW_AT_name.* alt_instr/ {a = 1; next} > + b == 0 && /DW_AT_name.* bug_frame/ {b = 1; next} > + e == 0 && /DW_AT_name.* exception_table_entry/ {e = 1; next} > + a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2} > + b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2} > + e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2} > + a == 2 && b == 2 && e == 2 {exit}') > + [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS" > + if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z $EX_STRUCT_SIZE ]]; then > + die "can't find special struct size" > + fi > + for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do > + if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then > + die "invalid special struct size $i" > + fi > + done > +fi > + > if [ "${SKIP}" != "build" ]; then > [ -e "${OUTPUT}" ] && die "Output directory exists" > grep -q 'CONFIG_LIVEPATCH=y' "${CONFIGFILE}" || die "CONFIG_LIVEPATCH must be enabled" > -- > 2.16.5 > > > > > Amazon Development Center Germany GmbH > Krausenstr. 38 > 10117 Berlin > Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich > Ust-ID: DE 289 237 879 > Eingetragen am Amtsgericht Charlottenburg HRB 149173 B > >
On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote: > Hard-coding the special section group sizes is unreliable. Instead, > determine them dynamically by finding the related struct definitions > in the DWARF metadata. > > This is a livepatch backport of kpatch upstream commit [1]: > kpatch-build: detect special section group sizes 170449847136a48b19fc > > Xen only deals with alt_instr, bug_frame and exception_table_entry > structures, so sizes of these structers are obtained from xen-syms. structers -> structures > > This change is needed since with recent Xen the alt_instr structure > has changed size from 12 to 14 bytes. > > [1] https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56 > > Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de> > Reviewed-by: Bjoern Doebel <doebel@amazon.de> > Reviewed-by: Martin Mazein <amazein@amazon.de> > --- > create-diff-object.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-------- > livepatch-build | 23 ++++++++++++++++++++ > 2 files changed, 74 insertions(+), 9 deletions(-) > > diff --git a/create-diff-object.c b/create-diff-object.c > index 1e6e617..b0b4dcb 100644 > --- a/create-diff-object.c > +++ b/create-diff-object.c > @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf) > } > } > > -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { return 8; } > -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { return 8; } > -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { return 8; } > -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { return 16; } > -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return 8; } > -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) { return 12; } > +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset) > +{ > + static int size = 0; > + char *str; > + if (!size) { > + str = getenv("BUG_STRUCT_SIZE"); > + size = str ? atoi(str) : 8; Why did you remove the hard error here from the original kpatch commit? I think a hard error is preferable to guessing. > + } > + > + return size; > +} > + > +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) > +{ > + static int size = 0; > + char *str; > + if (!size) { > + str = getenv("BUG_STRUCT_SIZE"); > + size = str ? atoi(str) : 16; > + } > + > + return size; > +} > + > +static int ex_table_group_size(struct kpatch_elf *kelf, int offset) > +{ > + static int size = 0; > + char *str; > + if (!size) { > + str = getenv("EX_STRUCT_SIZE"); > + size = str ? atoi(str) : 8; > + } > + > + 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; > + } > + > + printf("altinstr_size=%d\n", size); > + return size; > +} > > /* > * The rela groups in the .fixup section vary in size. The beginning of each > @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf *kelf, int offset) > static struct special_section special_sections[] = { > { > .name = ".bug_frames.0", > - .group_size = bug_frames_0_group_size, > + .group_size = bug_frames_group_size, > }, > { > .name = ".bug_frames.1", > - .group_size = bug_frames_1_group_size, > + .group_size = bug_frames_group_size, > }, > { > .name = ".bug_frames.2", > - .group_size = bug_frames_2_group_size, > + .group_size = bug_frames_group_size, > }, > { > .name = ".bug_frames.3", > diff --git a/livepatch-build b/livepatch-build > index c057fa1..a6cae12 100755 > --- a/livepatch-build > +++ b/livepatch-build > @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}" > echo "================================================" > echo > > +if [ -f "$XENSYMS" ]; then > + echo "Reading special section data" > + SPECIAL_VARS=$(readelf -wi "$XENSYMS" | > + gawk --non-decimal-data ' > + BEGIN { a = b = e = 0 } > + a == 0 && /DW_AT_name.* alt_instr/ {a = 1; next} > + b == 0 && /DW_AT_name.* bug_frame/ {b = 1; next} > + e == 0 && /DW_AT_name.* exception_table_entry/ {e = 1; next} > + a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2} > + b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2} > + e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2} > + a == 2 && b == 2 && e == 2 {exit}') > + [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS" > + if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z $EX_STRUCT_SIZE ]]; then > + die "can't find special struct size" > + fi > + for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do > + if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then > + die "invalid special struct size $i" > + fi > + done > +fi > + If this hunk is moved after the call to build_full(), then it can be run directly on the xen-syms that has just been built which would allow dropping `if [ -f "$XENSYMS" ]...` and guaranteeing that *_STRUCT_SIZE are always set.
On 4/25/19 5:53 AM, Konrad Rzeszutek Wilk wrote: > On Tue, Apr 16, 2019 at 12:07:14PM +0000, Pawel Wieczorkiewicz wrote: >> Hard-coding the special section group sizes is unreliable. Instead, >> determine them dynamically by finding the related struct definitions >> in the DWARF metadata. >> >> This is a livepatch backport of kpatch upstream commit [1]: >> kpatch-build: detect special section group sizes 170449847136a48b19fc >> >> Xen only deals with alt_instr, bug_frame and exception_table_entry >> structures, so sizes of these structers are obtained from xen-syms. >> >> This change is needed since with recent Xen the alt_instr structure >> has changed size from 12 to 14 bytes. > > Oh this is so much better than the "solution" we coded. > > Thank you! > > Ross, will commit to repo unless you have concerns.. I have reviewed it with a few comments. Note that this is basically the same as Glenn Enright's recent patch ("livepatch-build-tools: Detect special section group sizes") but slightly more complete as it detects sizes for the bug frames too so it should probably be used instead. Thanks,
On 4/29/19 3:10 PM, Ross Lagerwall wrote: > On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote: >> Hard-coding the special section group sizes is unreliable. Instead, >> determine them dynamically by finding the related struct definitions >> in the DWARF metadata. >> >> This is a livepatch backport of kpatch upstream commit [1]: >> kpatch-build: detect special section group sizes 170449847136a48b19fc >> >> Xen only deals with alt_instr, bug_frame and exception_table_entry >> structures, so sizes of these structers are obtained from xen-syms. > > structers -> structures > >> >> This change is needed since with recent Xen the alt_instr structure >> has changed size from 12 to 14 bytes. >> >> [1] >> https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56 >> >> >> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de> >> Reviewed-by: Bjoern Doebel <doebel@amazon.de> >> Reviewed-by: Martin Mazein <amazein@amazon.de> >> --- >> create-diff-object.c | 60 >> ++++++++++++++++++++++++++++++++++++++++++++-------- >> livepatch-build | 23 ++++++++++++++++++++ >> 2 files changed, 74 insertions(+), 9 deletions(-) >> >> diff --git a/create-diff-object.c b/create-diff-object.c >> index 1e6e617..b0b4dcb 100644 >> --- a/create-diff-object.c >> +++ b/create-diff-object.c >> @@ -958,12 +958,54 @@ static void >> kpatch_mark_constant_labels_same(struct kpatch_elf *kelf) >> } >> } >> -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int >> offset) { return 8; } >> -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int >> offset) { return 8; } >> -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int >> offset) { return 8; } >> -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int >> offset) { return 16; } >> -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { >> return 8; } >> -static int altinstructions_group_size(struct kpatch_elf *kelf, int >> offset) { return 12; } >> +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset) >> +{ >> + static int size = 0; >> + char *str; >> + if (!size) { >> + str = getenv("BUG_STRUCT_SIZE"); >> + size = str ? atoi(str) : 8; > > Why did you remove the hard error here from the original kpatch commit? > I think a hard error is preferable to guessing. > >> + } >> + >> + return size; >> +} >> + >> +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) >> +{ >> + static int size = 0; >> + char *str; >> + if (!size) { >> + str = getenv("BUG_STRUCT_SIZE"); >> + size = str ? atoi(str) : 16; >> + } >> + >> + return size; >> +} >> + >> +static int ex_table_group_size(struct kpatch_elf *kelf, int offset) >> +{ >> + static int size = 0; >> + char *str; >> + if (!size) { >> + str = getenv("EX_STRUCT_SIZE"); >> + size = str ? atoi(str) : 8; >> + } >> + >> + 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; >> + } >> + >> + printf("altinstr_size=%d\n", size); >> + return size; >> +} >> /* >> * The rela groups in the .fixup section vary in size. The >> beginning of each >> @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf >> *kelf, int offset) >> static struct special_section special_sections[] = { >> { >> .name = ".bug_frames.0", >> - .group_size = bug_frames_0_group_size, >> + .group_size = bug_frames_group_size, >> }, >> { >> .name = ".bug_frames.1", >> - .group_size = bug_frames_1_group_size, >> + .group_size = bug_frames_group_size, >> }, >> { >> .name = ".bug_frames.2", >> - .group_size = bug_frames_2_group_size, >> + .group_size = bug_frames_group_size, >> }, >> { >> .name = ".bug_frames.3", >> diff --git a/livepatch-build b/livepatch-build >> index c057fa1..a6cae12 100755 >> --- a/livepatch-build >> +++ b/livepatch-build >> @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}" >> echo "================================================" >> echo >> +if [ -f "$XENSYMS" ]; then >> + echo "Reading special section data" >> + SPECIAL_VARS=$(readelf -wi "$XENSYMS" | >> + gawk --non-decimal-data ' >> + BEGIN { a = b = e = 0 } >> + a == 0 && /DW_AT_name.* alt_instr/ {a = 1; next} >> + b == 0 && /DW_AT_name.* bug_frame/ {b = 1; next} >> + e == 0 && /DW_AT_name.* exception_table_entry/ {e = 1; >> next} >> + a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2} >> + b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2} >> + e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2} >> + a == 2 && b == 2 && e == 2 {exit}') >> + [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS" >> + if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ >> -z $EX_STRUCT_SIZE ]]; then >> + die "can't find special struct size" >> + fi >> + for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do >> + if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then >> + die "invalid special struct size $i" >> + fi >> + done >> +fi >> + > > If this hunk is moved after the call to build_full(), then it can be run > directly on the xen-syms that has just been built which would allow > dropping `if [ -f "$XENSYMS" ]...` and guaranteeing that *_STRUCT_SIZE > are always set. > One more thing, previously: bug_frames_0_group_size == 8 bug_frames_1_group_size == 8 bug_frames_2_group_size == 8 bug_frames_3_group_size == 16 And now: bug_frames_0_group_size == BUG_STRUCT_SIZE bug_frames_1_group_size == BUG_STRUCT_SIZE bug_frames_2_group_size == BUG_STRUCT_SIZE bug_frames_3_group_size == BUG_STRUCT_SIZE This seems wrong to me. When reading special section data, should you detect e.g. BUG0_STRUCT_SIZE, BUG1_STRUCT_SIZE, ... ?
> On 29. Apr 2019, at 16:21, Ross Lagerwall <ross.lagerwall@citrix.com> wrote: > > On 4/29/19 3:10 PM, Ross Lagerwall wrote: >> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote: >>> Hard-coding the special section group sizes is unreliable. Instead, >>> determine them dynamically by finding the related struct definitions >>> in the DWARF metadata. >>> >>> This is a livepatch backport of kpatch upstream commit [1]: >>> kpatch-build: detect special section group sizes 170449847136a48b19fc >>> >>> Xen only deals with alt_instr, bug_frame and exception_table_entry >>> structures, so sizes of these structers are obtained from xen-syms. >> structers -> structures Thanks, will fix. >>> >>> This change is needed since with recent Xen the alt_instr structure >>> has changed size from 12 to 14 bytes. >>> >>> [1] https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56 >>> >>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de> >>> Reviewed-by: Bjoern Doebel <doebel@amazon.de> >>> Reviewed-by: Martin Mazein <amazein@amazon.de> >>> --- >>> create-diff-object.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-------- >>> livepatch-build | 23 ++++++++++++++++++++ >>> 2 files changed, 74 insertions(+), 9 deletions(-) >>> >>> diff --git a/create-diff-object.c b/create-diff-object.c >>> index 1e6e617..b0b4dcb 100644 >>> --- a/create-diff-object.c >>> +++ b/create-diff-object.c >>> @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf) >>> } >>> } >>> -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { return 8; } >>> -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { return 8; } >>> -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { return 8; } >>> -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { return 16; } >>> -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return 8; } >>> -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) { return 12; } >>> +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset) >>> +{ >>> + static int size = 0; >>> + char *str; >>> + if (!size) { >>> + str = getenv("BUG_STRUCT_SIZE"); >>> + size = str ? atoi(str) : 8; >> Why did you remove the hard error here from the original kpatch commit? I think a hard error is preferable to guessing. Previously the sizes were hard-coded. I prefer to use them directly over failing hard here. If we could not come up with a sane defaults, than failing hard would be the only option. Anyway, I am cool to go either way upon your good judgment. >>> + } >>> + >>> + return size; >>> +} >>> + >>> +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) >>> +{ >>> + static int size = 0; >>> + char *str; >>> + if (!size) { >>> + str = getenv("BUG_STRUCT_SIZE"); >>> + size = str ? atoi(str) : 16; >>> + } >>> + >>> + return size; >>> +} >>> + >>> +static int ex_table_group_size(struct kpatch_elf *kelf, int offset) >>> +{ >>> + static int size = 0; >>> + char *str; >>> + if (!size) { >>> + str = getenv("EX_STRUCT_SIZE"); >>> + size = str ? atoi(str) : 8; >>> + } >>> + >>> + 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; >>> + } >>> + >>> + printf("altinstr_size=%d\n", size); >>> + return size; >>> +} >>> /* >>> * The rela groups in the .fixup section vary in size. The beginning of each >>> @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf *kelf, int offset) >>> static struct special_section special_sections[] = { >>> { >>> .name = ".bug_frames.0", >>> - .group_size = bug_frames_0_group_size, >>> + .group_size = bug_frames_group_size, >>> }, >>> { >>> .name = ".bug_frames.1", >>> - .group_size = bug_frames_1_group_size, >>> + .group_size = bug_frames_group_size, >>> }, >>> { >>> .name = ".bug_frames.2", >>> - .group_size = bug_frames_2_group_size, >>> + .group_size = bug_frames_group_size, >>> }, >>> { >>> .name = ".bug_frames.3", >>> diff --git a/livepatch-build b/livepatch-build >>> index c057fa1..a6cae12 100755 >>> --- a/livepatch-build >>> +++ b/livepatch-build >>> @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}" >>> echo "================================================" >>> echo >>> +if [ -f "$XENSYMS" ]; then >>> + echo "Reading special section data" >>> + SPECIAL_VARS=$(readelf -wi "$XENSYMS" | >>> + gawk --non-decimal-data ' >>> + BEGIN { a = b = e = 0 } >>> + a == 0 && /DW_AT_name.* alt_instr/ {a = 1; next} >>> + b == 0 && /DW_AT_name.* bug_frame/ {b = 1; next} >>> + e == 0 && /DW_AT_name.* exception_table_entry/ {e = 1; next} >>> + a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2} >>> + b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2} >>> + e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2} >>> + a == 2 && b == 2 && e == 2 {exit}') >>> + [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS" >>> + if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z $EX_STRUCT_SIZE ]]; then >>> + die "can't find special struct size" >>> + fi >>> + for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do >>> + if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then >>> + die "invalid special struct size $i" >>> + fi >>> + done >>> +fi >>> + >> If this hunk is moved after the call to build_full(), then it can be run directly on the xen-syms that has just been built which would allow dropping `if [ -f "$XENSYMS" ]...` and guaranteeing that *_STRUCT_SIZE are always set. > > One more thing, previously: > > bug_frames_0_group_size == 8 > bug_frames_1_group_size == 8 > bug_frames_2_group_size == 8 > bug_frames_3_group_size == 16 > > And now: > > bug_frames_0_group_size == BUG_STRUCT_SIZE > bug_frames_1_group_size == BUG_STRUCT_SIZE > bug_frames_2_group_size == BUG_STRUCT_SIZE > bug_frames_3_group_size == BUG_STRUCT_SIZE > > This seems wrong to me. When reading special section data, should you detect e.g. BUG0_STRUCT_SIZE, BUG1_STRUCT_SIZE, … ? > There is only one struct bug_frame definition in Xen: Using pahole: struct bug_frame { unsigned char ud2[2]; /* 0 2 */ unsigned char ret; /* 2 1 */ short unsigned int id; /* 3 2 */ /* size: 5, cachelines: 1, members: 3 */ /* last cacheline: 5 bytes */ }; It’s size is 5, so fits into 8 bytes. Definitions for all the 0, 1, 2, 3 groups use the same struct bug_frame: Example grep: include/asm-x86/bug.h:extern const struct bug_frame __start_bug_frames[], include/asm-x86/bug.h: __stop_bug_frames_0[], include/asm-x86/bug.h: __stop_bug_frames_1[], include/asm-x86/bug.h: __stop_bug_frames_2[], include/asm-x86/bug.h: __stop_bug_frames_3[]; So, the default group size of 16 bytes does not seem right. Example grep: $ objdump -g xen-syms|grep -A3 DW_TAG_structure_type|grep -A1 bug_frame|cut -f2- -d'>'|sort -u -- DW_AT_byte_size : 8 DW_AT_name : (indirect string, offset: 0x2556): bug_frame > -- > Ross Lagerwall Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich Ust-ID: DE 289 237 879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
On 30/04/19 2:14 AM, Ross Lagerwall wrote: > On 4/25/19 5:53 AM, Konrad Rzeszutek Wilk wrote: >> On Tue, Apr 16, 2019 at 12:07:14PM +0000, Pawel Wieczorkiewicz wrote: >>> Hard-coding the special section group sizes is unreliable. Instead, >>> determine them dynamically by finding the related struct definitions >>> in the DWARF metadata. >>> >>> This is a livepatch backport of kpatch upstream commit [1]: >>> kpatch-build: detect special section group sizes 170449847136a48b19fc >>> >>> Xen only deals with alt_instr, bug_frame and exception_table_entry >>> structures, so sizes of these structers are obtained from xen-syms. >>> >>> This change is needed since with recent Xen the alt_instr structure >>> has changed size from 12 to 14 bytes. >> >> Oh this is so much better than the "solution" we coded. >> >> Thank you! >> >> Ross, will commit to repo unless you have concerns.. > I have reviewed it with a few comments. Note that this is basically the > same as Glenn Enright's recent patch ("livepatch-build-tools: Detect > special section group sizes") but slightly more complete as it detects > sizes for the bug frames too so it should probably be used instead. > > Thanks, This is a goodness. Glad to see an appropriate patch get in regardless of which patch is used. Best, Glenn
diff --git a/create-diff-object.c b/create-diff-object.c index 1e6e617..b0b4dcb 100644 --- a/create-diff-object.c +++ b/create-diff-object.c @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf) } } -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { return 8; } -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { return 8; } -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { return 8; } -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { return 16; } -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return 8; } -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) { return 12; } +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset) +{ + static int size = 0; + char *str; + if (!size) { + str = getenv("BUG_STRUCT_SIZE"); + size = str ? atoi(str) : 8; + } + + return size; +} + +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) +{ + static int size = 0; + char *str; + if (!size) { + str = getenv("BUG_STRUCT_SIZE"); + size = str ? atoi(str) : 16; + } + + return size; +} + +static int ex_table_group_size(struct kpatch_elf *kelf, int offset) +{ + static int size = 0; + char *str; + if (!size) { + str = getenv("EX_STRUCT_SIZE"); + size = str ? atoi(str) : 8; + } + + 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; + } + + printf("altinstr_size=%d\n", size); + return size; +} /* * The rela groups in the .fixup section vary in size. The beginning of each @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf *kelf, int offset) static struct special_section special_sections[] = { { .name = ".bug_frames.0", - .group_size = bug_frames_0_group_size, + .group_size = bug_frames_group_size, }, { .name = ".bug_frames.1", - .group_size = bug_frames_1_group_size, + .group_size = bug_frames_group_size, }, { .name = ".bug_frames.2", - .group_size = bug_frames_2_group_size, + .group_size = bug_frames_group_size, }, { .name = ".bug_frames.3", diff --git a/livepatch-build b/livepatch-build index c057fa1..a6cae12 100755 --- a/livepatch-build +++ b/livepatch-build @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}" echo "================================================" echo +if [ -f "$XENSYMS" ]; then + echo "Reading special section data" + SPECIAL_VARS=$(readelf -wi "$XENSYMS" | + gawk --non-decimal-data ' + BEGIN { a = b = e = 0 } + a == 0 && /DW_AT_name.* alt_instr/ {a = 1; next} + b == 0 && /DW_AT_name.* bug_frame/ {b = 1; next} + e == 0 && /DW_AT_name.* exception_table_entry/ {e = 1; next} + a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2} + b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2} + e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2} + a == 2 && b == 2 && e == 2 {exit}') + [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS" + if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z $EX_STRUCT_SIZE ]]; then + die "can't find special struct size" + fi + for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do + if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then + die "invalid special struct size $i" + fi + done +fi + if [ "${SKIP}" != "build" ]; then [ -e "${OUTPUT}" ] && die "Output directory exists" grep -q 'CONFIG_LIVEPATCH=y' "${CONFIGFILE}" || die "CONFIG_LIVEPATCH must be enabled"