On 05/23/2017 06:48 PM, Bart Van Assche wrote:
> diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
> index 56738a41e346..27414ab01b24 100644
> --- a/drivers/target/target_core_xcopy.c
> +++ b/drivers/target/target_core_xcopy.c
> @@ -62,6 +62,8 @@ static int target_xcopy_locate_se_dev_e4(const unsigned char *dev_wwn,
> unsigned char tmp_dev_wwn[XCOPY_NAA_IEEE_REGEX_LEN];
> int rc;
>
> + *found_dev = NULL;
> +
> mutex_lock(&g_device_mutex);
> list_for_each_entry(se_dev, &g_device_list, g_dev_node) {
>
> @@ -71,32 +73,33 @@ static int target_xcopy_locate_se_dev_e4(const unsigned char *dev_wwn,
> memset(&tmp_dev_wwn[0], 0, XCOPY_NAA_IEEE_REGEX_LEN);
> target_xcopy_gen_naa_ieee(se_dev, &tmp_dev_wwn[0]);
>
> - rc = memcmp(&tmp_dev_wwn[0], dev_wwn, XCOPY_NAA_IEEE_REGEX_LEN);
> - if (rc != 0)
> + if (memcmp(&tmp_dev_wwn[0], dev_wwn, XCOPY_NAA_IEEE_REGEX_LEN))
> continue;
>
> - *found_dev = se_dev;
> - pr_debug("XCOPY 0xe4: located se_dev: %p\n", se_dev);
> + if (target_get_device(se_dev))
Hey Bart,
I think there is one small problem. Maybe not necessarily with this
patch but the change in it exposes it.
If after we do a successful target_get_device a user app deletes the
backend device (does rmdir
/sys/kernel/config/target/core/fileio_1/somedevice), rmdir will will
return success immediately since we have a done a get above. The app
will then do rmdir on (/sys/kernel/config/target/core/fileio_1).
The problem is that if the second rmdir on fileio_1 runs before
target_put_device below is done, then core_delete_hba will hit the
WARN_ON(hba->dev_count). Note that in this test target_depend_item will
fail because the first rmdir on "somedevice" has already succeeded.
This does not happen without your patch, because the first rmdir on
/sys/kernel/config/target/core/fileio_1/somedevice would drop the
refcount to zero and the removing thread would either run
target_free_device before target_xcopy_locate_se_dev_e4 can grab the
g_device_mutex. Or, if target_xcopy_locate_se_dev_e4 grabs the
g_device_mutex firs, then target_depend_item will fail since because we
already deleting the device.
> + *found_dev = se_dev;
> + break;
> + }
> + mutex_unlock(&g_device_mutex);
>
> - rc = target_depend_item(&se_dev->dev_group.cg_item);
> - if (rc != 0) {
> - pr_err("configfs_depend_item attempt failed:"
> - " %d for se_dev: %p\n", rc, se_dev);
> - mutex_unlock(&g_device_mutex);
> - return rc;
> - }
> + if (*found_dev == NULL) {
> + pr_debug_ratelimited("Unable to locate 0xe4 descriptor for EXTENDED_COPY\n");
> + return -EINVAL;
> + }
>
> - pr_debug("Called configfs_depend_item for se_dev: %p"
> - " se_dev->se_dev_group: %p\n", se_dev,
> - &se_dev->dev_group);
> + pr_debug("XCOPY 0xe4: located se_dev: %p\n", se_dev);
>
> - mutex_unlock(&g_device_mutex);
> - return 0;
> - }
> - mutex_unlock(&g_device_mutex);
> + rc = target_depend_item(&se_dev->dev_group.cg_item);
> + if (rc != 0)
> + pr_err("configfs_depend_item attempt failed: %d for se_dev: %p\n",
> + rc, se_dev);
> + else
> + pr_debug("Called configfs_depend_item for se_dev: %p se_dev->se_dev_group: %p\n",
> + se_dev, &se_dev->dev_group);
>
> - pr_debug_ratelimited("Unable to locate 0xe4 descriptor for EXTENDED_COPY\n");
> - return -EINVAL;
> + target_put_device(se_dev);
> +
> + return rc;
> }
>
> static int target_xcopy_parse_tiddesc_e4(struct se_cmd *se_cmd, struct xcopy_op *xop,
>
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
