[PATCH v4 30/32] cxlflash: Fix to avoid corrupting adapter fops

[Daniel Axtens]

"Matthew R. Ochs" <mrochs at linux.vnet.ibm.com> writes:

> The corruption that this fix remedies is due to the fact that the fops
> is initially defaulted to values found within a static structure. When
> the fops is handed down to the CXL services later in the attach path,
> certain services are patched. The fops structure remains correct until
> the user count drops to 0 and the fops is reset, triggering the process
> to repeat again. The user counts are tightly coupled with the creation
> and deletion of the user context. If multiple users perform a disk
> attach at the same time, when the user count is currently 0, some users
> can be in the middle of obtaining a file descriptor and have not yet
> reached the context creation code that [in addition to creating the
> context] increments the user count. Subsequent users coming in to
> perform the attach see that the user count is still 0, and reinitialize
> the fops, temporarily removing the patched fops. The users that are in
> the middle obtaining their file descriptor may then receive an invalid
> descriptor.
>
> The fix simply removes the user count altogether and moves the fops
> initialization to probe time such that it is only performed one time
> for the life of the adapter. In the future, if the CXL services adopt
> a private member for their context, that could be used to store the
> adapter structure reference and cxlflash could revert to a model that
> does not require an embedded fops.

Yep, this looks good.

We have discussed adding a private data field to a cxl context, and will
no doubt revisit the question at some point in the future :)

Reviewed-by: Daniel Axtens <dja at axtens.net>

>
> Signed-off-by: Matthew R. Ochs <mrochs at linux.vnet.ibm.com>
> Signed-off-by: Manoj N. Kumar <manoj at linux.vnet.ibm.com>
> ---
>  drivers/scsi/cxlflash/common.h    |  3 +--
>  drivers/scsi/cxlflash/main.c      |  1 +
>  drivers/scsi/cxlflash/superpipe.c | 11 +----------
>  3 files changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/cxlflash/common.h b/drivers/scsi/cxlflash/common.h
> index bbfe711..c11cd19 100644
> --- a/drivers/scsi/cxlflash/common.h
> +++ b/drivers/scsi/cxlflash/common.h
> @@ -21,6 +21,7 @@
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_device.h>
>  
> +extern const struct file_operations cxlflash_cxl_fops;
>  
>  #define MAX_CONTEXT  CXLFLASH_MAX_CONTEXT       /* num contexts per afu */
>  
> @@ -115,8 +116,6 @@ struct cxlflash_cfg {
>  	struct list_head ctx_err_recovery; /* contexts w/ recovery pending */
>  	struct file_operations cxl_fops;
>  
> -	atomic_t num_user_contexts;
> -
>  	/* Parameters that are LUN table related */
>  	int last_lun_index[CXLFLASH_NUM_FC_PORTS];
>  	int promote_lun_index;
> diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
> index be78906..38e7edc 100644
> --- a/drivers/scsi/cxlflash/main.c
> +++ b/drivers/scsi/cxlflash/main.c
> @@ -2386,6 +2386,7 @@ static int cxlflash_probe(struct pci_dev *pdev,
>  
>  	cfg->init_state = INIT_STATE_NONE;
>  	cfg->dev = pdev;
> +	cfg->cxl_fops = cxlflash_cxl_fops;
>  
>  	/*
>  	 * The promoted LUNs move to the top of the LUN table. The rest stay
> diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
> index 3cc8609..f625e07 100644
> --- a/drivers/scsi/cxlflash/superpipe.c
> +++ b/drivers/scsi/cxlflash/superpipe.c
> @@ -712,7 +712,6 @@ static void destroy_context(struct cxlflash_cfg *cfg,
>  	kfree(ctxi->rht_needs_ws);
>  	kfree(ctxi->rht_lun);
>  	kfree(ctxi);
> -	atomic_dec_if_positive(&cfg->num_user_contexts);
>  }
>  
>  /**
> @@ -769,7 +768,6 @@ static struct ctx_info *create_context(struct cxlflash_cfg *cfg,
>  	INIT_LIST_HEAD(&ctxi->luns);
>  	INIT_LIST_HEAD(&ctxi->list); /* initialize for list_empty() */
>  
> -	atomic_inc(&cfg->num_user_contexts);
>  	mutex_lock(&ctxi->mutex);
>  out:
>  	return ctxi;
> @@ -1164,10 +1162,7 @@ out:
>  	return rc;
>  }
>  
> -/*
> - * Local fops for adapter file descriptor
> - */
> -static const struct file_operations cxlflash_cxl_fops = {
> +const struct file_operations cxlflash_cxl_fops = {
>  	.owner = THIS_MODULE,
>  	.mmap = cxlflash_cxl_mmap,
>  	.release = cxlflash_cxl_release,
> @@ -1286,10 +1281,6 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
>  
>  	int fd = -1;
>  
> -	/* On first attach set fileops */
> -	if (atomic_read(&cfg->num_user_contexts) == 0)
> -		cfg->cxl_fops = cxlflash_cxl_fops;
> -
>  	if (attach->num_interrupts > 4) {
>  		dev_dbg(dev, "%s: Cannot support this many interrupts %llu\n",
>  			__func__, attach->num_interrupts);
> -- 
> 2.1.0
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 859 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20150930/5753e16b/attachment.sig>