bisected: [PATCH V4] x86/apic: Always explicitly disarm TSC-deadline timer

bisected: [PATCH V4] x86/apic: Always explicitly disarm TSC-deadline timer

[Sergey Senozhatsky]

Hi folks,
sorry if messed something up, this email has never been in my inbox.

> Disable the TSC Deadline timer in lapic_timer_shutdown() by writing to
> MSR_IA32_TSC_DEADLINE when in TSC-deadline mode. Also avoid writing
> to the initial-count register (APIC_TMICT) which is ignored in
> TSC-deadline mode.

So this commit hit stable and we now see section mismatch errors:

// stripped

WARNING: vmlinux.o(__ex_table+0x447c): Section mismatch in reference from the (unknown reference) (unknown) to the (unknown reference) .irqentry.text:(unknown)
The relocation at __ex_table+0x447c references
section ".irqentry.text" which is not in the list of
authorized sections.

WARNING: vmlinux.o(__ex_table+0x4480): Section mismatch in reference from the (unknown reference) (unknown) to the (unknown reference) .irqentry.text:(unknown)
The relocation at __ex_table+0x4480 references
section ".irqentry.text" which is not in the list of
authorized sections.

FATAL: modpost: Section mismatches detected.


Specifically because of wrmsrl.

I'm aware of the section mismatch errors on linux-5.4 (I know), not
aware of any other stable versions (but I haven't checked).  Is this
something specific to linux-5.4?

Re: bisected: [PATCH V4] x86/apic: Always explicitly disarm TSC-deadline timer

[Sergey Senozhatsky]

On (24/11/28 20:18), Sergey Senozhatsky wrote:
> > Disable the TSC Deadline timer in lapic_timer_shutdown() by writing to
> > MSR_IA32_TSC_DEADLINE when in TSC-deadline mode. Also avoid writing
> > to the initial-count register (APIC_TMICT) which is ignored in
> > TSC-deadline mode.

Upstream commit ffd95846c6ec6cf1f93da411ea10d504036cab42 (forgot
to mention)

Re: bisected: [PATCH V4] x86/apic: Always explicitly disarm TSC-deadline timer

[Thomas Gleixner]

On Thu, Nov 28 2024 at 20:18, Sergey Senozhatsky wrote:
>> Disable the TSC Deadline timer in lapic_timer_shutdown() by writing to
>> MSR_IA32_TSC_DEADLINE when in TSC-deadline mode. Also avoid writing
>> to the initial-count register (APIC_TMICT) which is ignored in
>> TSC-deadline mode.
>
> So this commit hit stable and we now see section mismatch errors:
>
> // stripped
>
> WARNING: vmlinux.o(__ex_table+0x447c): Section mismatch in reference from the (unknown reference) (unknown) to the (unknown reference) .irqentry.text:(unknown)
> The relocation at __ex_table+0x447c references
> section ".irqentry.text" which is not in the list of
> authorized sections.
>
> WARNING: vmlinux.o(__ex_table+0x4480): Section mismatch in reference from the (unknown reference) (unknown) to the (unknown reference) .irqentry.text:(unknown)
> The relocation at __ex_table+0x4480 references
> section ".irqentry.text" which is not in the list of
> authorized sections.
>
> FATAL: modpost: Section mismatches detected.
>
> Specifically because of wrmsrl.
>
> I'm aware of the section mismatch errors on linux-5.4 (I know), not
> aware of any other stable versions (but I haven't checked).  Is this
> something specific to linux-5.4?

So it seems the compiler inlines the inner guts of
sysvec_apic_timer_interrupt() and local_apic_timer_interrupt().

Can you try the patch below?

Thanks,

        tglx
---
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1007,7 +1007,7 @@ void setup_secondary_APIC_clock(void)
 /*
  * The guts of the apic timer interrupt
  */
-static void local_apic_timer_interrupt(void)
+static noinline void local_apic_timer_interrupt(void)
 {
 	struct clock_event_device *evt = this_cpu_ptr(&lapic_events);
 

Re: bisected: [PATCH V4] x86/apic: Always explicitly disarm TSC-deadline timer

[Sergey Senozhatsky]

On (24/11/30 12:21), Thomas Gleixner wrote:
> > WARNING: vmlinux.o(__ex_table+0x447c): Section mismatch in reference from the (unknown reference) (unknown) to the (unknown reference) .irqentry.text:(unknown)
> > The relocation at __ex_table+0x447c references
> > section ".irqentry.text" which is not in the list of
> > authorized sections.
> >
> > WARNING: vmlinux.o(__ex_table+0x4480): Section mismatch in reference from the (unknown reference) (unknown) to the (unknown reference) .irqentry.text:(unknown)
> > The relocation at __ex_table+0x4480 references
> > section ".irqentry.text" which is not in the list of
> > authorized sections.
> >
> > FATAL: modpost: Section mismatches detected.
> >
> > Specifically because of wrmsrl.
> >
> > I'm aware of the section mismatch errors on linux-5.4 (I know), not
> > aware of any other stable versions (but I haven't checked).  Is this
> > something specific to linux-5.4?
> 
> So it seems the compiler inlines the inner guts of
> sysvec_apic_timer_interrupt() and local_apic_timer_interrupt().
> 
> Can you try the patch below?

That works, as far as I can tell, thank you!

[PATCH] modpost: Add .irqentry.text to OTHER_SECTIONS

[Thomas Gleixner]

The compiler can fully inline the actual handler function of an interrupt
entry into the .irqentry.text entry point. If such a function contains an
access which has an exception table entry, modpost complains about a
section mismatch:

  WARNING: vmlinux.o(__ex_table+0x447c): Section mismatch in reference ...

  The relocation at __ex_table+0x447c references section ".irqentry.text"
  which is not in the list of authorized sections.

Add .irqentry.text to OTHER_SECTIONS to cure the issue.

Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 scripts/mod/modpost.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -785,7 +785,7 @@ static void check_section(const char *mo
 		".ltext", ".ltext.*"
 #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \
 		".fixup", ".entry.text", ".exception.text", \
-		".coldtext", ".softirqentry.text"
+		".coldtext", ".softirqentry.text", ".irqentry.text"
 
 #define ALL_TEXT_SECTIONS  ".init.text", ".exit.text", \
 		TEXT_SECTIONS, OTHER_TEXT_SECTIONS

Re: [PATCH] modpost: Add .irqentry.text to OTHER_SECTIONS

[Masahiro Yamada]

On Sun, Dec 1, 2024 at 8:17 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> The compiler can fully inline the actual handler function of an interrupt
> entry into the .irqentry.text entry point. If such a function contains an
> access which has an exception table entry, modpost complains about a
> section mismatch:
>
>   WARNING: vmlinux.o(__ex_table+0x447c): Section mismatch in reference ...
>
>   The relocation at __ex_table+0x447c references section ".irqentry.text"
>   which is not in the list of authorized sections.
>
> Add .irqentry.text to OTHER_SECTIONS to cure the issue.
>
> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>

I found the context in LKML.


Closes: https://lore.kernel.org/all/20241128111844.GE10431@google.com/



However, is this still relevant to the mainline kernel?

In Linux 5.4.y, I agree this because smp_apic_timer_interrupt()
is annotated as __irq_entry:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/x86/kernel/apic/apic.c?id=v5.4.286#n1145



In this mainline kernel, DEFINE_IDTENTRY_SYSVEC()
expands to a normal .text function which is explicitly
annotated 'noinline'.





> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  scripts/mod/modpost.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -785,7 +785,7 @@ static void check_section(const char *mo
>                 ".ltext", ".ltext.*"
>  #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \
>                 ".fixup", ".entry.text", ".exception.text", \
> -               ".coldtext", ".softirqentry.text"
> +               ".coldtext", ".softirqentry.text", ".irqentry.text"
>
>  #define ALL_TEXT_SECTIONS  ".init.text", ".exit.text", \
>                 TEXT_SECTIONS, OTHER_TEXT_SECTIONS



--
Best Regards
Masahiro Yamada

Re: [PATCH] modpost: Add .irqentry.text to OTHER_SECTIONS

[Sergey Senozhatsky]

On (24/12/01 12:17), Thomas Gleixner wrote:
> The compiler can fully inline the actual handler function of an interrupt
> entry into the .irqentry.text entry point. If such a function contains an
> access which has an exception table entry, modpost complains about a
> section mismatch:
> 
>   WARNING: vmlinux.o(__ex_table+0x447c): Section mismatch in reference ...
> 
>   The relocation at __ex_table+0x447c references section ".irqentry.text"
>   which is not in the list of authorized sections.
> 
> Add .irqentry.text to OTHER_SECTIONS to cure the issue.

This works.  In fact, this looks like a local fix which we applied
here on our side, but we were very unsure.  Thank you Thomas.

Re: [PATCH] modpost: Add .irqentry.text to OTHER_SECTIONS

[Thomas Gleixner]

On Mon, Dec 02 2024 at 11:02, Masahiro Yamada wrote:
> On Sun, Dec 1, 2024 at 8:17 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> The compiler can fully inline the actual handler function of an interrupt
>> entry into the .irqentry.text entry point. If such a function contains an
>> access which has an exception table entry, modpost complains about a
>> section mismatch:
>>
>>   WARNING: vmlinux.o(__ex_table+0x447c): Section mismatch in reference ...
>>
>>   The relocation at __ex_table+0x447c references section ".irqentry.text"
>>   which is not in the list of authorized sections.
>>
>> Add .irqentry.text to OTHER_SECTIONS to cure the issue.
>>
>> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
>
> I found the context in LKML.
> Closes: https://lore.kernel.org/all/20241128111844.GE10431@google.com/
>
> However, is this still relevant to the mainline kernel?
>
> In Linux 5.4.y, I agree this because smp_apic_timer_interrupt()
> is annotated as __irq_entry:

Correct.

> In this mainline kernel, DEFINE_IDTENTRY_SYSVEC()
> expands to a normal .text function which is explicitly
> annotated 'noinline'.

It's not annotated noinline, it's annotated 'noinstr', which puts the
code into the .noinstr.text section. That one is indeed covered.

So yes, the fix is only required for pre 5.8 kernels.

Thanks,

        tglx

Re: [PATCH] modpost: Add .irqentry.text to OTHER_SECTIONS

[Masahiro Yamada]

On Tue, Dec 3, 2024 at 6:03 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, Dec 02 2024 at 11:02, Masahiro Yamada wrote:
> > On Sun, Dec 1, 2024 at 8:17 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> The compiler can fully inline the actual handler function of an interrupt
> >> entry into the .irqentry.text entry point. If such a function contains an
> >> access which has an exception table entry, modpost complains about a
> >> section mismatch:
> >>
> >>   WARNING: vmlinux.o(__ex_table+0x447c): Section mismatch in reference ...
> >>
> >>   The relocation at __ex_table+0x447c references section ".irqentry.text"
> >>   which is not in the list of authorized sections.
> >>
> >> Add .irqentry.text to OTHER_SECTIONS to cure the issue.
> >>
> >> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> >
> > I found the context in LKML.
> > Closes: https://lore.kernel.org/all/20241128111844.GE10431@google.com/
> >
> > However, is this still relevant to the mainline kernel?
> >
> > In Linux 5.4.y, I agree this because smp_apic_timer_interrupt()
> > is annotated as __irq_entry:
>
> Correct.
>
> > In this mainline kernel, DEFINE_IDTENTRY_SYSVEC()
> > expands to a normal .text function which is explicitly
> > annotated 'noinline'.
>
> It's not annotated noinline, it's annotated 'noinstr', which puts the
> code into the .noinstr.text section. That one is indeed covered.


The callsite of local_apic_timer_interrupt() is annotated 'noinline'
if I correctly understand this line:
  https://github.com/torvalds/linux/blob/v6.13-rc1/arch/x86/include/asm/idtentry.h#L272


It expands to:

static noinline void __sysvec_apic_timer_interrupt(struct pt_regs *regs)
{
       [snip]
       local_apic_timer_interrupt();
       [snip]
}



> So yes, the fix is only required for pre 5.8 kernels.

This never occurs on x86 after commit f0178fc01fe46,
but theoretically this may occur for other architectures.

Now applied to linux-kbuild.
Thanks.




>
> Thanks,
>
>         tglx



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] modpost: Add .irqentry.text to OTHER_SECTIONS

[Thomas Gleixner]

On Wed, Dec 04 2024 at 00:27, Masahiro Yamada wrote:
> On Tue, Dec 3, 2024 at 6:03 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> > In this mainline kernel, DEFINE_IDTENTRY_SYSVEC()
>> > expands to a normal .text function which is explicitly
>> > annotated 'noinline'.
>>
>> It's not annotated noinline, it's annotated 'noinstr', which puts the
>> code into the .noinstr.text section. That one is indeed covered.
>
> The callsite of local_apic_timer_interrupt() is annotated 'noinline'
> if I correctly understand this line:
>   https://github.com/torvalds/linux/blob/v6.13-rc1/arch/x86/include/asm/idtentry.h#L272

You're right. I got lost in the macro maze and looked at the actual
sysvec_...() part.

>> So yes, the fix is only required for pre 5.8 kernels.
>
> This never occurs on x86 after commit f0178fc01fe46,
> but theoretically this may occur for other architectures.

Correct.

Thanks,

        tglx