Re: [PATCH] x86/mem_sharing: silence ubsan warning

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Jan 18, 2021 at 4:10 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 18.01.2021 02:38, Tamas K Lengyel wrote:
> > On Mon, Jan 4, 2021 at 12:21 PM Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
> >>
> >> On Mon, Jan 4, 2021 at 7:31 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> >> wrote:
> >>>
> >>> On 03/01/2021 18:47, Tamas K Lengyel wrote:
> >>>> Running Xen compiled with UBSAN produces a warning for mismatched size. 
> >>>> It's
> >>>> benign but this patch silences the warning.
> >>>>
> >>>> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> >>>> ---
> >>>>  xen/arch/x86/mm/mem_sharing.c | 5 ++++-
> >>>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/mm/mem_sharing.c 
> >>>> b/xen/arch/x86/mm/mem_sharing.c
> >>>> index c428fd16ce..6920077dbf 100644
> >>>> --- a/xen/arch/x86/mm/mem_sharing.c
> >>>> +++ b/xen/arch/x86/mm/mem_sharing.c
> >>>> @@ -1638,7 +1638,10 @@ static int fork_hap_allocation(struct domain *cd, 
> >>>> struct domain *d)
> >>>>      rc = hap_set_allocation(cd, mb << (20 - PAGE_SHIFT), &preempted);
> >>>>      paging_unlock(cd);
> >>>>
> >>>> -    return preempted ? -ERESTART : rc;
> >>>> +    if ( preempted )
> >>>> +        rc = -ERESTART;
> >>>> +
> >>>> +    return rc;
> >>>
> >>> I can't repro this at all, even with some simplified examples.
> >>>
> >>> -ERESTART is int (it is an enum constant in C files), as is rc, so I
> >>> can't spot a legitimate UBSAN complaint here.
> >>>
> >>> Which compiler, and/or do you have the exact complaint available?
> >>
> >> It was with gcc-7 on debian buster but can't recreate it after a make
> >> clean & make, it's now gone ¯\_(ツ)_/¯, seems like it was just a bad
> >> build. Sorry for the noise.
> >
> > In a recent build with gcc-10 I got the warning again:
> >
> > (XEN) 
> > ================================================================================
> > (XEN) UBSAN: Undefined behaviour in mem_sharing.c:1659:34
> > (XEN) load of value 6 is not a valid value for type '_Bool'
>
> Isn't this rather referring to the value found in "preempted"?
> After all neither 6 nor -6 is related to ERESTART. If so, your
> proposed change would paper over an issue elsewhere and the
> issue would be liable to show up again when the if() gains
> similar treatment to the conditional operator by the compiler.
>
> And indeed, looking at the two functions the issue appears to
> be that hap_set_allocation() only ever writes "true" to
> *preempted, while fork_hap_allocation() fails to initialize
> its variable to "false".

That would actually make sense and explain why the warning pops up
only intermittently. I suspected the ?: op was the culprit for some
bizarre compiler reason that was not entirely clear to me. Testing it
with this patch and not seeing the warning was most likely purely
chance that it got preempted and thus not trigger the warning. Thanks
for deciphering it! Will send v2 shortly.

Tamas