[PATCH v2 21/30] cxlflash: Fix to prevent workq from accessing freed memory

[PATCH v2 21/30] cxlflash: Fix to prevent workq from accessing freed memory

[Matthew R. Ochs]

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);
 	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;
-- 
2.1.0

[PATCH v2 00/30] cxlflash: Miscellaneous bug fixes and corrections

[Matthew R. Ochs]

This patch set contains various fixes and corrections for issues that
were found during test and code review. The series is based upon the
code upstreamed in 4.3 and is intended for the rc phase. The entire
set is bisectable. Please reference the changelog below for details
on what has been altered from previous versions of this patch set.

v2 Changes:
- Incorporate comments from Ian Munsie
- Rework commit messages to be more descriptive
- Add state change serialization patch

Manoj Kumar (3):
  cxlflash: Fix to avoid invalid port_sel value
  cxlflash: Replace magic numbers with literals
  cxlflash: Fix read capacity timeout

Matthew R. Ochs (27):
  cxlflash: Fix potential oops following LUN removal
  cxlflash: Fix data corruption when vLUN used over multiple cards
  cxlflash: Fix to avoid sizeof(bool)
  cxlflash: Fix context encode mask width
  cxlflash: Fix to avoid CXL services during EEH
  cxlflash: Check for removal when processing interrupt
  cxlflash: Correct naming of limbo state and waitq
  cxlflash: Make functions static
  cxlflash: Refine host/device attributes
  cxlflash: Fix to avoid spamming the kernel log
  cxlflash: Fix to avoid stall while waiting on TMF
  cxlflash: Fix location of setting resid
  cxlflash: Fix host link up event handling
  cxlflash: Fix async interrupt bypass logic
  cxlflash: Remove dual port online dependency
  cxlflash: Fix AFU version access/storage and add check
  cxlflash: Correct usage of scsi_host_put()
  cxlflash: Fix to prevent workq from accessing freed memory
  cxlflash: Correct behavior in device reset handler following EEH
  cxlflash: Remove unnecessary scsi_block_requests
  cxlflash: Fix function prolog parameters and return codes
  cxlflash: Fix MMIO and endianness errors
  cxlflash: Fix to prevent EEH recovery failure
  cxlflash: Correct spelling, grammar, and alignment mistakes
  cxlflash: Fix to prevent stale AFU RRQ
  cxlflash: Fix to avoid state change collision
  MAINTAINERS: Add cxlflash driver

 MAINTAINERS                       |    9 +
 drivers/scsi/cxlflash/common.h    |   29 +-
 drivers/scsi/cxlflash/lunmgt.c    |    9 +-
 drivers/scsi/cxlflash/main.c      | 1575 ++++++++++++++++++++-----------------
 drivers/scsi/cxlflash/main.h      |    1 +
 drivers/scsi/cxlflash/sislite.h   |    8 +-
 drivers/scsi/cxlflash/superpipe.c |  177 +++--
 drivers/scsi/cxlflash/superpipe.h |   11 +-
 drivers/scsi/cxlflash/vlun.c      |   39 +-
 9 files changed, 1036 insertions(+), 822 deletions(-)

-- 
2.1.0

[PATCH v2 21/30] cxlflash: Fix to prevent workq from accessing freed memory

[Matthew R. Ochs]

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);
 	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;
-- 
2.1.0

Re: [PATCH v2 21/30] cxlflash: Fix to prevent workq from accessing freed memory

[Tomas Henzl]

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;

Re: [PATCH v2 21/30] cxlflash: Fix to prevent workq from accessing freed memory

[Matthew R. Ochs]

> 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