diff mbox series

lpfc: Decouple port_template and vport_template

Message ID 20220705094203.50154-1-dwagner@suse.de
State New
Headers show
Series lpfc: Decouple port_template and vport_template | expand

Commit Message

Daniel Wagner July 5, 2022, 9:42 a.m. UTC
From: "Dwip N. Banerjee" <dnbanerg@us.ibm.com>

The problem here is that the lpfc_hba structure has been freed but the
Scsi_Host's hostt pointer is still pointing to the (v) port template
area inside the freed hba structure - through which the module is
accessed.

Basically we need to ensure that the access to the module structure
(via the host template or otherwise) stays valid even after the HBA
structure is freed (or delay that free).

Signed-off-by: Dwip N. Banerjee <dnbanerg@us.ibm.com>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
Hi,

This patch is in our downstream kernels for a while. I've forward
ported so we also fix the upstream driver for everyone.

@Dwip, I took the liberty to add your SoB, hope this is okay.

Daniel

 drivers/scsi/lpfc/lpfc.h      |  5 -----
 drivers/scsi/lpfc/lpfc_crtn.h |  1 +
 drivers/scsi/lpfc/lpfc_init.c | 22 +++++++---------------
 drivers/scsi/lpfc/lpfc_scsi.c | 28 ++++++++++++++++++++++++++++
 4 files changed, 36 insertions(+), 20 deletions(-)

Comments

Daniel Wagner July 21, 2022, 9:49 a.m. UTC | #1
On Tue, Jul 05, 2022 at 11:42:03AM +0200, Daniel Wagner wrote:
> From: "Dwip N. Banerjee" <dnbanerg@us.ibm.com>
> 
> The problem here is that the lpfc_hba structure has been freed but the
> Scsi_Host's hostt pointer is still pointing to the (v) port template
> area inside the freed hba structure - through which the module is
> accessed.
> 
> Basically we need to ensure that the access to the module structure
> (via the host template or otherwise) stays valid even after the HBA
> structure is freed (or delay that free).
> 
> Signed-off-by: Dwip N. Banerjee <dnbanerg@us.ibm.com>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
> Hi,
> 
> This patch is in our downstream kernels for a while. I've forward
> ported so we also fix the upstream driver for everyone.
> 
> @Dwip, I took the liberty to add your SoB, hope this is okay.
> 
> Daniel

ping

> 
>  drivers/scsi/lpfc/lpfc.h      |  5 -----
>  drivers/scsi/lpfc/lpfc_crtn.h |  1 +
>  drivers/scsi/lpfc/lpfc_init.c | 22 +++++++---------------
>  drivers/scsi/lpfc/lpfc_scsi.c | 28 ++++++++++++++++++++++++++++
>  4 files changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/lpfc/lpfc.h b/drivers/scsi/lpfc/lpfc.h
> index da9070cdad91..4c72e639fdaf 100644
> --- a/drivers/scsi/lpfc/lpfc.h
> +++ b/drivers/scsi/lpfc/lpfc.h
> @@ -1616,11 +1616,6 @@ struct lpfc_hba {
>  #define LPFC_POLL_SLOWPATH	1	/* called from slowpath */
>  
>  	char os_host_name[MAXHOSTNAMELEN];
> -
> -	/* SCSI host template information - for physical port */
> -	struct scsi_host_template port_template;
> -	/* SCSI host template information - for all vports */
> -	struct scsi_host_template vport_template;
>  	atomic_t dbg_log_idx;
>  	atomic_t dbg_log_cnt;
>  	atomic_t dbg_log_dmping;
> diff --git a/drivers/scsi/lpfc/lpfc_crtn.h b/drivers/scsi/lpfc/lpfc_crtn.h
> index b1be0dd0337a..4331b85c2e79 100644
> --- a/drivers/scsi/lpfc/lpfc_crtn.h
> +++ b/drivers/scsi/lpfc/lpfc_crtn.h
> @@ -456,6 +456,7 @@ extern const struct attribute_group *lpfc_hba_groups[];
>  extern const struct attribute_group *lpfc_vport_groups[];
>  extern struct scsi_host_template lpfc_template;
>  extern struct scsi_host_template lpfc_template_nvme;
> +extern struct scsi_host_template lpfc_vport_template;
>  extern struct fc_function_template lpfc_transport_functions;
>  extern struct fc_function_template lpfc_vport_transport_functions;
>  
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index 93b94c64518d..4d1e813a94db 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -4686,7 +4686,7 @@ lpfc_create_port(struct lpfc_hba *phba, int instance, struct device *dev)
>  {
>  	struct lpfc_vport *vport;
>  	struct Scsi_Host  *shost = NULL;
> -	struct scsi_host_template *template;
> +	struct scsi_host_template *template, *vport_template;
>  	int error = 0;
>  	int i;
>  	uint64_t wwn;
> @@ -4718,42 +4718,34 @@ lpfc_create_port(struct lpfc_hba *phba, int instance, struct device *dev)
>  
>  	/* Seed template for SCSI host registration */
>  	if (dev == &phba->pcidev->dev) {
> -		template = &phba->port_template;
> -
>  		if (phba->cfg_enable_fc4_type & LPFC_ENABLE_FCP) {
>  			/* Seed physical port template */
> -			memcpy(template, &lpfc_template, sizeof(*template));
> +			template = &lpfc_template;
>  
>  			if (use_no_reset_hba)
>  				/* template is for a no reset SCSI Host */
>  				template->eh_host_reset_handler = NULL;
>  
>  			/* Template for all vports this physical port creates */
> -			memcpy(&phba->vport_template, &lpfc_template,
> -			       sizeof(*template));
> -			phba->vport_template.shost_groups = lpfc_vport_groups;
> -			phba->vport_template.eh_bus_reset_handler = NULL;
> -			phba->vport_template.eh_host_reset_handler = NULL;
> -			phba->vport_template.vendor_id = 0;
> +			vport_template = &lpfc_vport_template;
>  
>  			/* Initialize the host templates with updated value */
>  			if (phba->sli_rev == LPFC_SLI_REV4) {
>  				template->sg_tablesize = phba->cfg_scsi_seg_cnt;
> -				phba->vport_template.sg_tablesize =
> +				vport_template->sg_tablesize =
>  					phba->cfg_scsi_seg_cnt;
>  			} else {
>  				template->sg_tablesize = phba->cfg_sg_seg_cnt;
> -				phba->vport_template.sg_tablesize =
> +				vport_template->sg_tablesize =
>  					phba->cfg_sg_seg_cnt;
>  			}
>  
>  		} else {
>  			/* NVMET is for physical port only */
> -			memcpy(template, &lpfc_template_nvme,
> -			       sizeof(*template));
> +			template = &lpfc_template_nvme;
>  		}
>  	} else {
> -		template = &phba->vport_template;
> +		template = &lpfc_vport_template;
>  	}
>  
>  	shost = scsi_host_alloc(template, sizeof(struct lpfc_vport));
> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index d43968203248..5b6368428cb8 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -6848,3 +6848,31 @@ struct scsi_host_template lpfc_template = {
>  	.change_queue_depth	= scsi_change_queue_depth,
>  	.track_queue_depth	= 1,
>  };
> +
> +/* Template for all vports this physical port creates */
> +struct scsi_host_template lpfc_vport_template = {
> +	.module			= THIS_MODULE,
> +	.name			= LPFC_DRIVER_NAME,
> +	.proc_name		= LPFC_DRIVER_NAME,
> +	.info			= lpfc_info,
> +	.queuecommand		= lpfc_queuecommand,
> +	.eh_timed_out		= fc_eh_timed_out,
> +	.eh_should_retry_cmd    = fc_eh_should_retry_cmd,
> +	.eh_abort_handler	= lpfc_abort_handler,
> +	.eh_device_reset_handler = lpfc_device_reset_handler,
> +	.eh_target_reset_handler = lpfc_target_reset_handler,
> +	.eh_bus_reset_handler	= NULL,
> +	.eh_host_reset_handler  = NULL,
> +	.slave_alloc		= lpfc_slave_alloc,
> +	.slave_configure	= lpfc_slave_configure,
> +	.slave_destroy		= lpfc_slave_destroy,
> +	.scan_finished		= lpfc_scan_finished,
> +	.this_id		= -1,
> +	.sg_tablesize		= LPFC_DEFAULT_SG_SEG_CNT,
> +	.cmd_per_lun		= LPFC_CMD_PER_LUN,
> +	.shost_groups		= lpfc_vport_groups,
> +	.max_sectors		= 0xFFFFFFFF,
> +	.vendor_id		= 0,
> +	.change_queue_depth	= scsi_change_queue_depth,
> +	.track_queue_depth	= 1,
> +};
> -- 
> 2.29.2
>
James Smart Aug. 2, 2022, 10:29 p.m. UTC | #2
On 7/21/2022 2:49 AM, Daniel Wagner wrote:
> On Tue, Jul 05, 2022 at 11:42:03AM +0200, Daniel Wagner wrote:
>> From: "Dwip N. Banerjee" <dnbanerg@us.ibm.com>
>>
>> The problem here is that the lpfc_hba structure has been freed but the
>> Scsi_Host's hostt pointer is still pointing to the (v) port template
>> area inside the freed hba structure - through which the module is
>> accessed.
>>
>> Basically we need to ensure that the access to the module structure
>> (via the host template or otherwise) stays valid even after the HBA
>> structure is freed (or delay that free).
>>
>> Signed-off-by: Dwip N. Banerjee <dnbanerg@us.ibm.com>
>> Signed-off-by: Daniel Wagner <dwagner@suse.de>
>> ---
>> Hi,
>>
>> This patch is in our downstream kernels for a while. I've forward
>> ported so we also fix the upstream driver for everyone.
>>
>> @Dwip, I took the liberty to add your SoB, hope this is okay.
>>
>> Daniel

We are reproducing this issue now, but think the fix should be slightly 
different. We're putting it together for testing.

-- james
diff mbox series

Patch

diff --git a/drivers/scsi/lpfc/lpfc.h b/drivers/scsi/lpfc/lpfc.h
index da9070cdad91..4c72e639fdaf 100644
--- a/drivers/scsi/lpfc/lpfc.h
+++ b/drivers/scsi/lpfc/lpfc.h
@@ -1616,11 +1616,6 @@  struct lpfc_hba {
 #define LPFC_POLL_SLOWPATH	1	/* called from slowpath */
 
 	char os_host_name[MAXHOSTNAMELEN];
-
-	/* SCSI host template information - for physical port */
-	struct scsi_host_template port_template;
-	/* SCSI host template information - for all vports */
-	struct scsi_host_template vport_template;
 	atomic_t dbg_log_idx;
 	atomic_t dbg_log_cnt;
 	atomic_t dbg_log_dmping;
diff --git a/drivers/scsi/lpfc/lpfc_crtn.h b/drivers/scsi/lpfc/lpfc_crtn.h
index b1be0dd0337a..4331b85c2e79 100644
--- a/drivers/scsi/lpfc/lpfc_crtn.h
+++ b/drivers/scsi/lpfc/lpfc_crtn.h
@@ -456,6 +456,7 @@  extern const struct attribute_group *lpfc_hba_groups[];
 extern const struct attribute_group *lpfc_vport_groups[];
 extern struct scsi_host_template lpfc_template;
 extern struct scsi_host_template lpfc_template_nvme;
+extern struct scsi_host_template lpfc_vport_template;
 extern struct fc_function_template lpfc_transport_functions;
 extern struct fc_function_template lpfc_vport_transport_functions;
 
diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index 93b94c64518d..4d1e813a94db 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -4686,7 +4686,7 @@  lpfc_create_port(struct lpfc_hba *phba, int instance, struct device *dev)
 {
 	struct lpfc_vport *vport;
 	struct Scsi_Host  *shost = NULL;
-	struct scsi_host_template *template;
+	struct scsi_host_template *template, *vport_template;
 	int error = 0;
 	int i;
 	uint64_t wwn;
@@ -4718,42 +4718,34 @@  lpfc_create_port(struct lpfc_hba *phba, int instance, struct device *dev)
 
 	/* Seed template for SCSI host registration */
 	if (dev == &phba->pcidev->dev) {
-		template = &phba->port_template;
-
 		if (phba->cfg_enable_fc4_type & LPFC_ENABLE_FCP) {
 			/* Seed physical port template */
-			memcpy(template, &lpfc_template, sizeof(*template));
+			template = &lpfc_template;
 
 			if (use_no_reset_hba)
 				/* template is for a no reset SCSI Host */
 				template->eh_host_reset_handler = NULL;
 
 			/* Template for all vports this physical port creates */
-			memcpy(&phba->vport_template, &lpfc_template,
-			       sizeof(*template));
-			phba->vport_template.shost_groups = lpfc_vport_groups;
-			phba->vport_template.eh_bus_reset_handler = NULL;
-			phba->vport_template.eh_host_reset_handler = NULL;
-			phba->vport_template.vendor_id = 0;
+			vport_template = &lpfc_vport_template;
 
 			/* Initialize the host templates with updated value */
 			if (phba->sli_rev == LPFC_SLI_REV4) {
 				template->sg_tablesize = phba->cfg_scsi_seg_cnt;
-				phba->vport_template.sg_tablesize =
+				vport_template->sg_tablesize =
 					phba->cfg_scsi_seg_cnt;
 			} else {
 				template->sg_tablesize = phba->cfg_sg_seg_cnt;
-				phba->vport_template.sg_tablesize =
+				vport_template->sg_tablesize =
 					phba->cfg_sg_seg_cnt;
 			}
 
 		} else {
 			/* NVMET is for physical port only */
-			memcpy(template, &lpfc_template_nvme,
-			       sizeof(*template));
+			template = &lpfc_template_nvme;
 		}
 	} else {
-		template = &phba->vport_template;
+		template = &lpfc_vport_template;
 	}
 
 	shost = scsi_host_alloc(template, sizeof(struct lpfc_vport));
diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index d43968203248..5b6368428cb8 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -6848,3 +6848,31 @@  struct scsi_host_template lpfc_template = {
 	.change_queue_depth	= scsi_change_queue_depth,
 	.track_queue_depth	= 1,
 };
+
+/* Template for all vports this physical port creates */
+struct scsi_host_template lpfc_vport_template = {
+	.module			= THIS_MODULE,
+	.name			= LPFC_DRIVER_NAME,
+	.proc_name		= LPFC_DRIVER_NAME,
+	.info			= lpfc_info,
+	.queuecommand		= lpfc_queuecommand,
+	.eh_timed_out		= fc_eh_timed_out,
+	.eh_should_retry_cmd    = fc_eh_should_retry_cmd,
+	.eh_abort_handler	= lpfc_abort_handler,
+	.eh_device_reset_handler = lpfc_device_reset_handler,
+	.eh_target_reset_handler = lpfc_target_reset_handler,
+	.eh_bus_reset_handler	= NULL,
+	.eh_host_reset_handler  = NULL,
+	.slave_alloc		= lpfc_slave_alloc,
+	.slave_configure	= lpfc_slave_configure,
+	.slave_destroy		= lpfc_slave_destroy,
+	.scan_finished		= lpfc_scan_finished,
+	.this_id		= -1,
+	.sg_tablesize		= LPFC_DEFAULT_SG_SEG_CNT,
+	.cmd_per_lun		= LPFC_CMD_PER_LUN,
+	.shost_groups		= lpfc_vport_groups,
+	.max_sectors		= 0xFFFFFFFF,
+	.vendor_id		= 0,
+	.change_queue_depth	= scsi_change_queue_depth,
+	.track_queue_depth	= 1,
+};