Messages in this thread |  | | From | Nicolai Stange <> | Subject | Re: Build warnings in Xen 5.15.y and 5.10.y with retbleed backports | Date | Sat, 16 Jul 2022 18:35:29 +0200 |
| |
Hi,
I see a patch for this has been queued up for 5.10 already ([1]), I'm just sharing my findings in support of this patch here -- it doesn't merely exchange one warning for another, but fixes a real issue and should perhaps get applied to other stable branches as well.
TL;DR: for this particular warning, objtool would exit early and fail to create any .orc_unwind* ELF sections for head_64.o, which are consumed by the ORC unwinder at runtime.
Boris Ostrovsky <boris.ostrovsky@oracle.com> writes:
> On 7/12/22 3:31 PM, Greg KH wrote: >> On Tue, Jul 12, 2022 at 03:19:39PM -0400, Boris Ostrovsky wrote: >>> >>> On 7/12/22 12:38 PM, Greg KH wrote: >>>> Hi all, >>>> >>>> I'm seeing the following build warning: >>>> arch/x86/kernel/head_64.o: warning: objtool: xen_hypercall_mmu_update(): can't find starting instruction >>>> in the 5.15.y and 5.10.y retbleed backports.
The reason for this is that with RET being multibyte, it can cross those "xen_hypecall_*" symbol boundaries, because ...
>>>> >>>> I don't know why just this one hypercall is being called out by objtool, >>>> and this warning isn't in 5.18 and Linus's tree due to I think commit >>>> 5b2fc51576ef ("x86/ibt,xen: Sprinkle the ENDBR") being there. >>>> >>>> But, is this a ret call that we "forgot" here? It's a "real" ret in >>>> Linus's branch: >>>> >>>> .pushsection .noinstr.text, "ax" >>>> .balign PAGE_SIZE >>>> SYM_CODE_START(hypercall_page) >>>> .rept (PAGE_SIZE / 32) >>>> UNWIND_HINT_FUNC >>>> ANNOTATE_NOENDBR >>>> ANNOTATE_UNRET_SAFE >>>> ret >>>> /* >>>> * Xen will write the hypercall page, and sort out ENDBR. >>>> */ >>>> .skip 31, 0xcc >>>> .endr >>>> >>>> while 5.15.y and older has: >>>> .pushsection .text >>>> .balign PAGE_SIZE >>>> SYM_CODE_START(hypercall_page) >>>> .rept (PAGE_SIZE / 32) >>>> UNWIND_HINT_FUNC >>>> .skip 31, 0x90
... the "31" is no longer correct, ...
>>>> ANNOTATE_UNRET_SAFE >>>> RET
... as with RET occupying more than one byte, the resulting hypercall entry's total size won't add up to 32 anymore.
Note that those xen_hypercall_* symbols' values are getting statically calculated as 'hypercall page + n * 32' in the HYPERCALL() #define from xen-head.S. So there's a mismatch and with RET == 'ret; int3', the resulting .text effectively becomes
101e: 90 nop 101f: c3 ret
0000000000001020 <xen_hypercall_mmu_update>: 1020: cc int3 1021: 90 nop 1022: 90 nop
This is probably already not what has been intended, but because 'ret' and 'int3' both are single-byte encoded, objtool would still be able to find at least some "starting instruction" at this point.
But with RET == 'jmp __x86_return_thunk', it becomes
101e: 90 nop 101f: e9 .byte 0xe9
0000000000001020 <xen_hypercall_mmu_update>: 1020: 00 00 add %al,(%rax) 1022: 00 00 add %al,(%rax) 1024: 90 nop
Here the 'e9 00 00 00 00' jmp crosses the symbol boundary and objtool errors out.
>>>> .endr >>>> >>>> So should the "ret" remain or be turned into "RET" in mainline right >>>> now? >>> >>> >>> It doesn't matter --- this is overwritten by the hypervisor during >>> initialization when Xen fills in actual hypercall code.
It does makes a difference though: even though objtool reports only a warning, it still exits early in this particular case and won't create any of the .orc_unwind* or .return_sites sections for head_64.o as it's supposed to.
The significance of not having .orc_unwind* for head_64.o is that the reliable stacktracing implementation would mark the swapper tasks' stacktraces as unreliable at runtime, because the ORC unwinder would fail to recognize their final secondary_startup_64() from head_64.o as being the end. Note that livepatching relies on reliable stacktraces when transitioning tasks.
>>> >>> >>> So f4b4bc10b0b85ec66f1a9bf5dddf475e6695b6d2 added 'ret' to make objtool happy and then 14b476e07fab6 replaced 'ret' with RET as part of SLS fixes. The latter was not really necessary but harmless. >>> >>> >>> So it can be 'ret', RET, or anything else that tools don't complain about. It will not be executed. >> Cool, thanks. >> But what about the objtool warning that I now see? Is that "real"? > > > > It's not real in the sense that the code there is not real, it will be overwritten. (Originally the whole page was 'nop's) > > > I am getting a different error BTW: > > arch/x86/kernel/head_64.o: warning: objtool: .text+0x5: unreachable instruction >
I think this one is (mostly?) harmless, at least as as far as the .orc_unwind* generation is concerned. Josh would know more.
Thanks,
Nicolai
[1] https://lore.kernel.org/r/Ys+8ZYxkDmSCcDWv@kroah.com
> > >> I don't run any Xen systems, so I can't test any of this myself. > > > You can't test any changes to that code --- it is rewritten when Xen guest is running. > > > We probably do want to shut up objtool. Josh, any suggestions? > > > -boris >
-- SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman (HRB 36809, AG Nürnberg)
|  |