diff mbox series

[v2,1/1] scsi: target: Fix unmap setup during configuration

Message ID 20240209215247.5213-1-michael.christie@oracle.com
State New
Headers show
Series [v2,1/1] scsi: target: Fix unmap setup during configuration | expand

Commit Message

Mike Christie Feb. 9, 2024, 9:52 p.m. UTC 
This issue was found and also debugged by Carl Lei <me@xecycle.info>.

If the device is not enabled, iblock/file will have not setup their
se_device to bdev/file mappings. If a user tries to config the unmap
settings at this time, we will then crash trying to access a NULL
pointer where the bdev/file should be.

This patch adds a check to make sure the device is configured before
we try to call the configure_unmap callout.

Fixes: 34bd1dcacf0d ("scsi: target: Detect UNMAP support post configuration")
Reported-by: Carl Lei <me@xecycle.info>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---

v2: Fix missing configure_unmap handling so failure is returned.

 drivers/target/target_core_configfs.c | 48 ++++++++++++++++++---------
 1 file changed, 32 insertions(+), 16 deletions(-)

Comments

Maurizio Lombardi Feb. 12, 2024, 8:07 a.m. UTC | #1 
pá 9. 2. 2024 v 22:52 odesílatel Mike Christie
<michael.christie@oracle.com> napsal:
>
> This issue was found and also debugged by Carl Lei <me@xecycle.info>.
>
> If the device is not enabled, iblock/file will have not setup their
> se_device to bdev/file mappings. If a user tries to config the unmap
> settings at this time, we will then crash trying to access a NULL
> pointer where the bdev/file should be.
>
> This patch adds a check to make sure the device is configured before
> we try to call the configure_unmap callout.
>
> Fixes: 34bd1dcacf0d ("scsi: target: Detect UNMAP support post configuration")
> Reported-by: Carl Lei <me@xecycle.info>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>
> v2: Fix missing configure_unmap handling so failure is returned.
>
>  drivers/target/target_core_configfs.c | 48 ++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index a5f58988130a..c1fbcdd16182 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -759,6 +759,29 @@ static ssize_t emulate_tas_store(struct config_item *item,
>         return count;
>  }
>
> +static int target_try_configure_unmap(struct se_device *dev,
> +                                     const char *config_opt)
> +{
> +       if (!dev->transport->configure_unmap) {
> +               pr_err("Generic Block Discard not supported\n");
> +               return -ENOSYS;
> +       }
> +
> +       if (!target_dev_configured(dev)) {
> +               pr_err("Generic Block Discard setup for %s requires device to be configured\n",
> +                      config_opt);
> +               return -ENODEV;
> +       }
> +
> +       if (!dev->transport->configure_unmap(dev)) {
> +               pr_err("Generic Block Discard setup for %s failed\n",
> +                      config_opt);
> +               return -ENOSYS;
> +       }
> +
> +       return 0;
> +}
> +
>  static ssize_t emulate_tpu_store(struct config_item *item,
>                 const char *page, size_t count)
>  {
> @@ -776,11 +799,9 @@ static ssize_t emulate_tpu_store(struct config_item *item,
>          * Discard supported is detected iblock_create_virtdevice().
>          */
>         if (flag && !da->max_unmap_block_desc_count) {
> -               if (!dev->transport->configure_unmap ||
> -                   !dev->transport->configure_unmap(dev)) {
> -                       pr_err("Generic Block Discard not supported\n");
> -                       return -ENOSYS;
> -               }
> +               ret = target_try_configure_unmap(dev, "emulate_tpu");
> +               if (ret)
> +                       return ret;
>         }
>
>         da->emulate_tpu = flag;
> @@ -806,11 +827,9 @@ static ssize_t emulate_tpws_store(struct config_item *item,
>          * Discard supported is detected iblock_create_virtdevice().
>          */
>         if (flag && !da->max_unmap_block_desc_count) {
> -               if (!dev->transport->configure_unmap ||
> -                   !dev->transport->configure_unmap(dev)) {
> -                       pr_err("Generic Block Discard not supported\n");
> -                       return -ENOSYS;
> -               }
> +               ret = target_try_configure_unmap(dev, "emulate_tpws");
> +               if (ret)
> +                       return ret;
>         }
>
>         da->emulate_tpws = flag;
> @@ -1022,12 +1041,9 @@ static ssize_t unmap_zeroes_data_store(struct config_item *item,
>          * Discard supported is detected iblock_configure_device().
>          */
>         if (flag && !da->max_unmap_block_desc_count) {
> -               if (!dev->transport->configure_unmap ||
> -                   !dev->transport->configure_unmap(dev)) {
> -                       pr_err("dev[%p]: Thin Provisioning LBPRZ will not be set because max_unmap_block_desc_count is zero\n",
> -                              da->da_dev);
> -                       return -ENOSYS;
> -               }
> +               ret = target_try_configure_unmap(dev, "unmap_zeroes_data");
> +               if (ret)
> +                       return ret;
>         }
>         da->unmap_zeroes_data = flag;
>         pr_debug("dev[%p]: SE Device Thin Provisioning LBPRZ bit: %d\n",
> --
> 2.34.1
>

Reviewed-by: Maurizio Lombardi <mlombard@redhat.com>
Martin K. Petersen Feb. 13, 2024, 1:57 a.m. UTC | #2 
On Fri, 09 Feb 2024 15:52:47 -0600, Mike Christie wrote:

> This issue was found and also debugged by Carl Lei <me@xecycle.info>.
> 
> If the device is not enabled, iblock/file will have not setup their
> se_device to bdev/file mappings. If a user tries to config the unmap
> settings at this time, we will then crash trying to access a NULL
> pointer where the bdev/file should be.
> 
> [...]

Applied to 6.8/scsi-fixes, thanks!

[1/1] scsi: target: Fix unmap setup during configuration
      https://git.kernel.org/mkp/scsi/c/4cbec7e89a41
diff mbox series

Patch

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index a5f58988130a..c1fbcdd16182 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -759,6 +759,29 @@  static ssize_t emulate_tas_store(struct config_item *item,
 	return count;
 }
 
+static int target_try_configure_unmap(struct se_device *dev,
+				      const char *config_opt)
+{
+	if (!dev->transport->configure_unmap) {
+		pr_err("Generic Block Discard not supported\n");
+		return -ENOSYS;
+	}
+
+	if (!target_dev_configured(dev)) {
+		pr_err("Generic Block Discard setup for %s requires device to be configured\n",
+		       config_opt);
+		return -ENODEV;
+	}
+
+	if (!dev->transport->configure_unmap(dev)) {
+		pr_err("Generic Block Discard setup for %s failed\n",
+		       config_opt);
+		return -ENOSYS;
+	}
+
+	return 0;
+}
+
 static ssize_t emulate_tpu_store(struct config_item *item,
 		const char *page, size_t count)
 {
@@ -776,11 +799,9 @@  static ssize_t emulate_tpu_store(struct config_item *item,
 	 * Discard supported is detected iblock_create_virtdevice().
 	 */
 	if (flag && !da->max_unmap_block_desc_count) {
-		if (!dev->transport->configure_unmap ||
-		    !dev->transport->configure_unmap(dev)) {
-			pr_err("Generic Block Discard not supported\n");
-			return -ENOSYS;
-		}
+		ret = target_try_configure_unmap(dev, "emulate_tpu");
+		if (ret)
+			return ret;
 	}
 
 	da->emulate_tpu = flag;
@@ -806,11 +827,9 @@  static ssize_t emulate_tpws_store(struct config_item *item,
 	 * Discard supported is detected iblock_create_virtdevice().
 	 */
 	if (flag && !da->max_unmap_block_desc_count) {
-		if (!dev->transport->configure_unmap ||
-		    !dev->transport->configure_unmap(dev)) {
-			pr_err("Generic Block Discard not supported\n");
-			return -ENOSYS;
-		}
+		ret = target_try_configure_unmap(dev, "emulate_tpws");
+		if (ret)
+			return ret;
 	}
 
 	da->emulate_tpws = flag;
@@ -1022,12 +1041,9 @@  static ssize_t unmap_zeroes_data_store(struct config_item *item,
 	 * Discard supported is detected iblock_configure_device().
 	 */
 	if (flag && !da->max_unmap_block_desc_count) {
-		if (!dev->transport->configure_unmap ||
-		    !dev->transport->configure_unmap(dev)) {
-			pr_err("dev[%p]: Thin Provisioning LBPRZ will not be set because max_unmap_block_desc_count is zero\n",
-			       da->da_dev);
-			return -ENOSYS;
-		}
+		ret = target_try_configure_unmap(dev, "unmap_zeroes_data");
+		if (ret)
+			return ret;
 	}
 	da->unmap_zeroes_data = flag;
 	pr_debug("dev[%p]: SE Device Thin Provisioning LBPRZ bit: %d\n",