Re: [PATCH v5 01/20] EDAC/synopsys: Fix ECC status data and IRQ disable race condition

[Serge Semin]

On Mon, Apr 15, 2024 at 08:36:16PM +0200, Borislav Petkov wrote:
> On Thu, Feb 22, 2024 at 09:12:46PM +0300, Serge Semin wrote:
> > The race condition around the ECCCLR register access happens in the IRQ
> > disable method called in the device remove() procedure and in the ECC IRQ
> > handler:
> > 1. Enable IRQ:
> >    a. ECCCLR = EN_CE | EN_UE
> > 2. Disable IRQ:
> >    a. ECCCLR = 0
> > 3. IRQ handler:
> >    a. ECCCLR = CLR_CE | CLR_CE_CNT | CLR_CE | CLR_CE_CNT
> >    b. ECCCLR = 0
> >    c. ECCCLR = EN_CE | EN_UE
> > So if the IRQ disabling procedure is called concurrently with the IRQ
> > handler method the IRQ might be actually left enabled due to the
> > statement 3c.
> > 
> > The root cause of the problem is that ECCCLR register (which since v3.10a
> > has been called as ECCCTL) has intermixed ECC status data clear flags and
> > the IRQ enable/disable flags. Thus the IRQ disabling (clear EN flags) and
> > handling (write 1 to clear ECC status data) procedures must be serialised
> > around the ECCCTL register modification to prevent the race.
> > 
> > So fix the problem described above by adding the spin-lock around the
> > ECCCLR modifications and preventing the IRQ-handler from modifying the
> > IRQs enable flags (there is no point in disabling the IRQ and then
> > re-enabling it again within a single IRQ handler call, see the statements
> > 3a/3b and 3c above).
> 

> So I'm looking at the code and am looking at this and wondering how we
> even ended up in this mess?!
> 
> An interrupt handler should not *enable* the interrupt again - that's
> just crazy. And I should've seen that in
> 
>   4bcffe941758 ("EDAC/synopsys: Re-enable the error interrupts on v3 hw")
> 
> and stopped it right there. But well, it is what it is...

It looks indeed crazy because the method is called enable_intr() and
is called in the IRQ handler. Right, re-enabling the IRQ in the handler
doesn't look good. But under the hood it was just a way to fix the
problem described in the commit you cited. enable_intr() just gets
back the IRQ Enable flags cleared a bit before in the
zynqmp_get_error_info() method.

The root cause of the problem is that the IRQ status/clear flags:
ECCCLR.ecc_corrected_err_clr	(R/W1C)
ECCCLR.ecc_uncorrected_err_clr	(R/W1C)
ECCCLR.ecc_corr_err_cnt_clr	(R/W1C)
ECCCLR.ecc_uncorr_err_cnt_clr	(R/W1C)
etc

and the IRQ enable/disable flags (since v3.10a):
ECCCLR.ecc_corrected_err_intr_en	(R/W)
ECCCLR.ecc_uncorrected_err_intr_en	(R/W)

reside in a single register - ECCCLR (Synopsys has renamed it to
ECCCTL since v3.10a due to adding the IRQ En/Dis flags).

Thus any concurrent access to that CSR like "Clear IRQ
status/counters" and "IRQ disable/enable" need to be protected from
the race condition.

> 
> So I'd like to see the following flow:
> 
> * on init, the interrupt is enabled with enable_intr() *after*
> registering the interrupt handler.
> 
> * on exit, the interrupt is disabled with disable_intr() and then no
> interrupts are coming in anymore.
> 
> And then I don't think you'll need the spinlock and it'll be sane
> design.
> 
> Right?

This is what is implemented at the moment and it's racy. IRQ-handler
clears the IRQ status/counters by writing 1s to the respective flags
in the ECCCLR register. This inevitable causes writing to the IRQ
Enable/disable bits. Thus if during the IRQ handling the driver/device
gets to be removed (disable_intr() is called), the IRQ might be left
enabled despite of having the disable_intr() method executed. In its
turn that may cause fatal problems if the IRQ handler is executed once
again before the IRQ line is freed and disabled in the GIC side.

If we wish to avoid using the atomic spin-lock we'll need to change
the order:
0. Enable ECC IRQ in ECCCLR/ECCCTL CSR
1. Request IRQ line and register the IRQ handler
..
2. Free IRQ line
3. Disable ECC IRQ in ECCCLR/ECCCTL CSR

But if that path is decided to be taken the next aspects will need to
be taken into account:

1. If the IRQ line is shared, then the ECC IRQ might be delivered
somewhere between steps 0 and 1, which won't make the IRQ subsystem
happy. (Although this isn't actual for the current driver because it
requests the non-shared IRQ line for ECC, but who knows how the
situation will change in future).

2. A bit later (in the patchset #3) I am adding the ECC Scrubber
support, which will need the spin-lock anyway to protect another two
CSRs access. These CSRs are touched in run-time to enable/disable the
scrubbing and set/get the scrub rate.

So since we'll need to have a spin-lock anyway and from the
scalability point of view, it sounds reasonable to keep what is
suggested in my patch. What do you think?

-Serge(y)

> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette