| Message ID | 7536d7bd92337933e6e23be359ca981deab50016.1609699565.git.tamas@tklengyel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mem_sharing: silence ubsan warning | expand |
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
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
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
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
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 --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)
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(-)