LinuxLists.cc - WARNING: can't access registers at asm_common

2020-11-06 06:06:58

Subject: WARNING: can't access registers at asm_common_interrupt

Greetings,

I observe "WARNING: can't access registers at asm_common_interrupt+0x1e/0x40"
in my kernel test system repeatedly, which is printed by unwind_next_frame() in
"arch/x86/kernel/unwind_orc.c". Syzbot already reported that [1]. Similar
warning was reported and discussed [2], but I suppose the cause is not yet
clarified.

The warning was observed with v5.10-rc2 and older tags. I bisected and found
that the commit 044d0d6de9f5 ("lockdep: Only trace IRQ edges") in v5.9-rc3
triggered the warning. Reverting that from 5.10-rc2, the warning disappeared.
May I ask comment by expertise on CC how this commit can relate to the warning?

The test condition to reproduce the warning is rather unique (blktests,
dm-linear and ZNS device emulation by QEMU). If any action is suggested for
further analysis, I'm willing to take it with my test system.

Wish this report helps.

[1] https://lkml.org/lkml/2020/9/6/231
[2] https://lkml.org/lkml/2020/9/8/1538

--
Best Regards,
Shin'ichiro Kawasaki

2020-11-06 18:11:26

by Josh Poimboeuf

[permalink] [raw]

On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
<[email protected]> wrote:
>
> On 11/11/2020 20:15, Josh Poimboeuf wrote:
> > On Wed, Nov 11, 2020 at 09:07:30PM +0100, Peter Zijlstra wrote:
> >> On Wed, Nov 11, 2020 at 01:59:00PM -0600, Josh Poimboeuf wrote:
> >>> On Wed, Nov 11, 2020 at 08:42:06PM +0100, Peter Zijlstra wrote:
> >>>>> Would objtool have an easier time coping if this were implemented in
> >>>>> terms of a static call?
> >>>> I doubt it, the big problem is that there is no visibility into the
> >>>> actual alternative text. Runtime patching fragments into static call
> >>>> would have the exact same problem.
> >>>>
> >>>> Something that _might_ maybe work is trying to morph the immediate
> >>>> fragments into an alternative. That is, instead of this:
> >>>>
> >>>> static inline notrace unsigned long arch_local_save_flags(void)
> >>>> {
> >>>> return PVOP_CALLEE0(unsigned long, irq.save_fl);
> >>>> }
> >>>>
> >>>> Write it something like:
> >>>>
> >>>> static inline notrace unsigned long arch_local_save_flags(void)
> >>>> {
> >>>> PVOP_CALL_ARGS;
> >>>> PVOP_TEST_NULL(irq.save_fl);
> >>>> asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> >>>> "PUSHF; POP _ASM_AX",
> >>>> X86_FEATURE_NATIVE)
> >>>> : CLBR_RET_REG, ASM_CALL_CONSTRAINT
> >>>> : paravirt_type(irq.save_fl.func),
> >>>> paravirt_clobber(PVOP_CALLEE_CLOBBERS)
> >>>> : "memory", "cc");
> >>>> return __eax;
> >>>> }
> >>>>
> >>>> And then we have to teach objtool how to deal with conflicting
> >>>> alternatives...
> >>>>
> >>>> That would remove most (all, if we can figure out a form that deals with
> >>>> the spinlock fragments) of paravirt_patch.c
> >>>>
> >>>> Hmm?
> >>> I was going to suggest something similar. Though I would try to take it
> >>> further and replace paravirt_patch_default() with static calls.
> >> Possible, we just need to be _really_ careful to not allow changing
> >> those static_call()s. So maybe we need DEFINE_STATIC_CALL_RO() which
> >> does a __ro_after_init on the whole thing.
> > But what if you want to live migrate to another hypervisor ;-)
>
> The same as what happens currently. The user gets to keep all the
> resulting pieces ;)
>
> >>> Either way it doesn't make objtool's job much easier. But it would be
> >>> nice to consolidate runtime patching mechanisms and get rid of
> >>> .parainstructions.
> >> I think the above (combining alternative and paravirt/static_call) does
> >> make objtool's job easier, since then we at least have the actual
> >> alternative instructions available to inspect, or am I mis-understanding
> >> things?
> > Right, it makes objtool's job a _little_ easier, since it already knows
> > how to read alternatives. But it still has to learn to deal with the
> > conflicting stack layouts.
>
> I suppose the needed abstraction is "these blocks will start and end
> with the same stack layout", while allowing the internals to diverge.
>

How much of this stuff is actually useful anymore? I'm wondering if
we can move most or all of this crud to
cpu_feature_enabled(X86_FEATURE_XENPV) and its asm equivalent. The
full list, annotated, appears to be:

const unsigned char irq_irq_disable[1];

This is CLI or CALL, right?

const unsigned char irq_irq_enable[1];

STI or CALL.

const unsigned char irq_save_fl[2];

PUSHF; POP %r/eax. I *think* I read the paravirt mess correctly and
this also turns into CALL.

const unsigned char mmu_read_cr2[3];
const unsigned char mmu_read_cr3[3];
const unsigned char mmu_write_cr3[3];

The write CR3 is so slow that I can't imagine us caring. Reading CR3
should already be fairly optimized because it's slow on old non-PV
hypervisors, too. Reading CR2 is rare and lives in asm. These also
appear to just switch between MOV and CALL, anyway.

const unsigned char irq_restore_fl[2];

Ugh, this one sucks. IMO it should be, for native and PV:

if (flags & X86_EFLAGS_IF) {
local_irq_enable(); /* or raw? */
} else {
if (some debugging option) {
WARN_ON_ONCE(save_fl() & X86_EFLAGS_IF);
}
}

POPF is slooooow.

const unsigned char cpu_wbinvd[2];

This is hilariously slow no matter what. static_call() or even just a
plain old indirect call should be fine.

const unsigned char cpu_usergs_sysret64[6];

This is in the asm and we shouldn't be doing it at all for Xen PV.
IOW we should just drop this patch site entirely. I can possibly find
some time to get rid of it, and maybe someone from Xen land can help.
I bet that we can gain a lot of perf on Xen PV by cleaning this up,
and I bet it will simplify everything.

const unsigned char cpu_swapgs[3];

This is SWAPGS or nop, unless I've missed some subtlety.

const unsigned char mov64[3];

This is some PTE magic, and I haven't deciphered it yet.

So I think there is at most one of these that wants anything more
complicated than a plain ALTERNATIVE. Any volunteers to make it so?
Juergen, if you do all of them except USERGS_SYSRET64, I hereby
volunteer to do that one.

BTW, if y'all want to live migrate between Xen PV and anything else,
you are nuts.

2020-11-14 09:20:19

On 14.11.20 19:10, Andy Lutomirski wrote:
> On Sat, Nov 14, 2020 at 1:16 AM Jürgen Groß <[email protected]> wrote:
>>
>> On 13.11.20 18:34, Andy Lutomirski wrote:
>>> On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
>>> <[email protected]> wrote:
>>>
>>> So I think there is at most one of these that wants anything more
>>> complicated than a plain ALTERNATIVE. Any volunteers to make it so?
>>> Juergen, if you do all of them except USERGS_SYSRET64, I hereby
>>> volunteer to do that one.
>>
>> Why is a plain alternative (either swapgs; sysretq or a jmp xen_sysret64
>> depending on X86_FEATURE_XENPV) no option?
>>
>> Its not as if this code would run before alternative patching.
>
> ALTERNATIVE would "work" in the sense that it would function and be
> just about as nonsensical as the current code. Fundamentally, Xen
> PV's sysret feature is not a drop-in replacement for SYSRET64, and
> pretending that it is seems unlikely to work well. I suspect that the
> current code is some combination of exceedingly slow, non-functional,
> and incorrect in subtle ways.
>
> We should just have a separate Xen PV exit path the same way we have a
> separate entry path in recent kernels. *This* is what I'm
> volunteering to do.

I don't think there is much work needed. Xen PV does basically a return
to user mode via IRET. I think it might work just to use
swapgs_restore_regs_and_return_to_usermode() unconditionally for Xen PV.

Juergen

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.06 kB)
OpenPGP_signature (505.00 B)
OpenPGP digital signature Download all attachments

2020-11-15 16:37:33

by Andy Lutomirski

[permalink] [raw]

Subject: Re: WARNING: can't access registers at asm_common_interrupt

> On Nov 14, 2020, at 10:33 PM, Jürgen Groß <[email protected]> wrote:
>
> On 14.11.20 19:10, Andy Lutomirski wrote:
>>> On Sat, Nov 14, 2020 at 1:16 AM Jürgen Groß <[email protected]> wrote:
>>>
>>> On 13.11.20 18:34, Andy Lutomirski wrote:
>>>> On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
>>>> <[email protected]> wrote:
>>>>
>>>> So I think there is at most one of these that wants anything more
>>>> complicated than a plain ALTERNATIVE. Any volunteers to make it so?
>>>> Juergen, if you do all of them except USERGS_SYSRET64, I hereby
>>>> volunteer to do that one.
>>>
>>> Why is a plain alternative (either swapgs; sysretq or a jmp xen_sysret64
>>> depending on X86_FEATURE_XENPV) no option?
>>>
>>> Its not as if this code would run before alternative patching.
>> ALTERNATIVE would "work" in the sense that it would function and be
>> just about as nonsensical as the current code. Fundamentally, Xen
>> PV's sysret feature is not a drop-in replacement for SYSRET64, and
>> pretending that it is seems unlikely to work well. I suspect that the
>> current code is some combination of exceedingly slow, non-functional,
>> and incorrect in subtle ways.
>> We should just have a separate Xen PV exit path the same way we have a
>> separate entry path in recent kernels. *This* is what I'm
>> volunteering to do.
>
> I don't think there is much work needed. Xen PV does basically a return
> to user mode via IRET. I think it might work just to use
> swapgs_restore_regs_and_return_to_usermode() unconditionally for Xen PV.
>

I’m quite confident that will work, but I was hoping to get it to work quickly too :)

>
> Juergen
> <OpenPGP_0xB0DE9DD628BF132F.asc>

2020-11-15 16:38:43

by Juergen Gross

[permalink] [raw]

Subject: Re: WARNING: can't access registers at asm_common_interrupt

On 15.11.20 17:05, Andy Lutomirski wrote:
>
>> On Nov 14, 2020, at 10:33 PM, Jürgen Groß <[email protected]> wrote:
>>
>> On 14.11.20 19:10, Andy Lutomirski wrote:
>>>> On Sat, Nov 14, 2020 at 1:16 AM Jürgen Groß <[email protected]> wrote:
>>>>
>>>> On 13.11.20 18:34, Andy Lutomirski wrote:
>>>>> On Wed, Nov 11, 2020 at 12:25 PM Andrew Cooper
>>>>> <[email protected]> wrote:
>>>>>
>>>>> So I think there is at most one of these that wants anything more
>>>>> complicated than a plain ALTERNATIVE. Any volunteers to make it so?
>>>>> Juergen, if you do all of them except USERGS_SYSRET64, I hereby
>>>>> volunteer to do that one.
>>>>
>>>> Why is a plain alternative (either swapgs; sysretq or a jmp xen_sysret64
>>>> depending on X86_FEATURE_XENPV) no option?
>>>>
>>>> Its not as if this code would run before alternative patching.
>>> ALTERNATIVE would "work" in the sense that it would function and be
>>> just about as nonsensical as the current code. Fundamentally, Xen
>>> PV's sysret feature is not a drop-in replacement for SYSRET64, and
>>> pretending that it is seems unlikely to work well. I suspect that the
>>> current code is some combination of exceedingly slow, non-functional,
>>> and incorrect in subtle ways.
>>> We should just have a separate Xen PV exit path the same way we have a
>>> separate entry path in recent kernels. *This* is what I'm
>>> volunteering to do.
>>
>> I don't think there is much work needed. Xen PV does basically a return
>> to user mode via IRET. I think it might work just to use
>> swapgs_restore_regs_and_return_to_usermode() unconditionally for Xen PV.
>>
>
> I’m quite confident that will work, but I was hoping to get it to work quickly too :)

The PV interface requires to use the iret hypercall, because there
is no sysret equivalent.

Juergen

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.06 kB)
OpenPGP_signature (505.00 B)
OpenPGP digital signature Download all attachments

2020-11-16 12:01:38

by Juergen Gross

[permalink] [raw]

Subject: Re: WARNING: can't access registers at asm_common_interrupt

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.06 kB)
OpenPGP_signature (505.00 B)
OpenPGP digital signature Download all attachments

2020-11-16 13:09:27

by Peter Zijlstra

[permalink] [raw]

Subject: Re: WARNING: can't access registers at asm_common_interrupt

On Mon, Nov 16, 2020 at 12:56:32PM +0100, J?rgen Gro? wrote:
> > > > > > > static inline notrace unsigned long arch_local_save_flags(void)
> > > > > > > {
> > > > > > > PVOP_CALL_ARGS;
> > > > > > > PVOP_TEST_NULL(irq.save_fl);
> > > > > > > asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
> > > > > > > "PUSHF; POP _ASM_AX",
> > > > > > > X86_FEATURE_NATIVE)
>
> I am wondering whether we really want a new feature (basically "not
> XENPV). We could use ~X86_FEATURE_XENPV and teach apply_alternatives()
> to understand negated features (yes, this limits the number of features
> to 32767, but I don't think this is a real problem for quite some time).
>
> Thoughts?

I went with the simple thing for now... If we ever want/need another
negative alternative I suppose we can do as you suggest...

I was still poking at objtool to actually dtrt though..

---
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dad350d42ecf..cc88f358bac5 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -237,6 +237,7 @@
#define X86_FEATURE_VMCALL ( 8*32+18) /* "" Hypervisor supports the VMCALL instruction */
#define X86_FEATURE_VMW_VMMCALL ( 8*32+19) /* "" VMware prefers VMMCALL hypercall instruction */
#define X86_FEATURE_SEV_ES ( 8*32+20) /* AMD Secure Encrypted Virtualization - Encrypted State */
+#define X86_FEATURE_NOT_XENPV ( 8*32+21) /* "" inverse of X86_FEATURE_XENPV */

/* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */
#define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index d25cc6830e89..e2a9d3e6b7ad 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -645,22 +645,56 @@ bool __raw_callee_save___native_vcpu_is_preempted(long cpu);
#ifdef CONFIG_PARAVIRT_XXL
static inline notrace unsigned long arch_local_save_flags(void)
{
- return PVOP_CALLEE0(unsigned long, irq.save_fl);
+ PVOP_CALL_ARGS;
+ PVOP_TEST_NULL(irq.save_fl);
+ asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
+ "pushf; pop %%" _ASM_AX ";",
+ X86_FEATURE_NOT_XENPV)
+ : PVOP_CALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
+ : paravirt_type(irq.save_fl.func),
+ paravirt_clobber(CLBR_RET_REG)
+ : "memory", "cc");
+ return __eax;
}

static inline notrace void arch_local_irq_restore(unsigned long f)
{
- PVOP_VCALLEE1(irq.restore_fl, f);
+ PVOP_CALL_ARGS;
+ PVOP_TEST_NULL(irq.restore_fl);
+ asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
+ "push %%" _ASM_ARG1 "; popf;",
+ X86_FEATURE_NOT_XENPV)
+ : PVOP_VCALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
+ : paravirt_type(irq.restore_fl.func),
+ paravirt_clobber(CLBR_RET_REG),
+ PVOP_CALL_ARG1(f)
+ : "memory", "cc");
}

static inline notrace void arch_local_irq_disable(void)
{
- PVOP_VCALLEE0(irq.irq_disable);
+ PVOP_CALL_ARGS;
+ PVOP_TEST_NULL(irq.irq_disable);
+ asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
+ "cli;",
+ X86_FEATURE_NOT_XENPV)
+ : PVOP_VCALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
+ : paravirt_type(irq.irq_disable.func),
+ paravirt_clobber(CLBR_RET_REG)
+ : "memory", "cc");
}

static inline notrace void arch_local_irq_enable(void)
{
- PVOP_VCALLEE0(irq.irq_enable);
+ PVOP_CALL_ARGS;
+ PVOP_TEST_NULL(irq.irq_enable);
+ asm_inline volatile(ALTERNATIVE(paravirt_alt(PARAVIRT_CALL),
+ "sti;",
+ X86_FEATURE_NOT_XENPV)
+ : PVOP_VCALLEE_CLOBBERS, ASM_CALL_CONSTRAINT
+ : paravirt_type(irq.irq_enable.func),
+ paravirt_clobber(CLBR_RET_REG)
+ : "memory", "cc");
}

static inline notrace unsigned long arch_local_irq_save(void)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 2400ad62f330..5bd8f5503652 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -723,6 +723,19 @@ void __init alternative_instructions(void)
* patching.
*/

+ if (!boot_cpu_has(X86_FEATURE_XENPV))
+ setup_force_cpu_cap(X86_FEATURE_NOT_XENPV);
+
+ /*
+ * First patch paravirt functions, such that we overwrite the indirect
+ * call with the direct call.
+ */
+ apply_paravirt(__parainstructions, __parainstructions_end);
+
+ /*
+ * Then patch alternatives, such that those paravirt calls that are in
+ * alternatives can be overwritten by their immediate fragments.
+ */
apply_alternatives(__alt_instructions, __alt_instructions_end);

#ifdef CONFIG_SMP
@@ -741,8 +754,6 @@ void __init alternative_instructions(void)
}
#endif

- apply_paravirt(__parainstructions, __parainstructions_end);
-
restart_nmi();
alternatives_patched = 1;
}
diff --git a/arch/x86/kernel/paravirt_patch.c b/arch/x86/kernel/paravirt_patch.c
index ace6e334cb39..867498db53ad 100644
--- a/arch/x86/kernel/paravirt_patch.c
+++ b/arch/x86/kernel/paravirt_patch.c
@@ -33,13 +33,9 @@ struct patch_xxl {
};

static const struct patch_xxl patch_data_xxl = {
- .irq_irq_disable = { 0xfa }, // cli
- .irq_irq_enable = { 0xfb }, // sti
- .irq_save_fl = { 0x9c, 0x58 }, // pushf; pop %[re]ax
.mmu_read_cr2 = { 0x0f, 0x20, 0xd0 }, // mov %cr2, %[re]ax
.mmu_read_cr3 = { 0x0f, 0x20, 0xd8 }, // mov %cr3, %[re]ax
.mmu_write_cr3 = { 0x0f, 0x22, 0xdf }, // mov %rdi, %cr3
- .irq_restore_fl = { 0x57, 0x9d }, // push %rdi; popfq
.cpu_wbinvd = { 0x0f, 0x09 }, // wbinvd
.cpu_usergs_sysret64 = { 0x0f, 0x01, 0xf8,
0x48, 0x0f, 0x07 }, // swapgs; sysretq
@@ -76,11 +72,6 @@ unsigned int native_patch(u8 type, void *insn_buff, unsigned long addr,
switch (type) {

#ifdef CONFIG_PARAVIRT_XXL
- PATCH_CASE(irq, restore_fl, xxl, insn_buff, len);
- PATCH_CASE(irq, save_fl, xxl, insn_buff, len);
- PATCH_CASE(irq, irq_enable, xxl, insn_buff, len);
- PATCH_CASE(irq, irq_disable, xxl, insn_buff, len);
-
PATCH_CASE(mmu, read_cr2, xxl, insn_buff, len);
PATCH_CASE(mmu, read_cr3, xxl, insn_buff, len);
PATCH_CASE(mmu, write_cr3, xxl, insn_buff, len);

Attachments:

(No filename) (6.20 kB)
signature.asc (849.00 B)
Download all attachments

2020-11-18 06:52:00

On 19.11.20 13:01, Peter Zijlstra wrote:
> On Thu, Nov 19, 2020 at 11:51:00AM +0000, Shinichiro Kawasaki wrote:
>> I tried Peter's patch in my test system and result looks good. The warning is
>> still observed with 5.10-rc4. With the patch on top of 5.10-rc4, the warning
>> disappeared.
>
> The patch on its own is not sufficient to fix things; there needs to be
> an accompanying objtool patch to generate the correct unwind
> information.
>
> This patch only ensures objtool has enough information to be able to
> dtrt, but as it stands it shouldn't even compile, it should complain
> about an alternative trying to modify the stack and bail.
>

It complains, but doesn't bail.

I'm nearly ready with my series replacing all the custom code paravirt
patches by ALTERNATIVEs. Quite some paravirt macro cruft turned out to
be unused, so there is a rather large cleanup patch included. :-)

Juergen

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.06 kB)
OpenPGP_signature (505.00 B)
OpenPGP digital signature Download all attachments

2020-11-19 12:51:32

by Shinichiro Kawasaki

[permalink] [raw]

Subject: Re: WARNING: can't access registers at asm_common_interrupt

On Nov 19, 2020 / 13:01, Peter Zijlstra wrote:
> On Thu, Nov 19, 2020 at 11:51:00AM +0000, Shinichiro Kawasaki wrote:
> > I tried Peter's patch in my test system and result looks good. The warning is
> > still observed with 5.10-rc4. With the patch on top of 5.10-rc4, the warning
> > disappeared.
>
> The patch on its own is not sufficient to fix things; there needs to be
> an accompanying objtool patch to generate the correct unwind
> information.
>
> This patch only ensures objtool has enough information to be able to
> dtrt, but as it stands it shouldn't even compile, it should complain
> about an alternative trying to modify the stack and bail.

Thanks for the clarification. It was too early to try out... When it gets ready
to try and test, please let me know.

--
Best Regards,
Shin'ichiro Kawasaki