linux-kernel - Re: [PATCH] m68k: coldfire: Prevent spurious interrupts when masking IMR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d6f4e8b7-2626-4fed-b46d-80a9cdb54a50@yoseli.org>
Date: Tue, 4 Feb 2025 21:17:02 +0100
From: Jean-Michel Hautbois <jeanmichel.hautbois@...eli.org>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Greg Ungerer <gerg@...ux-m68k.org>, linux-m68k@...ts.linux-m68k.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] m68k: coldfire: Prevent spurious interrupts when masking
 IMR

Hi Geert,

On 04/02/2025 20:27, Geert Uytterhoeven wrote:
> Hi Jean-Michel,
> 
> On Tue, 4 Feb 2025 at 19:38, Jean-Michel Hautbois
> <jeanmichel.hautbois@...eli.org> wrote:
>> The ColdFire interrupt controller can generate spurious interrupts if an
>> interrupt source is masked in the IMR while the CPU interrupt priority
>> mask (SR[I]) is set lower than the interrupt level.
>>
>> The reference manual states:
>>
>> To avoid this situation for interrupts sources with levels 1-6, first
>> write a higher level interrupt mask to the status register, before
>> setting the mask in the IMR or the module’s interrupt mask register.
>> After the mask is set, return the interrupt mask in the status register
>> to its previous value.
>>
>> It can be tested like this:
>> - Prepare a iperf3 server on the coldfire target (iperf3 -s -D)
>> - Start a high priority cyclictest:
>>      cyclictest --secaligned -m -p 99 -i 2500 -q
>> - Start iperf3 -c $COLDFIRE_IP -t 0
>>
>> After a few seconds the dmesg may display:
>> [   84.784301] irq 24, desc: dbc502da, depth: 1, count: 0, unhandled: 0
>> [   84.784455] ->handle_irq():  0ba0aca3, handle_bad_irq+0x0/0x1e0
>> [   84.784610] ->irq_data.chip(): c6779d4f, 0x41652544
>> [   84.784719] ->action(): 00000000
>> [   84.784770] unexpected IRQ trap at vector 18
>>
>> With this patch, I never saw it in a few hours testing.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@...eli.org>
> 
> Thanks for your patch!
> 
>> --- a/arch/m68k/coldfire/intc-simr.c
>> +++ b/arch/m68k/coldfire/intc-simr.c
>> @@ -58,6 +58,14 @@ static inline unsigned int irq2ebit(unsigned int irq)
>>
>>   #endif
>>
>> +static inline void intc_irq_setlevel(unsigned long level)
>> +{
>> +       asm volatile ("move.w %0,%%sr"
>> +                     : /* no outputs */
>> +                     : "d" (0x2000 | ((level) << 8))
>> +                     : "memory");
>> +}
>> +
>>   /*
>>    *     There maybe one, two or three interrupt control units, each has 64
>>    *     interrupts. If there is no second or third unit then MCFINTC1_* or
>> @@ -67,13 +75,17 @@ static inline unsigned int irq2ebit(unsigned int irq)
>>   static void intc_irq_mask(struct irq_data *d)
>>   {
>>          unsigned int irq = d->irq - MCFINT_VECBASE;
>> +       unsigned long flags = arch_local_save_flags();
>>
>> +       intc_irq_setlevel(7);
> 
> Can't all of the above just be replaced by
> 
>      unsigned long flags = arch_local_irq_save();
> 

I will check it again, but it seems a bit different. Now I get:
[   97.919407] hrtimer: interrupt took 203520 ns
[  161.757921] irq 24, desc: 24ef8971, depth: 1, count: 0, unhandled: 0
[  161.758074] ->handle_irq():  60d6a158, handle_bad_irq+0x0/0x1e0
[  161.758241] ->irq_data.chip(): 93aaf7a5, 0x4164e544
[  161.758359] ->action(): 00000000
[  161.758415] unexpected IRQ trap at vector 18

So, maybe isn't it exactly the same after all ?

JM

>>          if (MCFINTC2_SIMR && (irq > 127))
>>                  __raw_writeb(irq - 128, MCFINTC2_SIMR);
>>          else if (MCFINTC1_SIMR && (irq > 63))
>>                  __raw_writeb(irq - 64, MCFINTC1_SIMR);
>>          else
>>                  __raw_writeb(irq, MCFINTC0_SIMR);
>> +
>> +       arch_local_irq_restore(flags);
>>   }
>>
>>   static void intc_irq_unmask(struct irq_data *d)
>>
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>