diff mbox series

x86/mem_sharing: silence ubsan warning

Message ID 7536d7bd92337933e6e23be359ca981deab50016.1609699565.git.tamas@tklengyel.com (mailing list archive)
State New
Headers show
Series x86/mem_sharing: silence ubsan warning | expand

Commit Message

Tamas K Lengyel Jan. 3, 2021, 6:47 p.m. UTC
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@tklengyel.com>
---
 xen/arch/x86/mm/mem_sharing.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Andrew Cooper Jan. 4, 2021, 12:31 p.m. UTC | #1
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@tklengyel.com>
> ---
>  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?

~Andrew
Tamas K Lengyel Jan. 4, 2021, 5:21 p.m. UTC | #2
On Mon, Jan 4, 2021 at 7:31 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@tklengyel.com>
> > ---
> >  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.

Tamas
Tamas K Lengyel Jan. 18, 2021, 1:38 a.m. UTC | #3
On Mon, Jan 4, 2021 at 12:21 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> On Mon, Jan 4, 2021 at 7:31 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@tklengyel.com>
> > > ---
> > >  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'
(XEN) ----[ Xen-4.15-unstable  x86_64  debug=y ubsan=y  Not tainted ]----
(XEN) CPU:    0
(XEN) RIP:    e008:[<ffff82d040321271>]
common/ubsan/ubsan.c#ubsan_epilogue+0x5/0xc6
(XEN) RFLAGS: 0000000000010086   CONTEXT: hypervisor (d0v0)
(XEN) rax: 0000000000000000   rbx: ffff83007fc7f930   rcx: 0000000000000000
(XEN) rdx: ffff83007fc7ffd0   rsi: 000000000000000a   rdi: ffff83007fc7f930
(XEN) rbp: 0000000000000006   rsp: ffff83007fc7f8f0   r8:  00000000ffffffff
(XEN) r9:  0000000000000000   r10: ffff83007fc7f908   r11: 0000000000000000
(XEN) r12: ffff83036bb58000   r13: ffff830241ed0000   r14: 0000000000000006
(XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4: 0000000000372660
(XEN) cr3: 00000002466ab000   cr2: 00007f32f50a514d
(XEN) fsb: 00007f32f4e6c2c0   gsb: ffff8881f2800000   gss: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around <ffff82d040321271>
(common/ubsan/ubsan.c#ubsan_epilogue+0x5/0xc6):
(XEN)  00 eb c2 55 53 48 89 fb <0f> 0b 48 8d 3d ee 22 53 00 e8 ec 2a 00 00 48 85
(XEN) Xen stack trace from rsp=ffff83007fc7f8f0:
(XEN)    ffff82d040c2f7b2 0000000000000006 ffff82d0403222a2 0000000000240036
(XEN)    ffff83036bb58748 00007f32f50e9010 0000000000000000 ffff83036bb586a0
(XEN)    0000000000000202 00007f32f50e9010 0000000000000000 ffff82d0404c9101
(XEN)    0000004100000000 ffff83036bb586a0 ffff82d040f88910 ffff82d000000040
(XEN)    ffff83007fc7fb30 ffff82d040539437 ffff82004001dfb8 0000000000000001
(XEN)    ffff83007fc7fb30 ffff83007fc7fa88 0000000000000206 ffff830241ed0000
(XEN)    0000000000000000 0000000000000003 ffff83036bb58000 0000000000020009
(XEN)    0000000000030001 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000016 0000000000000016 ffff830361f69000
(XEN)    deadbeefdeadf00d 0000000000000000 00007f32f50e9010 ffff82d040583da6
(XEN)    00007f32f50e9010 0000000000000016 0000000000000008 ffff82d040f8ed40
(XEN)    0000000000423000 0000000000800163 00000000000000b0 0000000000000016
(XEN)    0000000000000016 ffff830361f69000 deadbeefdeadf00d 0000000000000000
(XEN)    00007f32f50e9010 ffff82d040565a1a 00007f32f50e9010 00007d2fbf0716c8
(XEN)    0000000000ff5800 ffff82d040f8e940 ffff82d040f8ed40 0000000000000100
(XEN)    0000000000000001 0000000000000028 0000000000000001 0000000000000028
(XEN)    0000000000000000 ffff82e006d73f80 0000000000000001 0000000000000016
(XEN)    0000000000000016 ffff830361f69000 deadbeefdeadf00d 0000000000000000
(XEN)    00007f32f50e9010 ffff82d040268334 0000000000000100 0000000000000000
(XEN)    000000000012ca7c 0000000000000000 0000000000000000 ffff830364a40000
(XEN) Xen call trace:
(XEN)    [<ffff82d040321271>] R common/ubsan/ubsan.c#ubsan_epilogue+0x5/0xc6
(XEN)    [<ffff82d0403222a2>] S __ubsan_handle_load_invalid_value+0x92/0xc9
(XEN)    [<ffff82d0404c9101>] S mem_sharing_memop+0x4fc4/0x5e4c
(XEN)    [<ffff82d040539437>] S
arch/x86/domain_page.c#mapcache_current_vcpu+0x47/0x3ea
(XEN)    [<ffff82d040583da6>] S subarch_memory_op+0xc4f/0xc80
(XEN)    [<ffff82d040565a1a>] S arch_memory_op+0x45/0x2bd9
(XEN)    [<ffff82d040268334>] S do_memory_op+0x4ce/0x7330
(XEN)    [<ffff82d0402a6ad2>] S xmem_pool_alloc+0xd61/0x1325
(XEN)    [<ffff82d04028b8ec>] S common/timer.c#timer_lock+0x156/0x694
(XEN)    [<ffff82d0402e21d8>] S
common/sched/credit2.c#replenish_domain_budget+0/0x3ae
(XEN)    [<ffff82d040309c81>] S sched_init_domain+0x14e/0x47c
(XEN)    [<ffff82d04020b05c>] S domain_create+0x70a/0xd55
(XEN)    [<ffff82d0402b1631>] S domctl_lock_release+0x6b/0xd9
(XEN)    [<ffff82d0402b5f20>] S do_domctl+0x4819/0x49f5
(XEN)    [<ffff82d04057ad4e>] S do_mmu_update+0x34ec/0x36a1
(XEN)    [<ffff82d040564913>] S update_cr3+0x8e/0x1b0
(XEN)    [<ffff82d040507b55>] S arch/x86/pv/domain.c#_toggle_guest_pt+0x34/0x177
(XEN)    [<ffff82d040508edd>] S toggle_guest_mode+0x143/0x45f
(XEN)    [<ffff82d04051534d>] S do_iret+0x206/0x530
(XEN)    [<ffff82d040513ba6>] S pv_hypercall+0x866/0xef3
(XEN)    [<ffff82d040507b55>] S arch/x86/pv/domain.c#_toggle_guest_pt+0x34/0x177
(XEN)    [<ffff82d040508edd>] S toggle_guest_mode+0x143/0x45f
(XEN)    [<ffff82d04062c457>] S lstar_enter+0x127/0x130
(XEN)
(XEN) ================================================================================

Tamas
Jan Beulich Jan. 18, 2021, 9:09 a.m. UTC | #4
On 18.01.2021 02:38, Tamas K Lengyel wrote:
> On Mon, Jan 4, 2021 at 12:21 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>
>> On Mon, Jan 4, 2021 at 7:31 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@tklengyel.com>
>>>> ---
>>>>  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
Tamas K Lengyel Jan. 18, 2021, 1:47 p.m. UTC | #5
On Mon, Jan 18, 2021 at 4:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 18.01.2021 02:38, Tamas K Lengyel wrote:
> > On Mon, Jan 4, 2021 at 12:21 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:
> >>
> >> On Mon, Jan 4, 2021 at 7:31 AM Andrew Cooper <andrew.cooper3@citrix.com> 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@tklengyel.com>
> >>>> ---
> >>>>  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
diff mbox series

Patch

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;
 }
 
 static void copy_tsc(struct domain *cd, struct domain *d)