Re: [PATCH] ubsan: Introduce CONFIG_UBSAN_FATAL to panic on UBSAN failure

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 28 Nov 2023, Julien Grall wrote:
> On 28/11/2023 18:15, Michal Orzel wrote:
> > On 28/11/2023 18:09, Julien Grall wrote:
> > > On 28/11/2023 18:00, Michal Orzel wrote:
> > > > On 28/11/2023 17:14, Julien Grall wrote:
> > > > > On 27/11/2023 15:41, Michal Orzel wrote:
> > > > > > Introduce the CONFIG_UBSAN_FATAL option to cater to scenarios where
> > > > > > prompt
> > > > > > attention to undefined behavior issues, notably during CI test runs,
> > > > > > is
> > > > > > essential. When enabled, this option causes Xen to panic upon
> > > > > > detecting
> > > > > > UBSAN failure (as the last step in ubsan_epilogue()).
> > > > > 
> > > > > I have mixed opinions on this patch. This would be a good one if we
> > > > > had
> > > > > a Xen mostly free from UBSAN behavior. But this is not the case at
> > > > > least
> > > > > on arm32. So we would end up to stop at the first error. IOW, we would
> > > > > need to fix the first error before we can see the next one.
> > > > Well, this patch introduces a config option disabled by default.
> > > 
> > > I understood this is disabled by default... I am pointing out that I am
> > > not convinced about the usefulness until we are at the stage where Xen
> > > is normally not reporting any USBAN error.
> > > 
> > > > If we end up enabling it for CI for reasons* stated by Andrew, then the
> > > > natural way
> > > > of handling such issues is to do the investigation locally.
> > > 
> > > This will not always be possible. One example is when you are only able
> > > to reproduce some of the USBAN errors on a specific HW.
> > > 
> > > > Then, you are not forced
> > > > to select this option and you can see all the UBSAN issues if you want.
> > > 
> > > See above, I got that point. I am mostly concerned about the implication
> > > in the CI right now.
> > > 
> > > > 
> > > > > 
> > > > > So I feel it would be best if the gitlab CI jobs actually check for
> > > > > USBAN in the logs and fail if there are any. With that, we can get the
> > > > > full list of UBSAN issues on each job.
> > > > Well, I prefer Andrew suggestion (both [1] and on IRC), hence this
> > > > patch.
> > > > 
> > > > *my plan was to first fix the UBSAN issues and then enable this option
> > > > for CI.
> > > 
> > > That would have been useful to describe your goal after "---". With that
> > > in mind, then I suggest to revisit this patch once all the UBSAN issues
> > > in a normal build are fixed.
> > But this patch does not enable this option for CI automatically, right?
> 
> Correct.
> 
> > Why are you so keen to push it back?
> 
> I have been pushing back because your commit message refers to the CI
> specifically and today this would not really work (read as I would not be
> happy if this option is enabled in the CI right now at least on arm32).
> 
> If you want to fail a CI job for UBSAN today, then we need to find a different
> approach so we can get the full list of UBSAN error rather than fixing one,
> retry and then next one.
> 
> > Is it because you see no point in this option other than for the upstream CI
> > loop?
> 
> Even in the upstream CI loop, I am a little unsure about this approach. At
> least, I feel I would want to see all the reports at once in the CI.
> 
> But this is not really a strong feeling.
> 
> > I find it useful on a day-to-day
> > Xen runs, and I would for sure enable it by default in my config not to miss
> > UBSAN failures.
> 
> Fair enough. I view USBAN issues the same a WARN_ON. They all need to be
> investigated. So now you have an inconsistent policy.
> 
> Are you are also intending to switch WARN_ON() to fatal? If not, then why
> would UBSAN warnings more important that the other warnings?

I think it would be useful to be able to turn WARN_ONs into BUG_ONs
selectively by component. We could turn all WARN_ONs into BUG_ONs but
that's a bit extreme.

It is true that this patch is a bit... "ad-hoc". But given its
simplicity it doesn't hurt and I think it is nice-to-have for UBSAN. So
I would go with that.

But if we want something more sophisticated, here is an idea. The
concept is that one could specify warn=arch/arm/domain_build.c in the
Xen command line to change a WARN_ON in arch/arm/domain_build.c into a
BUG. I tested it with a WARN_ON I added on purpose and it works.


diff --git a/xen/common/symbols.c b/xen/common/symbols.c
index 133b580768..c78d2963c3 100644
--- a/xen/common/symbols.c
+++ b/xen/common/symbols.c
@@ -20,6 +20,10 @@
 #include <public/platform.h>
 #include <xen/guest_access.h>
 #include <xen/errno.h>
+#include <xen/param.h>
+
+char opt_warn[30] = "";
+string_param("warn", opt_warn);
 
 #ifdef SYMBOLS_ORIGIN
 extern const unsigned int symbols_offsets[];
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 1793be5b6b..60e14cdb85 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -12,12 +12,19 @@
 #include <xen/xmalloc.h>
 #include <xen/string.h>
 
+extern char opt_warn[30];
+
 #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
 #define WARN_ON(p)  ({                  \
     bool ret_warn_on_ = (p);            \
                                         \
     if ( unlikely(ret_warn_on_) )       \
-        WARN();                         \
+    {                                   \
+        if ( !strcmp(__FILE__, opt_warn) ) \
+            BUG();                      \
+        else                            \
+            WARN();                     \
+    }                                   \
     unlikely(ret_warn_on_);             \
 })