[v3,5/7] x86/livepatch: Fail the build if duplicate symbols exist
diff mbox series

Message ID 20191023135812.21348-6-andrew.cooper3@citrix.com
State New
Headers show
Series
  • Unbreak evaluate_nospec() and livepatching
Related show

Commit Message

Andrew Cooper Oct. 23, 2019, 1:58 p.m. UTC
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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Norbert Manthey <nmanthey@amazon.de>
CC: Juergen Gross <jgross@suse.com>

v3:
 * Use a new config option
---
 xen/arch/x86/Makefile              |  1 +
 xen/common/Kconfig                 | 18 ++++++++++++++++--
 xen/tools/kconfig/allrandom.config |  1 +
 xen/tools/symbols.c                | 11 +++++++++--
 4 files changed, 27 insertions(+), 4 deletions(-)

Comments

Jürgen Groß Oct. 23, 2019, 2:46 p.m. UTC | #1
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
Konrad Rzeszutek Wilk Oct. 23, 2019, 4:14 p.m. UTC | #2
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
Ross Lagerwall Oct. 23, 2019, 4:37 p.m. UTC | #3
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".
Jan Beulich Oct. 24, 2019, 12:03 p.m. UTC | #4
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
Andrew Cooper Oct. 29, 2019, 5:06 p.m. UTC | #5
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
Jan Beulich Oct. 30, 2019, 8:41 a.m. UTC | #6
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
Andrew Cooper Oct. 30, 2019, 10:37 a.m. UTC | #7
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
Jan Beulich Oct. 30, 2019, 11:21 a.m. UTC | #8
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

Patch
diff mbox series

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);