| Message ID | 20191023135812.21348-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unbreak evaluate_nospec() and livepatching | expand |
On 23.10.19 15:58, Andrew Cooper wrote: > From: Ross Lagerwall <ross.lagerwall@citrix.com> > > The binary diffing algorithm used by xen-livepatch depends on having unique > symbols. > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > > The livepatch loading algorithm used by Xen resolves relocations by symbol > name, and thus also depends on having unique symbols. > > Introduce CONFIG_ENFORCE_UNIQUE_SYMBOLS to control failing the build if > duplicate symbols are found, and disable it in the RANDCONFIG build. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Release-acked-by: Juergen Gross <jgross@suse.com> Juergen
On October 23, 2019 10:46:37 AM EDT, "Jürgen Groß" <jgross@suse.com> wrote: >On 23.10.19 15:58, Andrew Cooper wrote: >> From: Ross Lagerwall <ross.lagerwall@citrix.com> >> >> The binary diffing algorithm used by xen-livepatch depends on having >unique >> symbols. >> >> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >> >> The livepatch loading algorithm used by Xen resolves relocations by >symbol >> name, and thus also depends on having unique symbols. >> >> Introduce CONFIG_ENFORCE_UNIQUE_SYMBOLS to control failing the build >if >> duplicate symbols are found, and disable it in the RANDCONFIG build. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> What is up with that SoBs not being together? Could you squash them please? Patch wise, feel free to add my Reviewed-by. Thx > >Release-acked-by: Juergen Gross <jgross@suse.com> > > >Juergen
On 10/23/19 2:58 PM, Andrew Cooper wrote: > From: Ross Lagerwall <ross.lagerwall@citrix.com> > > The binary diffing algorithm used by xen-livepatch depends on having unique > symbols. > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > Presumably this should be "livepatch-build" or "livepatch-build-tools" rather than "xen-livepatch".
On 23.10.2019 15:58, Andrew Cooper wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP > > If unsure, say Y. > > +config ENFORCE_UNIQUE_SYMBOLS > + bool "Enforce unique symbols" if LIVEPATCH > + default y if LIVEPATCH Instead of two identical "if", why not "depends on LIVEPATCH"? > + ---help--- > + Multiple symbols with the same name aren't generally a problem > + unless Live patching is to be used. > + > + Livepatch loading involves resolving relocations against symbol > + names, and attempting to a duplicate symbol in a livepatch will > + result in incorrect livepatch application. > + > + This option should be used to ensure that a build of Xen can have a > + livepatch build and apply correctly. > + > config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS > - bool "Suppress duplicate symbol warnings" if !LIVEPATCH > - default y if !LIVEPATCH > + bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS > + default y if !ENFORCE_UNIQUE_SYMBOLS Similarly here then. With this changed, or with a proper reason supplied Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 24/10/2019 13:03, Jan Beulich wrote: > On 23.10.2019 15:58, Andrew Cooper wrote: >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP >> >> If unsure, say Y. >> >> +config ENFORCE_UNIQUE_SYMBOLS >> + bool "Enforce unique symbols" if LIVEPATCH >> + default y if LIVEPATCH > Instead of two identical "if", why not "depends on LIVEPATCH"? > >> + ---help--- >> + Multiple symbols with the same name aren't generally a problem >> + unless Live patching is to be used. >> + >> + Livepatch loading involves resolving relocations against symbol >> + names, and attempting to a duplicate symbol in a livepatch will >> + result in incorrect livepatch application. >> + >> + This option should be used to ensure that a build of Xen can have a >> + livepatch build and apply correctly. >> + >> config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS >> - bool "Suppress duplicate symbol warnings" if !LIVEPATCH >> - default y if !LIVEPATCH >> + bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS >> + default y if !ENFORCE_UNIQUE_SYMBOLS > Similarly here then. With this changed, or with a proper reason > supplied > Reviewed-by: Jan Beulich <jbeulich@suse.com> That's a question for the author of c/s 064a2652233 to answer... I'm merely following the prevailing style. ~Andrew
On 29.10.2019 18:06, Andrew Cooper wrote: > On 24/10/2019 13:03, Jan Beulich wrote: >> On 23.10.2019 15:58, Andrew Cooper wrote: >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP >>> >>> If unsure, say Y. >>> >>> +config ENFORCE_UNIQUE_SYMBOLS >>> + bool "Enforce unique symbols" if LIVEPATCH >>> + default y if LIVEPATCH >> Instead of two identical "if", why not "depends on LIVEPATCH"? >> >>> + ---help--- >>> + Multiple symbols with the same name aren't generally a problem >>> + unless Live patching is to be used. >>> + >>> + Livepatch loading involves resolving relocations against symbol >>> + names, and attempting to a duplicate symbol in a livepatch will >>> + result in incorrect livepatch application. >>> + >>> + This option should be used to ensure that a build of Xen can have a >>> + livepatch build and apply correctly. >>> + >>> config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS >>> - bool "Suppress duplicate symbol warnings" if !LIVEPATCH >>> - default y if !LIVEPATCH >>> + bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS >>> + default y if !ENFORCE_UNIQUE_SYMBOLS >> Similarly here then. With this changed, or with a proper reason >> supplied >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > That's a question for the author of c/s 064a2652233 to answer... I'm > merely following the prevailing style. "Prevailing style" seems a little bold considering that according to my grep-ing there's exactly on other such instance (VGA). I.e. you'd grow the "badness" by 50% when you could equally well shrink it by this same percentage. Jan
On 30/10/2019 08:41, Jan Beulich wrote: > On 29.10.2019 18:06, Andrew Cooper wrote: >> On 24/10/2019 13:03, Jan Beulich wrote: >>> On 23.10.2019 15:58, Andrew Cooper wrote: >>>> --- a/xen/common/Kconfig >>>> +++ b/xen/common/Kconfig >>>> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP >>>> >>>> If unsure, say Y. >>>> >>>> +config ENFORCE_UNIQUE_SYMBOLS >>>> + bool "Enforce unique symbols" if LIVEPATCH >>>> + default y if LIVEPATCH >>> Instead of two identical "if", why not "depends on LIVEPATCH"? >>> >>>> + ---help--- >>>> + Multiple symbols with the same name aren't generally a problem >>>> + unless Live patching is to be used. >>>> + >>>> + Livepatch loading involves resolving relocations against symbol >>>> + names, and attempting to a duplicate symbol in a livepatch will >>>> + result in incorrect livepatch application. >>>> + >>>> + This option should be used to ensure that a build of Xen can have a >>>> + livepatch build and apply correctly. >>>> + >>>> config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS >>>> - bool "Suppress duplicate symbol warnings" if !LIVEPATCH >>>> - default y if !LIVEPATCH >>>> + bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS >>>> + default y if !ENFORCE_UNIQUE_SYMBOLS >>> Similarly here then. With this changed, or with a proper reason >>> supplied >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> That's a question for the author of c/s 064a2652233 to answer... I'm >> merely following the prevailing style. > "Prevailing style" seems a little bold considering that according to > my grep-ing there's exactly on other such instance (VGA). I.e. you'd > grow the "badness" by 50% when you could equally well shrink it by > this same percentage. Allow me to be less subtle. *You* are the author of the code, in this form. As a consequence, I expect there is a deliberate reason. And seeing as I've had to reverse engineer the reason myself, it looks to be related to the fact that other options in the build select these, so they need not to be dependent on livepatching in the first place. ~Andrew
On 30.10.2019 11:37, Andrew Cooper wrote: > On 30/10/2019 08:41, Jan Beulich wrote: >> On 29.10.2019 18:06, Andrew Cooper wrote: >>> On 24/10/2019 13:03, Jan Beulich wrote: >>>> On 23.10.2019 15:58, Andrew Cooper wrote: >>>>> --- a/xen/common/Kconfig >>>>> +++ b/xen/common/Kconfig >>>>> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP >>>>> >>>>> If unsure, say Y. >>>>> >>>>> +config ENFORCE_UNIQUE_SYMBOLS >>>>> + bool "Enforce unique symbols" if LIVEPATCH >>>>> + default y if LIVEPATCH >>>> Instead of two identical "if", why not "depends on LIVEPATCH"? >>>> >>>>> + ---help--- >>>>> + Multiple symbols with the same name aren't generally a problem >>>>> + unless Live patching is to be used. >>>>> + >>>>> + Livepatch loading involves resolving relocations against symbol >>>>> + names, and attempting to a duplicate symbol in a livepatch will >>>>> + result in incorrect livepatch application. >>>>> + >>>>> + This option should be used to ensure that a build of Xen can have a >>>>> + livepatch build and apply correctly. >>>>> + >>>>> config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS >>>>> - bool "Suppress duplicate symbol warnings" if !LIVEPATCH >>>>> - default y if !LIVEPATCH >>>>> + bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS >>>>> + default y if !ENFORCE_UNIQUE_SYMBOLS >>>> Similarly here then. With this changed, or with a proper reason >>>> supplied >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> That's a question for the author of c/s 064a2652233 to answer... I'm >>> merely following the prevailing style. >> "Prevailing style" seems a little bold considering that according to >> my grep-ing there's exactly on other such instance (VGA). I.e. you'd >> grow the "badness" by 50% when you could equally well shrink it by >> this same percentage. > > Allow me to be less subtle. > > *You* are the author of the code, in this form. I'm sorry for not recalling. > As a consequence, I expect there is a deliberate reason. > > And seeing as I've had to reverse engineer the reason myself, it looks > to be related to the fact that other options in the build select these, > so they need not to be dependent on livepatching in the first place. It wasn't without reason that I did say "or with a proper reason supplied" - the select in xen/Kconfig.debug is a proper reason for SUPPRESS_DUPLICATE_SYMBOL_WARNINGS staying as it is, indeed. But it's then still not a reason for ENFORCE_UNIQUE_SYMBOLS to be this same way, as there's no similar select for it anywhere. Jan
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 2443fd2cc5..6b369f21cb 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -99,6 +99,7 @@ endif syms-warn-dup-y := --warn-dup syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) := +syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup $(TARGET): TMP = $(@D)/.$(@F).elf32 $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 diff --git a/xen/common/Kconfig b/xen/common/Kconfig index c9e671869e..4c837d6892 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP If unsure, say Y. +config ENFORCE_UNIQUE_SYMBOLS + bool "Enforce unique symbols" if LIVEPATCH + default y if LIVEPATCH + ---help--- + Multiple symbols with the same name aren't generally a problem + unless Live patching is to be used. + + Livepatch loading involves resolving relocations against symbol + names, and attempting to a duplicate symbol in a livepatch will + result in incorrect livepatch application. + + This option should be used to ensure that a build of Xen can have a + livepatch build and apply correctly. + config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS - bool "Suppress duplicate symbol warnings" if !LIVEPATCH - default y if !LIVEPATCH + bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS + default y if !ENFORCE_UNIQUE_SYMBOLS ---help--- Multiple symbols with the same name aren't generally a problem unless Live patching is to be used, so these warnings can be diff --git a/xen/tools/kconfig/allrandom.config b/xen/tools/kconfig/allrandom.config index 76f74320b5..c480896b96 100644 --- a/xen/tools/kconfig/allrandom.config +++ b/xen/tools/kconfig/allrandom.config @@ -2,3 +2,4 @@ CONFIG_GCOV_FORMAT_AUTODETECT=y CONFIG_UBSAN=n +CONFIG_ENFORCE_UNIQUE_SYMBOLS=n diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c index 05139d1600..9f9e2c9900 100644 --- a/xen/tools/symbols.c +++ b/xen/tools/symbols.c @@ -599,7 +599,7 @@ static int compare_name(const void *p1, const void *p2) int main(int argc, char **argv) { unsigned int i; - bool unsorted = false, warn_dup = false; + bool unsorted = false, warn_dup = false, error_dup = false, found_dup = false; if (argc >= 2) { for (i = 1; i < argc; i++) { @@ -619,6 +619,8 @@ int main(int argc, char **argv) sort_by_name = 1; else if (strcmp(argv[i], "--warn-dup") == 0) warn_dup = true; + else if (strcmp(argv[i], "--error-dup") == 0) + warn_dup = error_dup = true; else if (strcmp(argv[i], "--xensyms") == 0) map_only = true; else @@ -634,14 +636,19 @@ int main(int argc, char **argv) for (i = 1; i < table_cnt; ++i) if (strcmp(SYMBOL_NAME(table + i - 1), SYMBOL_NAME(table + i)) == 0 && - table[i - 1].addr != table[i].addr) + table[i - 1].addr != table[i].addr) { fprintf(stderr, "Duplicate symbol '%s' (%llx != %llx)\n", SYMBOL_NAME(table + i), table[i].addr, table[i - 1].addr); + found_dup = true; + } unsorted = true; } + if (error_dup && found_dup) + exit(1); + if (unsorted) qsort(table, table_cnt, sizeof(*table), compare_value);