| Message ID | 20171009141119.32595-4-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/10/17 15:11, Wei Liu wrote: > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 30c2769684..64955dc017 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig Based on Julien's testing, shouldn't ARM64 also select HAS_UBSAN? Otherwise, LGTM. ~Andrew
On Mon, Oct 09, 2017 at 03:23:37PM +0100, Andrew Cooper wrote: > On 09/10/17 15:11, Wei Liu wrote: > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > > index 30c2769684..64955dc017 100644 > > --- a/xen/arch/x86/Kconfig > > +++ b/xen/arch/x86/Kconfig > > Based on Julien's testing, shouldn't ARM64 also select HAS_UBSAN? > Otherwise, LGTM. IIRC there is an issue with the size of the image being larger than 2MB. I will let Julien decide. I can't test ARM support anyway.
On 09/10/17 15:23, Andrew Cooper wrote: > On 09/10/17 15:11, Wei Liu wrote: >> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig >> index 30c2769684..64955dc017 100644 >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig > > Based on Julien's testing, shouldn't ARM64 also select HAS_UBSAN? I tested it with some patches on top. Currently, the size of Xen Arm is limited to 2MB. That limit will be exploded when building with UBSAN. So for now, I will leave that unselectable for both Arm32 and Arm64. Cheers,
>>> On 09.10.17 at 16:11, <wei.liu2@citrix.com> wrote: > --- a/xen/common/ubsan/ubsan.c > +++ b/xen/common/ubsan/ubsan.c > @@ -10,13 +10,18 @@ > * > */ > > -#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/spinlock.h> > +#include <xen/percpu.h> > + > +#define __noreturn noreturn > +#define pr_err(...) printk(XENLOG_ERR __VA_ARGS__) > +struct xen_ubsan { int in_ubsan; }; > +static DEFINE_PER_CPU(struct xen_ubsan[1], in_ubsan); > +#undef current > +#define current this_cpu(in_ubsan) > +#define dump_stack dump_execution_state > +#define u64 long long unsigned int > +#define s64 long long int Wasn't it agreed to make these uint64_t and int64_t? Jan
On 09/10/17 15:36, Jan Beulich wrote: >>>> On 09.10.17 at 16:11, <wei.liu2@citrix.com> wrote: >> --- a/xen/common/ubsan/ubsan.c >> +++ b/xen/common/ubsan/ubsan.c >> @@ -10,13 +10,18 @@ >> * >> */ >> >> -#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/spinlock.h> >> +#include <xen/percpu.h> >> + >> +#define __noreturn noreturn >> +#define pr_err(...) printk(XENLOG_ERR __VA_ARGS__) >> +struct xen_ubsan { int in_ubsan; }; >> +static DEFINE_PER_CPU(struct xen_ubsan[1], in_ubsan); >> +#undef current >> +#define current this_cpu(in_ubsan) >> +#define dump_stack dump_execution_state >> +#define u64 long long unsigned int >> +#define s64 long long int > Wasn't it agreed to make these uint64_t and int64_t? Sadly that doesn't work, because of the explicit casts used in the sprintf(). Depending on how much we tolerate modifying the Linux code, these can be removed if we change to using appropriate PRIx64 formatters instead. However, I would prefer to avoid changing the Linux code if possible. ~Andrew
On Mon, Oct 09, 2017 at 03:38:55PM +0100, Andrew Cooper wrote: > On 09/10/17 15:36, Jan Beulich wrote: > >>>> On 09.10.17 at 16:11, <wei.liu2@citrix.com> wrote: > >> --- a/xen/common/ubsan/ubsan.c > >> +++ b/xen/common/ubsan/ubsan.c > >> @@ -10,13 +10,18 @@ > >> * > >> */ > >> > >> -#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/spinlock.h> > >> +#include <xen/percpu.h> > >> + > >> +#define __noreturn noreturn > >> +#define pr_err(...) printk(XENLOG_ERR __VA_ARGS__) > >> +struct xen_ubsan { int in_ubsan; }; > >> +static DEFINE_PER_CPU(struct xen_ubsan[1], in_ubsan); > >> +#undef current > >> +#define current this_cpu(in_ubsan) > >> +#define dump_stack dump_execution_state > >> +#define u64 long long unsigned int > >> +#define s64 long long int > > Wasn't it agreed to make these uint64_t and int64_t? > No, not yet. > Sadly that doesn't work, because of the explicit casts used in the > sprintf(). > > Depending on how much we tolerate modifying the Linux code, these can be > removed if we change to using appropriate PRIx64 formatters instead. > However, I would prefer to avoid changing the Linux code if possible. +1 for this.
>>> On 09.10.17 at 16:38, <andrew.cooper3@citrix.com> wrote: > On 09/10/17 15:36, Jan Beulich wrote: >>>>> On 09.10.17 at 16:11, <wei.liu2@citrix.com> wrote: >>> --- a/xen/common/ubsan/ubsan.c >>> +++ b/xen/common/ubsan/ubsan.c >>> @@ -10,13 +10,18 @@ >>> * >>> */ >>> >>> -#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/spinlock.h> >>> +#include <xen/percpu.h> >>> + >>> +#define __noreturn noreturn >>> +#define pr_err(...) printk(XENLOG_ERR __VA_ARGS__) >>> +struct xen_ubsan { int in_ubsan; }; >>> +static DEFINE_PER_CPU(struct xen_ubsan[1], in_ubsan); >>> +#undef current >>> +#define current this_cpu(in_ubsan) >>> +#define dump_stack dump_execution_state >>> +#define u64 long long unsigned int >>> +#define s64 long long int >> Wasn't it agreed to make these uint64_t and int64_t? > > Sadly that doesn't work, because of the explicit casts used in the > sprintf(). > > Depending on how much we tolerate modifying the Linux code, these can be > removed if we change to using appropriate PRIx64 formatters instead. > However, I would prefer to avoid changing the Linux code if possible. Ah, yes, that's good enough a reason. Together with Julien's reply then Acked-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/Kconfig b/xen/Kconfig index 65d491d776..ea7229ad1f 100644 --- a/xen/Kconfig +++ b/xen/Kconfig @@ -38,4 +38,10 @@ config LTO If unsure, say N. +# +# For architectures that know their compiler __int128 support is sound +# +config ARCH_SUPPORTS_INT128 + bool + source "Kconfig.debug" diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug index 195d504147..3329c75bfd 100644 --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -121,6 +121,16 @@ 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 HAS_UBSAN + ---help--- + Enable undefined behaviour sanitizer. It uses compiler to insert code + snippets so that undefined behaviours in C are detected during runtime. + This requires a UBSAN capable compiler and it is a debug only feature. + + If unsure, say N here. + endif # DEBUG || EXPERT endmenu diff --git a/xen/Rules.mk b/xen/Rules.mk index cafc67b86e..2659f8a4d1 100644 --- 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 + ifeq ($(CONFIG_LTO),y) CFLAGS += -flto LDFLAGS-$(clang) += -plugin LLVMgold.so diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 30c2769684..64955dc017 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -5,6 +5,7 @@ config X86 def_bool y select ACPI select ACPI_LEGACY_TABLES_LOOKUP + select ARCH_SUPPORTS_INT128 select COMPAT select CORE_PARKING select HAS_ALTERNATIVE @@ -21,6 +22,7 @@ config X86 select HAS_PASSTHROUGH select HAS_PCI select HAS_PDX + select HAS_UBSAN select NUMA select VGA diff --git a/xen/common/Kconfig b/xen/common/Kconfig index e9bb849298..103ef44cb5 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -32,6 +32,9 @@ config HAS_MEM_SHARING config HAS_PDX bool +config HAS_UBSAN + bool + config HAS_KEXEC bool diff --git a/xen/common/Makefile b/xen/common/Makefile index 39e2614546..66cc2c8995 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -75,6 +75,7 @@ tmem-$(CONFIG_COMPAT) += compat/tmem_xen.o obj-$(CONFIG_TMEM) += $(tmem-y) subdir-$(CONFIG_GCOV) += gcov +subdir-$(CONFIG_UBSAN) += ubsan subdir-y += libelf subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c index 685b4de0d6..fbe568562a 100644 --- a/xen/common/ubsan/ubsan.c +++ b/xen/common/ubsan/ubsan.c @@ -10,13 +10,18 @@ * */ -#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/spinlock.h> +#include <xen/percpu.h> + +#define __noreturn noreturn +#define pr_err(...) printk(XENLOG_ERR __VA_ARGS__) +struct xen_ubsan { int in_ubsan; }; +static DEFINE_PER_CPU(struct xen_ubsan[1], in_ubsan); +#undef current +#define current this_cpu(in_ubsan) +#define dump_stack dump_execution_state +#define u64 long long unsigned int +#define s64 long long int #include "ubsan.h"
Make the following changes: 1. Introduce CONFIG_UBSAN and other auxiliary options. 2. Introduce Build system rune to filter objects. 3. Make ubsan.c build. Currently only x86 is supported. All init.o's are filtered out because of limitation in the build system. There is no user of noubsan-y yet but it is worth keeping to ease future development. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- xen/Kconfig | 6 ++++++ xen/Kconfig.debug | 10 ++++++++++ xen/Rules.mk | 4 ++++ xen/arch/x86/Kconfig | 2 ++ xen/common/Kconfig | 3 +++ xen/common/Makefile | 1 + xen/common/ubsan/ubsan.c | 19 ++++++++++++------- 7 files changed, 38 insertions(+), 7 deletions(-)