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

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

[Guixin Liu]

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");
+		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);
-- 
1.8.3.1

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

[Damien Le Moal]

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

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

[Bodo Stroesser]

Hi,

I don't think we need any fix here.

1) Generally userspace can easily violate the ring protocol.
E.g., if userspace uses a currently not existing cmd_id in a cmd
response, it forces tcmu into RING_BROKEN state.
Resetting the ring while processing cmds is just another way to
probably force tcmu into RING_BROKEN.
Why should we try to fix just this one single situation?
If userspace resets the ring, and after that continues to write
responses to the ring for cmds it has read before the reset, we
probably have to fix userspace app.

2) Resetting the ring without being blocked is _not_ forbidden.
The situation you describe is more about resetting the ring while
userspace holds the uio dev open. But even that is not useless, if
userspace just stops to process cmds, resets the ring and then
re-starts processing the ring, including to read the new head and
tail pointers.

Bodo


On 16.03.22 09: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");
> +		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);