| Message ID | 1442439105-50148-1-git-send-email-mrochs@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 16.9.2015 23:31, Matthew R. Ochs wrote: > The workq can process work in parallel with a remove event, leading > to a condition where the workq handler can access freed memory. > > To remedy, the workq should be terminated prior to freeing memory. Move > the termination call earlier in remove and use cancel_work_sync() instead > of flush_work() as there is not a need to process any scheduled work when > shutting down. > > Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com> > Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com> > --- > drivers/scsi/cxlflash/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c > index 1856a73..1625aea 100644 > --- a/drivers/scsi/cxlflash/main.c > +++ b/drivers/scsi/cxlflash/main.c > @@ -736,12 +736,12 @@ static void cxlflash_remove(struct pci_dev *pdev) > scsi_remove_host(cfg->host); > /* Fall through */ > case INIT_STATE_AFU: > + cancel_work_sync(&cfg->work_q); > term_afu(cfg); You disable irqs after a call to cancel_work_sync. That means a late int could trigger the workqueue again? Please disable irqs earlier - as described in Documentation/PCI/pci.txt > case INIT_STATE_PCI: > pci_release_regions(cfg->dev); > pci_disable_device(pdev); > case INIT_STATE_NONE: > - flush_work(&cfg->work_q); > free_mem(cfg); > scsi_host_put(cfg->host); > break;
> On Sep 21, 2015, at 7:25 AM, Tomas Henzl <thenzl@redhat.com> wrote: > On 16.9.2015 23:31, Matthew R. Ochs wrote: >> The workq can process work in parallel with a remove event, leading >> to a condition where the workq handler can access freed memory. >> >> To remedy, the workq should be terminated prior to freeing memory. Move >> the termination call earlier in remove and use cancel_work_sync() instead >> of flush_work() as there is not a need to process any scheduled work when >> shutting down. >> >> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com> >> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com> >> --- >> drivers/scsi/cxlflash/main.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c >> index 1856a73..1625aea 100644 >> --- a/drivers/scsi/cxlflash/main.c >> +++ b/drivers/scsi/cxlflash/main.c >> @@ -736,12 +736,12 @@ static void cxlflash_remove(struct pci_dev *pdev) >> scsi_remove_host(cfg->host); >> /* Fall through */ >> case INIT_STATE_AFU: >> + cancel_work_sync(&cfg->work_q); >> term_afu(cfg); > > You disable irqs after a call to cancel_work_sync. > That means a late int could trigger the workqueue again? > Please disable irqs earlier - as described in Documentation/PCI/pci.txt I'll change the order here such that the work is cancelled after term_afu() is called. -matt
diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c index 1856a73..1625aea 100644 --- a/drivers/scsi/cxlflash/main.c +++ b/drivers/scsi/cxlflash/main.c @@ -736,12 +736,12 @@ static void cxlflash_remove(struct pci_dev *pdev) scsi_remove_host(cfg->host); /* Fall through */ case INIT_STATE_AFU: + cancel_work_sync(&cfg->work_q); term_afu(cfg); case INIT_STATE_PCI: pci_release_regions(cfg->dev); pci_disable_device(pdev); case INIT_STATE_NONE: - flush_work(&cfg->work_q); free_mem(cfg); scsi_host_put(cfg->host); break;