diff mbox

[v2,20/30] cxlflash: Correct usage of scsi_host_put()

Message ID 1442439049-49970-1-git-send-email-mrochs@linux.vnet.ibm.com (mailing list archive)
State Superseded
Headers show

Commit Message

Matthew R. Ochs Sept. 16, 2015, 9:30 p.m. UTC
Currently, scsi_host_put() is being called prematurely in the
remove path and is missing entirely in an error cleanup path.
The former can lead to memory being freed too early with
subsequent access potentially corrupting data whilst the former
would result in a memory leak.

Move the usage on remove to be the last cleanup action taken
and introduce a call to scsi_host_put() in the one initialization
error path that does not use remove to cleanup.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Brian King Sept. 22, 2015, 8:53 p.m. UTC | #1
On 09/16/2015 04:30 PM, Matthew R. Ochs wrote:
> Currently, scsi_host_put() is being called prematurely in the
> remove path and is missing entirely in an error cleanup path.
> The former can lead to memory being freed too early with
> subsequent access potentially corrupting data whilst the former
> would result in a memory leak.
> 
> Move the usage on remove to be the last cleanup action taken
> and introduce a call to scsi_host_put() in the one initialization
> error path that does not use remove to cleanup.
> 
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
> index fc77cd4..1856a73 100644
> --- a/drivers/scsi/cxlflash/main.c
> +++ b/drivers/scsi/cxlflash/main.c
> @@ -734,7 +734,6 @@ static void cxlflash_remove(struct pci_dev *pdev)
>  	case INIT_STATE_SCSI:
>  		cxlflash_term_local_luns(cfg);
>  		scsi_remove_host(cfg->host);
> -		scsi_host_put(cfg->host);
>  		/* Fall through */
>  	case INIT_STATE_AFU:
>  		term_afu(cfg);
> @@ -744,6 +743,7 @@ static void cxlflash_remove(struct pci_dev *pdev)
>  	case INIT_STATE_NONE:
>  		flush_work(&cfg->work_q);
>  		free_mem(cfg);
> +		scsi_host_put(cfg->host);
>  		break;
>  	}
> 
> @@ -2415,6 +2415,7 @@ static int cxlflash_probe(struct pci_dev *pdev,
>  		dev_err(&pdev->dev, "%s: call to scsi_host_alloc failed!\n",

This message text is wrong. Its the call to alloc_mem that has failed in this
leg, not the call to scsi_host_alloc.

>  			__func__);
>  		rc = -ENOMEM;
> +		scsi_host_put(cfg->host);
>  		goto out;
>  	}
>
Matthew R. Ochs Sept. 22, 2015, 9:49 p.m. UTC | #2
> On Sep 22, 2015, at 3:53 PM, Brian King <brking@linux.vnet.ibm.com> wrote:
> On 09/16/2015 04:30 PM, Matthew R. Ochs wrote:
>> Currently, scsi_host_put() is being called prematurely in the
>> remove path and is missing entirely in an error cleanup path.
>> The former can lead to memory being freed too early with
>> subsequent access potentially corrupting data whilst the former
>> would result in a memory leak.
>> 
>> Move the usage on remove to be the last cleanup action taken
>> and introduce a call to scsi_host_put() in the one initialization
>> error path that does not use remove to cleanup.
>> 
>> 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 | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
>> index fc77cd4..1856a73 100644
>> --- a/drivers/scsi/cxlflash/main.c
>> +++ b/drivers/scsi/cxlflash/main.c
>> @@ -734,7 +734,6 @@ static void cxlflash_remove(struct pci_dev *pdev)
>> 	case INIT_STATE_SCSI:
>> 		cxlflash_term_local_luns(cfg);
>> 		scsi_remove_host(cfg->host);
>> -		scsi_host_put(cfg->host);
>> 		/* Fall through */
>> 	case INIT_STATE_AFU:
>> 		term_afu(cfg);
>> @@ -744,6 +743,7 @@ static void cxlflash_remove(struct pci_dev *pdev)
>> 	case INIT_STATE_NONE:
>> 		flush_work(&cfg->work_q);
>> 		free_mem(cfg);
>> +		scsi_host_put(cfg->host);
>> 		break;
>> 	}
>> 
>> @@ -2415,6 +2415,7 @@ static int cxlflash_probe(struct pci_dev *pdev,
>> 		dev_err(&pdev->dev, "%s: call to scsi_host_alloc failed!\n",
> 
> This message text is wrong. Its the call to alloc_mem that has failed in this
> leg, not the call to scsi_host_alloc.

Good find. I'll fix this in a separate patch.


-matt
diff mbox

Patch

diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index fc77cd4..1856a73 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -734,7 +734,6 @@  static void cxlflash_remove(struct pci_dev *pdev)
 	case INIT_STATE_SCSI:
 		cxlflash_term_local_luns(cfg);
 		scsi_remove_host(cfg->host);
-		scsi_host_put(cfg->host);
 		/* Fall through */
 	case INIT_STATE_AFU:
 		term_afu(cfg);
@@ -744,6 +743,7 @@  static void cxlflash_remove(struct pci_dev *pdev)
 	case INIT_STATE_NONE:
 		flush_work(&cfg->work_q);
 		free_mem(cfg);
+		scsi_host_put(cfg->host);
 		break;
 	}
 
@@ -2415,6 +2415,7 @@  static int cxlflash_probe(struct pci_dev *pdev,
 		dev_err(&pdev->dev, "%s: call to scsi_host_alloc failed!\n",
 			__func__);
 		rc = -ENOMEM;
+		scsi_host_put(cfg->host);
 		goto out;
 	}