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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 28/11/2023 18:52, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> 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:
>>>> Hi Julien,
>>>>
>>>> On 28/11/2023 17:14, Julien Grall wrote:
>>>>>
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>> 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).
I mentioned CI as a noteworthy example. In no case, I wrote that this implies 
the immediate
enabling of this option for all the arches in CI. Especially, given that I'm 
aware of arm32 issues
as you might know.

> 
> 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?
WARN_ON() is placed by the developer to detect e.g. incorrect configuration. 
The fact that someone
decided to use WARN_ON and not BUG_ON means that this person did some 
investigation the result of
which suggests no critical consequence. For UBSAN, you can't always be sure 
(read undefined).
It might be at the same level as WARN_ON but can also result in unpredictable 
behavior leading to security issues.

That said, I do believe that we should also have option to panic on WARN().
As for this patch, Andrew provided Rb and Stefano is happy with it. Do you want 
more people to vote?

~Michal