linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal
@ 2015-09-16 16:52 Matthew R. Ochs
  0 siblings, 0 replies; 6+ messages in thread
From: Matthew R. Ochs @ 2015-09-16 16:52 UTC (permalink / raw)
  To: linux-scsi, James.Bottomley, nab, brking, imunsie, dja,
	andrew.donnellan
  Cc: mikey, linuxppc-dev, Manoj N. Kumar

When a LUN is removed, the sdev that is associated with the LUN
remains intact until its reference count drops to 0. In order
to prevent an sdev from being removed while a context is still
associated with it, obtain an additional reference per-context
for each LUN attached to the context.

This resolves a potential Oops in the release handler when a
dealing with a LUN that has already been removed.

Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
Suggested-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/superpipe.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index fa513ba..1fa4af6 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -880,6 +880,9 @@ static int _cxlflash_disk_detach(struct scsi_device *sdev,
 			sys_close(lfd);
 	}
 
+	/* Release the sdev reference that bound this LUN to the context */
+	scsi_device_put(sdev);
+
 out:
 	if (put_ctx)
 		put_context(ctxi);
@@ -1287,11 +1290,18 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 			}
 	}
 
+	rc = scsi_device_get(sdev);
+	if (unlikely(rc)) {
+		dev_err(dev, "%s: Unable to get sdev reference!\n", __func__);
+		goto out;
+	}
+
 	lun_access = kzalloc(sizeof(*lun_access), GFP_KERNEL);
 	if (unlikely(!lun_access)) {
 		dev_err(dev, "%s: Unable to allocate lun_access!\n", __func__);
+		scsi_device_put(sdev);
 		rc = -ENOMEM;
-		goto out;
+		goto err0;
 	}
 
 	lun_access->lli = lli;
@@ -1311,21 +1321,21 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 		dev_err(dev, "%s: Could not initialize context %p\n",
 			__func__, ctx);
 		rc = -ENODEV;
-		goto err0;
+		goto err1;
 	}
 
 	ctxid = cxl_process_element(ctx);
 	if (unlikely((ctxid > MAX_CONTEXT) || (ctxid < 0))) {
 		dev_err(dev, "%s: ctxid (%d) invalid!\n", __func__, ctxid);
 		rc = -EPERM;
-		goto err1;
+		goto err2;
 	}
 
 	file = cxl_get_fd(ctx, &cfg->cxl_fops, &fd);
 	if (unlikely(fd < 0)) {
 		rc = -ENODEV;
 		dev_err(dev, "%s: Could not get file descriptor\n", __func__);
-		goto err1;
+		goto err2;
 	}
 
 	/* Translate read/write O_* flags from fcntl.h to AFU permission bits */
@@ -1335,7 +1345,7 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 	if (unlikely(!ctxi)) {
 		dev_err(dev, "%s: Failed to create context! (%d)\n",
 			__func__, ctxid);
-		goto err2;
+		goto err3;
 	}
 
 	work = &ctxi->work;
@@ -1346,13 +1356,13 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 	if (unlikely(rc)) {
 		dev_dbg(dev, "%s: Could not start context rc=%d\n",
 			__func__, rc);
-		goto err3;
+		goto err4;
 	}
 
 	rc = afu_attach(cfg, ctxi);
 	if (unlikely(rc)) {
 		dev_err(dev, "%s: Could not attach AFU rc %d\n", __func__, rc);
-		goto err4;
+		goto err5;
 	}
 
 	/*
@@ -1388,13 +1398,13 @@ out:
 		__func__, ctxid, fd, attach->block_size, rc, attach->last_lba);
 	return rc;
 
-err4:
+err5:
 	cxl_stop_context(ctx);
-err3:
+err4:
 	put_context(ctxi);
 	destroy_context(cfg, ctxi);
 	ctxi = NULL;
-err2:
+err3:
 	/*
 	 * Here, we're overriding the fops with a dummy all-NULL fops because
 	 * fput() calls the release fop, which will cause us to mistakenly
@@ -1406,10 +1416,12 @@ err2:
 	fput(file);
 	put_unused_fd(fd);
 	fd = -1;
-err1:
+err2:
 	cxl_release_context(ctx);
-err0:
+err1:
 	kfree(lun_access);
+err0:
+	scsi_device_put(sdev);
 	goto out;
 }
 
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal
  2015-09-16 21:23 [PATCH v2 00/30] cxlflash: Miscellaneous bug fixes and corrections Matthew R. Ochs
@ 2015-09-16 21:27 ` Matthew R. Ochs
  2015-09-18  1:26   ` Brian King
  2015-09-21 12:11   ` Tomas Henzl
  0 siblings, 2 replies; 6+ messages in thread
From: Matthew R. Ochs @ 2015-09-16 21:27 UTC (permalink / raw)
  To: linux-scsi, James Bottomley, Nicholas A. Bellinger, Brian King,
	Ian Munsie, Daniel Axtens, Andrew Donnellan
  Cc: Michael Neuling, linuxppc-dev, Manoj N. Kumar

When a LUN is removed, the sdev that is associated with the LUN
remains intact until its reference count drops to 0. In order
to prevent an sdev from being removed while a context is still
associated with it, obtain an additional reference per-context
for each LUN attached to the context.

This resolves a potential Oops in the release handler when a
dealing with a LUN that has already been removed.

Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
Suggested-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/superpipe.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index fa513ba..1fa4af6 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -880,6 +880,9 @@ static int _cxlflash_disk_detach(struct scsi_device *sdev,
 			sys_close(lfd);
 	}
 
+	/* Release the sdev reference that bound this LUN to the context */
+	scsi_device_put(sdev);
+
 out:
 	if (put_ctx)
 		put_context(ctxi);
@@ -1287,11 +1290,18 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 			}
 	}
 
+	rc = scsi_device_get(sdev);
+	if (unlikely(rc)) {
+		dev_err(dev, "%s: Unable to get sdev reference!\n", __func__);
+		goto out;
+	}
+
 	lun_access = kzalloc(sizeof(*lun_access), GFP_KERNEL);
 	if (unlikely(!lun_access)) {
 		dev_err(dev, "%s: Unable to allocate lun_access!\n", __func__);
+		scsi_device_put(sdev);
 		rc = -ENOMEM;
-		goto out;
+		goto err0;
 	}
 
 	lun_access->lli = lli;
@@ -1311,21 +1321,21 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 		dev_err(dev, "%s: Could not initialize context %p\n",
 			__func__, ctx);
 		rc = -ENODEV;
-		goto err0;
+		goto err1;
 	}
 
 	ctxid = cxl_process_element(ctx);
 	if (unlikely((ctxid > MAX_CONTEXT) || (ctxid < 0))) {
 		dev_err(dev, "%s: ctxid (%d) invalid!\n", __func__, ctxid);
 		rc = -EPERM;
-		goto err1;
+		goto err2;
 	}
 
 	file = cxl_get_fd(ctx, &cfg->cxl_fops, &fd);
 	if (unlikely(fd < 0)) {
 		rc = -ENODEV;
 		dev_err(dev, "%s: Could not get file descriptor\n", __func__);
-		goto err1;
+		goto err2;
 	}
 
 	/* Translate read/write O_* flags from fcntl.h to AFU permission bits */
@@ -1335,7 +1345,7 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 	if (unlikely(!ctxi)) {
 		dev_err(dev, "%s: Failed to create context! (%d)\n",
 			__func__, ctxid);
-		goto err2;
+		goto err3;
 	}
 
 	work = &ctxi->work;
@@ -1346,13 +1356,13 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
 	if (unlikely(rc)) {
 		dev_dbg(dev, "%s: Could not start context rc=%d\n",
 			__func__, rc);
-		goto err3;
+		goto err4;
 	}
 
 	rc = afu_attach(cfg, ctxi);
 	if (unlikely(rc)) {
 		dev_err(dev, "%s: Could not attach AFU rc %d\n", __func__, rc);
-		goto err4;
+		goto err5;
 	}
 
 	/*
@@ -1388,13 +1398,13 @@ out:
 		__func__, ctxid, fd, attach->block_size, rc, attach->last_lba);
 	return rc;
 
-err4:
+err5:
 	cxl_stop_context(ctx);
-err3:
+err4:
 	put_context(ctxi);
 	destroy_context(cfg, ctxi);
 	ctxi = NULL;
-err2:
+err3:
 	/*
 	 * Here, we're overriding the fops with a dummy all-NULL fops because
 	 * fput() calls the release fop, which will cause us to mistakenly
@@ -1406,10 +1416,12 @@ err2:
 	fput(file);
 	put_unused_fd(fd);
 	fd = -1;
-err1:
+err2:
 	cxl_release_context(ctx);
-err0:
+err1:
 	kfree(lun_access);
+err0:
+	scsi_device_put(sdev);
 	goto out;
 }
 
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal
  2015-09-16 21:27 ` [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal Matthew R. Ochs
@ 2015-09-18  1:26   ` Brian King
  2015-09-18 23:18     ` Matthew R. Ochs
  2015-09-21 12:11   ` Tomas Henzl
  1 sibling, 1 reply; 6+ messages in thread
From: Brian King @ 2015-09-18  1:26 UTC (permalink / raw)
  To: Matthew R. Ochs, linux-scsi, James Bottomley,
	Nicholas A. Bellinger, Ian Munsie, Daniel Axtens,
	Andrew Donnellan
  Cc: Michael Neuling, linuxppc-dev, Manoj N. Kumar

On 09/16/2015 04:27 PM, Matthew R. Ochs wrote:
> When a LUN is removed, the sdev that is associated with the LUN
> remains intact until its reference count drops to 0. In order
> to prevent an sdev from being removed while a context is still
> associated with it, obtain an additional reference per-context
> for each LUN attached to the context.
> 
> This resolves a potential Oops in the release handler when a
> dealing with a LUN that has already been removed.
> 
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
> Suggested-by: Brian King <brking@linux.vnet.ibm.com>
> ---
>  drivers/scsi/cxlflash/superpipe.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
> index fa513ba..1fa4af6 100644
> --- a/drivers/scsi/cxlflash/superpipe.c
> +++ b/drivers/scsi/cxlflash/superpipe.c
> @@ -880,6 +880,9 @@ static int _cxlflash_disk_detach(struct scsi_device *sdev,
>  			sys_close(lfd);
>  	}
> 
> +	/* Release the sdev reference that bound this LUN to the context */
> +	scsi_device_put(sdev);
> +
>  out:
>  	if (put_ctx)
>  		put_context(ctxi);
> @@ -1287,11 +1290,18 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
>  			}
>  	}
> 
> +	rc = scsi_device_get(sdev);
> +	if (unlikely(rc)) {
> +		dev_err(dev, "%s: Unable to get sdev reference!\n", __func__);
> +		goto out;
> +	}
> +
>  	lun_access = kzalloc(sizeof(*lun_access), GFP_KERNEL);
>  	if (unlikely(!lun_access)) {
>  		dev_err(dev, "%s: Unable to allocate lun_access!\n", __func__);
> +		scsi_device_put(sdev);

Looks like you've got a double scsi_device_put in this path, since there is another put
in the the err0 path.

>  		rc = -ENOMEM;
> -		goto out;
> +		goto err0;
>  	}
> 
>  	lun_access->lli = lli;
> @@ -1311,21 +1321,21 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
>  		dev_err(dev, "%s: Could not initialize context %p\n",
>  			__func__, ctx);
>  		rc = -ENODEV;
> -		goto err0;
> +		goto err1;
>  	}
> 
>  	ctxid = cxl_process_element(ctx);
>  	if (unlikely((ctxid > MAX_CONTEXT) || (ctxid < 0))) {
>  		dev_err(dev, "%s: ctxid (%d) invalid!\n", __func__, ctxid);
>  		rc = -EPERM;
> -		goto err1;
> +		goto err2;
>  	}
> 
>  	file = cxl_get_fd(ctx, &cfg->cxl_fops, &fd);
>  	if (unlikely(fd < 0)) {
>  		rc = -ENODEV;
>  		dev_err(dev, "%s: Could not get file descriptor\n", __func__);
> -		goto err1;
> +		goto err2;
>  	}
> 
>  	/* Translate read/write O_* flags from fcntl.h to AFU permission bits */
> @@ -1335,7 +1345,7 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
>  	if (unlikely(!ctxi)) {
>  		dev_err(dev, "%s: Failed to create context! (%d)\n",
>  			__func__, ctxid);
> -		goto err2;
> +		goto err3;
>  	}
> 
>  	work = &ctxi->work;
> @@ -1346,13 +1356,13 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
>  	if (unlikely(rc)) {
>  		dev_dbg(dev, "%s: Could not start context rc=%d\n",
>  			__func__, rc);
> -		goto err3;
> +		goto err4;
>  	}
> 
>  	rc = afu_attach(cfg, ctxi);
>  	if (unlikely(rc)) {
>  		dev_err(dev, "%s: Could not attach AFU rc %d\n", __func__, rc);
> -		goto err4;
> +		goto err5;
>  	}
> 
>  	/*
> @@ -1388,13 +1398,13 @@ out:
>  		__func__, ctxid, fd, attach->block_size, rc, attach->last_lba);
>  	return rc;
> 
> -err4:
> +err5:
>  	cxl_stop_context(ctx);
> -err3:
> +err4:
>  	put_context(ctxi);
>  	destroy_context(cfg, ctxi);
>  	ctxi = NULL;
> -err2:
> +err3:
>  	/*
>  	 * Here, we're overriding the fops with a dummy all-NULL fops because
>  	 * fput() calls the release fop, which will cause us to mistakenly
> @@ -1406,10 +1416,12 @@ err2:
>  	fput(file);
>  	put_unused_fd(fd);
>  	fd = -1;
> -err1:
> +err2:
>  	cxl_release_context(ctx);
> -err0:
> +err1:
>  	kfree(lun_access);
> +err0:
> +	scsi_device_put(sdev);
>  	goto out;
>  }
> 


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal
  2015-09-18  1:26   ` Brian King
@ 2015-09-18 23:18     ` Matthew R. Ochs
  0 siblings, 0 replies; 6+ messages in thread
From: Matthew R. Ochs @ 2015-09-18 23:18 UTC (permalink / raw)
  To: Brian King
  Cc: linux-scsi, James Bottomley, Nicholas A. Bellinger, Ian Munsie,
	Daniel Axtens, Andrew Donnellan, Michael Neuling, linuxppc-dev,
	Manoj N. Kumar

> On Sep 17, 2015, at 8:26 PM, Brian King <brking@linux.vnet.ibm.com> =
wrote:
>=20
> On 09/16/2015 04:27 PM, Matthew R. Ochs wrote:
>>=20
>> 	lun_access =3D kzalloc(sizeof(*lun_access), GFP_KERNEL);
>> 	if (unlikely(!lun_access)) {
>> 		dev_err(dev, "%s: Unable to allocate lun_access!\n", =
__func__);
>> +		scsi_device_put(sdev);
>=20
> Looks like you've got a double scsi_device_put in this path, since =
there is another put
> in the the err0 path.

Good catch! I'll fix in v3.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal
  2015-09-16 21:27 ` [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal Matthew R. Ochs
  2015-09-18  1:26   ` Brian King
@ 2015-09-21 12:11   ` Tomas Henzl
  2015-09-21 22:32     ` Matthew R. Ochs
  1 sibling, 1 reply; 6+ messages in thread
From: Tomas Henzl @ 2015-09-21 12:11 UTC (permalink / raw)
  To: Matthew R. Ochs, linux-scsi, James Bottomley,
	Nicholas A. Bellinger, Brian King, Ian Munsie, Daniel Axtens,
	Andrew Donnellan
  Cc: Michael Neuling, linuxppc-dev, Manoj N. Kumar

On 16.9.2015 23:27, Matthew R. Ochs wrote:
> When a LUN is removed, the sdev that is associated with the LUN
> remains intact until its reference count drops to 0. In order
> to prevent an sdev from being removed while a context is still
> associated with it, obtain an additional reference per-context
> for each LUN attached to the context.
>
> This resolves a potential Oops in the release handler when a
> dealing with a LUN that has already been removed.
>
> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
> Suggested-by: Brian King <brking@linux.vnet.ibm.com>
> ---
>  drivers/scsi/cxlflash/superpipe.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
> index fa513ba..1fa4af6 100644
> --- a/drivers/scsi/cxlflash/superpipe.c
> +++ b/drivers/scsi/cxlflash/superpipe.c
> @@ -880,6 +880,9 @@ static int _cxlflash_disk_detach(struct scsi_device *sdev,
>  			sys_close(lfd);
>  	}
>  
> +	/* Release the sdev reference that bound this LUN to the context */
> +	scsi_device_put(sdev);
> +

I'm not sure here with the use if scsi_device_get+put, also I don't quite well
understand what you are going to fix here and how can it happen.
The scsi_device_get takes an additional module reference, so if used from
a module it shouldn't be held for a long time.
Is it possible for a user to rmmod the czlflash module
after the disk attach function is called?

Cheers,
--tm

>  out:
>  	if (put_ctx)
>  		put_context(ctxi);
> @@ -1287,11 +1290,18 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
>  			}
>  	}
>  
> +	rc = scsi_device_get(sdev);
> +	if (unlikely(rc)) {
> +		dev_err(dev, "%s: Unable to get sdev reference!\n", __func__);
> +		goto out;
> +	}
> +
>  	lun_access = kzalloc(sizeof(*lun_access), GFP_KERNEL);
>  	if (unlikely(!lun_access)) {
>  		dev_err(dev, "%s: Unable to allocate lun_access!\n", __func__);
> +		scsi_device_put(sdev);
>  		rc = -ENOMEM;
> -		goto out;
> +		goto err0;
>  	}
>  
>  	lun_access->lli = lli;
> @@ -1311,21 +1321,21 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
>  		dev_err(dev, "%s: Could not initialize context %p\n",
>  			__func__, ctx);
>  		rc = -ENODEV;
> -		goto err0;
> +		goto err1;
>  	}
>  
>  	ctxid = cxl_process_element(ctx);
>  	if (unlikely((ctxid > MAX_CONTEXT) || (ctxid < 0))) {
>  		dev_err(dev, "%s: ctxid (%d) invalid!\n", __func__, ctxid);
>  		rc = -EPERM;
> -		goto err1;
> +		goto err2;
>  	}
>  
>  	file = cxl_get_fd(ctx, &cfg->cxl_fops, &fd);
>  	if (unlikely(fd < 0)) {
>  		rc = -ENODEV;
>  		dev_err(dev, "%s: Could not get file descriptor\n", __func__);
> -		goto err1;
> +		goto err2;
>  	}
>  
>  	/* Translate read/write O_* flags from fcntl.h to AFU permission bits */
> @@ -1335,7 +1345,7 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
>  	if (unlikely(!ctxi)) {
>  		dev_err(dev, "%s: Failed to create context! (%d)\n",
>  			__func__, ctxid);
> -		goto err2;
> +		goto err3;
>  	}
>  
>  	work = &ctxi->work;
> @@ -1346,13 +1356,13 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
>  	if (unlikely(rc)) {
>  		dev_dbg(dev, "%s: Could not start context rc=%d\n",
>  			__func__, rc);
> -		goto err3;
> +		goto err4;
>  	}
>  
>  	rc = afu_attach(cfg, ctxi);
>  	if (unlikely(rc)) {
>  		dev_err(dev, "%s: Could not attach AFU rc %d\n", __func__, rc);
> -		goto err4;
> +		goto err5;
>  	}
>  
>  	/*
> @@ -1388,13 +1398,13 @@ out:
>  		__func__, ctxid, fd, attach->block_size, rc, attach->last_lba);
>  	return rc;
>  
> -err4:
> +err5:
>  	cxl_stop_context(ctx);
> -err3:
> +err4:
>  	put_context(ctxi);
>  	destroy_context(cfg, ctxi);
>  	ctxi = NULL;
> -err2:
> +err3:
>  	/*
>  	 * Here, we're overriding the fops with a dummy all-NULL fops because
>  	 * fput() calls the release fop, which will cause us to mistakenly
> @@ -1406,10 +1416,12 @@ err2:
>  	fput(file);
>  	put_unused_fd(fd);
>  	fd = -1;
> -err1:
> +err2:
>  	cxl_release_context(ctx);
> -err0:
> +err1:
>  	kfree(lun_access);
> +err0:
> +	scsi_device_put(sdev);
>  	goto out;
>  }
>  

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal
  2015-09-21 12:11   ` Tomas Henzl
@ 2015-09-21 22:32     ` Matthew R. Ochs
  0 siblings, 0 replies; 6+ messages in thread
From: Matthew R. Ochs @ 2015-09-21 22:32 UTC (permalink / raw)
  To: Tomas Henzl
  Cc: linux-scsi, James Bottomley, Nicholas A. Bellinger, Brian King,
	Ian Munsie, Daniel Axtens, Andrew Donnellan, Michael Neuling,
	linuxppc-dev, Manoj N. Kumar

> On Sep 21, 2015, at 7:11 AM, Tomas Henzl <thenzl@redhat.com> wrote:
> On 16.9.2015 23:27, Matthew R. Ochs wrote:
>> When a LUN is removed, the sdev that is associated with the LUN
>> remains intact until its reference count drops to 0. In order
>> to prevent an sdev from being removed while a context is still
>> associated with it, obtain an additional reference per-context
>> for each LUN attached to the context.
>>=20
>> This resolves a potential Oops in the release handler when a
>> dealing with a LUN that has already been removed.
>>=20
>> Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
>> Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
>> Suggested-by: Brian King <brking@linux.vnet.ibm.com>
>> ---
>> drivers/scsi/cxlflash/superpipe.c | 36 =
++++++++++++++++++++++++------------
>> 1 file changed, 24 insertions(+), 12 deletions(-)
>>=20
>> diff --git a/drivers/scsi/cxlflash/superpipe.c =
b/drivers/scsi/cxlflash/superpipe.c
>> index fa513ba..1fa4af6 100644
>> --- a/drivers/scsi/cxlflash/superpipe.c
>> +++ b/drivers/scsi/cxlflash/superpipe.c
>> @@ -880,6 +880,9 @@ static int _cxlflash_disk_detach(struct =
scsi_device *sdev,
>> 			sys_close(lfd);
>> 	}
>>=20
>> +	/* Release the sdev reference that bound this LUN to the context =
*/
>> +	scsi_device_put(sdev);
>> +
>=20
> I'm not sure here with the use if scsi_device_get+put, also I don't =
quite well
> understand what you are going to fix here and how can it happen.
> The scsi_device_get takes an additional module reference, so if used =
from
> a module it shouldn't be held for a long time.

The issue here is that the user context needs to be bound to the device =
so that
in the event that device goes away, it doesn't completely go away until =
the user
context is done using it. Without it, it is possible to crash when the =
context is
being freed.

Essentially this is the same as incrementing the count when an open is =
performed
on the device. The device can be removed (and is hidden upon doing so) =
but is
not actually freed until the reference is resolved (close()).

> Is it possible for a user to rmmod the czlflash module
> after the disk attach function is called?

Not while a user is present.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-09-21 22:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-16 16:52 [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal Matthew R. Ochs
  -- strict thread matches above, loose matches on Subject: below --
2015-09-16 21:23 [PATCH v2 00/30] cxlflash: Miscellaneous bug fixes and corrections Matthew R. Ochs
2015-09-16 21:27 ` [PATCH v2 04/30] cxlflash: Fix potential oops following LUN removal Matthew R. Ochs
2015-09-18  1:26   ` Brian King
2015-09-18 23:18     ` Matthew R. Ochs
2015-09-21 12:11   ` Tomas Henzl
2015-09-21 22:32     ` Matthew R. Ochs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).