Re: [Xen-devel] [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/10/17 13:03, Jan Beulich wrote:
>>>> On 03.10.17 at 20:07, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/Kconfig
>> +++ b/xen/Kconfig
>> @@ -38,4 +38,10 @@ config LTO
>>  
>>        If unsure, say N.
>>  
>> +#
>> +# For architectures that know their GCC __int128 support is sound
>> +#
>> +config ARCH_SUPPORTS_INT128
>> +    bool
> Why GCC? What about Clang?

This came straight from Linux.  I can s/GCC/compiler/ if you like?

>
>> --- a/xen/Kconfig.debug
>> +++ b/xen/Kconfig.debug
>> @@ -121,6 +121,14 @@ config SCRUB_DEBUG
>>        Verify that pages that need to be scrubbed before being allocated to
>>        a guest are indeed scrubbed.
>>  
>> +config UBSAN
>> +    bool "Undefined behaviour sanitizer"
>> +    depends on X86
> I think we should switch away from this model of explicitly stating
> architectures, and instead have HAVE_* symbols selected by each
> architecture supporting it, and the main symbol then depending on
> the HAVE_* one. Us having only two architectures right now
> doesn't make this a big difference, but Linux has (partially?)
> switched to that model for a reason, I think.

I'm not fussed.  Which would you prefer?

>
>> --- a/xen/Rules.mk
>> +++ b/xen/Rules.mk
>> @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y)
>>  $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS 
>> += 
>> -fprofile-arcs -ftest-coverage
>>  endif
>>  
>> +ifeq ($(CONFIG_UBSAN),y)
>> +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): 
>> CFLAGS += -fsanitize=undefined
>> +endif
> You have no users of noubsan-y, other than what Wei's RFC patch
> had. Also neither you nor he explain why *.init.o are unilaterally
> excluded.

The answer is complicated.  If you want it to work with .init. files,
then the following change is required:

diff --git a/xen/Rules.mk b/xen/Rules.mk
index cafc67b..9ce5b56 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -189,7 +189,7 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)):
%.init.o: %.o Makefile
                .text|.text.*|.data|.data.*|.bss) \
                        test $$sz != 0 || continue; \
                        echo "Error: size of $<:$$name is 0x$$sz" >&2; \
-                       exit $$(expr $$idx + 1);; \
+                       # exit $$(expr $$idx + 1);; \
                esac; \
        done
        $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section
.$(s)=.init.$(s)) $< $@

I was debating whether to keep or remove the noubsan, but I figured that
keeping it would be more flexible for developing with.

>
>> --- a/xen/common/ubsan/ubsan.c
>> +++ b/xen/common/ubsan/ubsan.c
>> @@ -10,13 +10,21 @@
>>   *
>>   */
>>  
>> -#include <linux/bitops.h>
>> -#include <linux/bug.h>
>> -#include <linux/ctype.h>
>> -#include <linux/init.h>
>> -#include <linux/kernel.h>
>> -#include <linux/types.h>
>> -#include <linux/sched.h>
>> +#include <xen/bitops.h>
>> +#include <xen/kernel.h>
>> +#include <xen/lib.h>
>> +#include <xen/types.h>
>> +#include <xen/spinlock.h>
>> +#include <xen/percpu.h>
> I don't think all of these are needed - xen/types.h is certainly
> being included implicitly by several others, for example, and
> always will be.

Hmm.  This list of headers is ~18 months old.  I will see if I can prune
it some more.

>
>> --- a/xen/include/xen/compiler.h
>> +++ b/xen/include/xen/compiler.h
>> @@ -15,6 +15,7 @@
>>  #define noinline      __attribute__((__noinline__))
>>  
>>  #define noreturn      __attribute__((__noreturn__))
>> +#define __noreturn    noreturn
> Please let's avoid new name space violations if at all possibly, or
> at least restrict them to individual source files where eliminating
> them would be undesirable.

This is entirely down to how much we want to diverge the Linux code. 
For ubsan, I've gone out of my way not to modify the Linux code at all.

I can see an argument for making this local to the file in question. 
However, that needs to be weighed up against other Linux source we
choose to take.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel