| Message ID | 20171117211455.29031-4-gpiccoli@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
> -----Original Message----- > From: Guilherme G. Piccoli [mailto:gpiccoli@linux.vnet.ibm.com] > Sent: Friday, November 17, 2017 1:15 PM > To: dl-esc-Aacraid Linux Driver <aacraid@microsemi.com>; linux- > scsi@vger.kernel.org > Cc: gpiccoli@linux.vnet.ibm.com; Dave Carroll > <david.carroll@microsemi.com>; Raghava Aditya Renukunta > <RaghavaAditya.Renukunta@microsemi.com>; gpiccoli@protonmail.ch > Subject: [PATCH 3/3] scsi: aacraid: Prevent crash in case of free interrupt > during scsi EH path > > EXTERNAL EMAIL > > > As part of the scsi EH path, aacraid performs a reinitialization of > the adapter, which encompass freeing resources and IRQs, NULLifying > lots of pointers, and then initialize it all over again. > We've identified a problem during the free IRQ portion of this path > if CONFIG_DEBUG_SHIRQ is enabled on kernel config file. > > Happens that, in case this flag was set, right after free_irq() > effectively clears the interrupt, it checks if it was requested > as IRQF_SHARED. In positive case, it performs another call to the > IRQ handler on driver. Problem is: since aacraid currently free > some resources *before* freeing the IRQ, once free_irq() path > calls the handler again (due to CONFIG_DEBUG_SHIRQ), aacraid > crashes due to NULL pointer dereference with the following trace: > > aac_src_intr_message+0xf8/0x740 [aacraid] > __free_irq+0x33c/0x4a0 > free_irq+0x78/0xb0 > aac_free_irq+0x13c/0x150 [aacraid] > aac_reset_adapter+0x2e8/0x970 [aacraid] > aac_eh_reset+0x3a8/0x5d0 [aacraid] > scsi_try_host_reset+0x74/0x180 > scsi_eh_ready_devs+0xc70/0x1510 > scsi_error_handler+0x624/0xa20 > > This patch prevents the crash by changing the order of the > deinitialization in this path of aacraid: first we clear the IRQ, > then we free other resources. No functional change intended. > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > --- Thank you for the fix. Much appreciated. Reviewed-by :Raghava Aditya Renukunta <RaghavaAditya.Renukunta@microsemi.com>
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c index 2abe8fd83494..bec9f3193f60 100644 --- a/drivers/scsi/aacraid/commsup.c +++ b/drivers/scsi/aacraid/commsup.c @@ -1554,6 +1554,7 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type) * will ensure that i/o is queisced and the card is flushed in that * case. */ + aac_free_irq(aac); aac_fib_map_free(aac); dma_free_coherent(&aac->pdev->dev, aac->comm_size, aac->comm_addr, aac->comm_phys); @@ -1561,7 +1562,6 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type) aac->comm_phys = 0; kfree(aac->queues); aac->queues = NULL; - aac_free_irq(aac); kfree(aac->fsa_dev); aac->fsa_dev = NULL;
As part of the scsi EH path, aacraid performs a reinitialization of the adapter, which encompass freeing resources and IRQs, NULLifying lots of pointers, and then initialize it all over again. We've identified a problem during the free IRQ portion of this path if CONFIG_DEBUG_SHIRQ is enabled on kernel config file. Happens that, in case this flag was set, right after free_irq() effectively clears the interrupt, it checks if it was requested as IRQF_SHARED. In positive case, it performs another call to the IRQ handler on driver. Problem is: since aacraid currently free some resources *before* freeing the IRQ, once free_irq() path calls the handler again (due to CONFIG_DEBUG_SHIRQ), aacraid crashes due to NULL pointer dereference with the following trace: aac_src_intr_message+0xf8/0x740 [aacraid] __free_irq+0x33c/0x4a0 free_irq+0x78/0xb0 aac_free_irq+0x13c/0x150 [aacraid] aac_reset_adapter+0x2e8/0x970 [aacraid] aac_eh_reset+0x3a8/0x5d0 [aacraid] scsi_try_host_reset+0x74/0x180 scsi_eh_ready_devs+0xc70/0x1510 scsi_error_handler+0x624/0xa20 This patch prevents the crash by changing the order of the deinitialization in this path of aacraid: first we clear the IRQ, then we free other resources. No functional change intended. Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> --- drivers/scsi/aacraid/commsup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)