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

[Jan Beulich <jbeulich@xxxxxxxx>]

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".

Jan