| Message ID | 20171017113644.31866-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.10.17 at 13:36, <roger.pau@citrix.com> wrote: > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -120,7 +120,7 @@ $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) > $(extra-y)): CFLAGS += - > endif > > ifeq ($(CONFIG_UBSAN),y) > -$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined > +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined -fno-sanitize=alignment > endif As was said by I think Andrew on irc, this is fine for x86 but not fine for ARM. Even if we enable UBSAN for x86 only right now, I think such option additions should be done in an arch-specific way. Jan
On Tue, Oct 17, 2017 at 12:36:44PM +0100, Roger Pau Monne wrote: > Currently there are many offenders of the unaligned access checks, > which makes booting with the unaligned check a PVH Dom0 impossible. > > The main offenders seem to be the ACPI code, the VMX code and > specially the intremap code (set_ire_sid). > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Tim Deegan <tim@xen.org> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Julien Grall <julien.grall@arm.com> > --- > I'm not sure whether we prefer to fix the offenders, or just disable > the alignment wholesale. In any case if we decide to disable the > check, the patch should have vary low impact, and hence should be > committed to 4.10 on the base that it only affects ubsan, which is not > enabled by default and not to be used on production systems. I would very much like to fix the offenders but if the fixes turn out to be cumbersome, so be it. What is wrong to leave this enabled? Each location is reported once, right?
On Tue, Oct 17, 2017 at 01:56:18PM +0100, Wei Liu wrote: > On Tue, Oct 17, 2017 at 12:36:44PM +0100, Roger Pau Monne wrote: > > Currently there are many offenders of the unaligned access checks, > > which makes booting with the unaligned check a PVH Dom0 impossible. > > > > The main offenders seem to be the ACPI code, the VMX code and > > specially the intremap code (set_ire_sid). > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > Cc: Tim Deegan <tim@xen.org> > > Cc: Wei Liu <wei.liu2@citrix.com> > > Cc: Julien Grall <julien.grall@arm.com> > > --- > > I'm not sure whether we prefer to fix the offenders, or just disable > > the alignment wholesale. In any case if we decide to disable the > > check, the patch should have vary low impact, and hence should be > > committed to 4.10 on the base that it only affects ubsan, which is not > > enabled by default and not to be used on production systems. > > I would very much like to fix the offenders but if the fixes turn out to > be cumbersome, so be it. > > What is wrong to leave this enabled? Each location is reported once, > right? With clang it's reported every time it's hit AFAICT (certainly more than once). Roger.
On Tue, Oct 17, 2017 at 01:58:47PM +0100, Roger Pau Monné wrote: > On Tue, Oct 17, 2017 at 01:56:18PM +0100, Wei Liu wrote: > > On Tue, Oct 17, 2017 at 12:36:44PM +0100, Roger Pau Monne wrote: > > > Currently there are many offenders of the unaligned access checks, > > > which makes booting with the unaligned check a PVH Dom0 impossible. > > > > > > The main offenders seem to be the ACPI code, the VMX code and > > > specially the intremap code (set_ire_sid). > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > --- > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > Cc: Jan Beulich <jbeulich@suse.com> > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > > Cc: Tim Deegan <tim@xen.org> > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > Cc: Julien Grall <julien.grall@arm.com> > > > --- > > > I'm not sure whether we prefer to fix the offenders, or just disable > > > the alignment wholesale. In any case if we decide to disable the > > > check, the patch should have vary low impact, and hence should be > > > committed to 4.10 on the base that it only affects ubsan, which is not > > > enabled by default and not to be used on production systems. > > > > I would very much like to fix the offenders but if the fixes turn out to > > be cumbersome, so be it. > > > > What is wrong to leave this enabled? Each location is reported once, > > right? > > With clang it's reported every time it's hit AFAICT (certainly more > than once). > It could also be the case that some functions are inlined so they could appear multiple times, i.e. they have different instances of struct location? I've also seen something like that before. The code in ubsan.c already deals with setting the reported bit so I suspect after all the instances have been marked reported Xen should run fine?
On 17/10/17 13:58, Roger Pau Monné wrote: > On Tue, Oct 17, 2017 at 01:56:18PM +0100, Wei Liu wrote: >> On Tue, Oct 17, 2017 at 12:36:44PM +0100, Roger Pau Monne wrote: >>> Currently there are many offenders of the unaligned access checks, >>> which makes booting with the unaligned check a PVH Dom0 impossible. >>> >>> The main offenders seem to be the ACPI code, the VMX code and >>> specially the intremap code (set_ire_sid). >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>> Cc: George Dunlap <George.Dunlap@eu.citrix.com> >>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>> Cc: Jan Beulich <jbeulich@suse.com> >>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> Cc: Tim Deegan <tim@xen.org> >>> Cc: Wei Liu <wei.liu2@citrix.com> >>> Cc: Julien Grall <julien.grall@arm.com> >>> --- >>> I'm not sure whether we prefer to fix the offenders, or just disable >>> the alignment wholesale. In any case if we decide to disable the >>> check, the patch should have vary low impact, and hence should be >>> committed to 4.10 on the base that it only affects ubsan, which is not >>> enabled by default and not to be used on production systems. >> I would very much like to fix the offenders but if the fixes turn out to >> be cumbersome, so be it. >> >> What is wrong to leave this enabled? Each location is reported once, >> right? > With clang it's reported every time it's hit AFAICT (certainly more > than once). suppress_report() is supposed to take care of this. Is Clang feeding in different ->location information each time through? ~Andrew
diff --git a/xen/Rules.mk b/xen/Rules.mk index 2659f8a4d1..1472adf1d6 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -120,7 +120,7 @@ $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += - endif ifeq ($(CONFIG_UBSAN),y) -$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined -fno-sanitize=alignment endif ifeq ($(CONFIG_LTO),y)
Currently there are many offenders of the unaligned access checks, which makes booting with the unaligned check a PVH Dom0 impossible. The main offenders seem to be the ACPI code, the VMX code and specially the intremap code (set_ire_sid). Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Julien Grall <julien.grall@arm.com> --- I'm not sure whether we prefer to fix the offenders, or just disable the alignment wholesale. In any case if we decide to disable the check, the patch should have vary low impact, and hence should be committed to 4.10 on the base that it only affects ubsan, which is not enabled by default and not to be used on production systems. --- xen/Rules.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)