Re: [PATCH] scsi:target:tcmu: make sure dev blocked before resetting ring

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 3/16/22 17:01, Guixin Liu wrote:
> If dev is not blocked when resetting ring, then there could be new
> commands coming in after resetting ring, this will make cmd ring broken,
> because tcmu can not find tcmu_cmd when tcmu-runner handled these
> newcome commands.
> 
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>  drivers/target/target_core_user.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 7b2a89a..548ad94 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -2333,7 +2333,7 @@ static void tcmu_block_dev(struct tcmu_dev *udev)
>  	mutex_unlock(&udev->cmdr_lock);
>  }
>  
> -static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level)
> +static int tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level)
>  {
>  	struct tcmu_mailbox *mb;
>  	struct tcmu_cmd *cmd;
> @@ -2341,6 +2341,12 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level)
>  
>  	mutex_lock(&udev->cmdr_lock);
>  
> +	if (!test_bit(TCMU_DEV_BIT_BLOCKED, &udev->flags)) {
> +		pr_err("The dev should be blocked before resetting ring.\n");

This looks like a bug... So I think this should at least be a WARN_ON(). E.g.

	if (WARN_ON(!test_bit(TCMU_DEV_BIT_BLOCKED, &udev->flags))) {
		mutex_unlock(&udev->cmdr_lock);
		return -EINVAL;
	}

But why is the ring reset even allowed without the device being blocked in
the first place ? What is the root cause ?

> +		mutex_unlock(&udev->cmdr_lock);
> +		return -EINVAL;
> +	}
> +
>  	xa_for_each(&udev->commands, i, cmd) {
>  		pr_debug("removing cmd %u on dev %s from ring %s\n",
>  			 cmd->cmd_id, udev->name,
> @@ -2396,6 +2402,7 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level)
>  	run_qfull_queue(udev, false);
>  
>  	mutex_unlock(&udev->cmdr_lock);
> +	return 0;
>  }
>  
>  enum {
> @@ -2995,7 +3002,10 @@ static ssize_t tcmu_reset_ring_store(struct config_item *item, const char *page,
>  		return -EINVAL;
>  	}
>  
> -	tcmu_reset_ring(udev, val);
> +	ret = tcmu_reset_ring(udev, val);
> +	if (ret < 0)
> +		return ret;
> +
>  	return count;
>  }
>  CONFIGFS_ATTR_WO(tcmu_, reset_ring);


-- 
Damien Le Moal
Western Digital Research