[PATCHv5 0/2] tcmu: For bugs fix only

[PATCHv5 0/2] tcmu: For bugs fix only

[lixiubo]

From: Xiubo Li <lixiubo@cmss.chinamobile.com>

Changed for V5:
- This only includes #1 and #2. And for old #3, #4 are still reviewing.
- #1, since the issue reported by Ilias is a separate new one, and will
  create a new patch later.
- #2, address the issue pointed out by Mike, thanks.
- #1 and #2 have been tested by Ilias in BIDI case, thanks.

Changed for V4:
- re-order the #3, #4 at the head.
- merge most of the #5 to others.

Xiubo Li (2):
  tcmu: Fix possible overwrite of t_data_sg's last iov[]
  tcmu: Fix wrongly calculating of the base_command_size

 drivers/target/target_core_user.c | 44 +++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 13 deletions(-)

-- 
1.8.3.1

[PATCHv5 1/2] tcmu: Fix possible overwrite of t_data_sg's last iov[]

[lixiubo]

From: Xiubo Li <lixiubo@cmss.chinamobile.com>

If there has BIDI data, its first iov[] will overwrite the last
iov[] for se_cmd->t_data_sg.

To fix this, we can just increase the iov pointer, but this may
introuduce a new memory leakage bug: If the se_cmd->data_length
and se_cmd->t_bidi_data_sg->length are all not aligned up to the
DATA_BLOCK_SIZE, the actual length needed maybe larger than just
sum of them.

So, this could be avoided by rounding all the data lengthes up
to DATA_BLOCK_SIZE.

Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
Reviewed-by: Mike Christie <mchristi@redhat.com>
Tested-by: Ilias Tsitsimpis <iliastsi@arrikto.com>
---
 drivers/target/target_core_user.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 57defc3..65d475f 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -394,6 +394,20 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 	return true;
 }
 
+static inline size_t tcmu_cmd_get_data_length(struct tcmu_cmd *tcmu_cmd)
+{
+	struct se_cmd *se_cmd = tcmu_cmd->se_cmd;
+	size_t data_length = round_up(se_cmd->data_length, DATA_BLOCK_SIZE);
+
+	if (se_cmd->se_cmd_flags & SCF_BIDI) {
+		BUG_ON(!(se_cmd->t_bidi_data_sg && se_cmd->t_bidi_data_nents));
+		data_length += round_up(se_cmd->t_bidi_data_sg->length,
+				DATA_BLOCK_SIZE);
+	}
+
+	return data_length;
+}
+
 static sense_reason_t
 tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
 {
@@ -407,7 +421,7 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 	uint32_t cmd_head;
 	uint64_t cdb_off;
 	bool copy_to_data_area;
-	size_t data_length;
+	size_t data_length = tcmu_cmd_get_data_length(tcmu_cmd);
 	DECLARE_BITMAP(old_bitmap, DATA_BLOCK_BITS);
 
 	if (test_bit(TCMU_DEV_BIT_BROKEN, &udev->flags))
@@ -433,11 +447,6 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 
 	mb = udev->mb_addr;
 	cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
-	data_length = se_cmd->data_length;
-	if (se_cmd->se_cmd_flags & SCF_BIDI) {
-		BUG_ON(!(se_cmd->t_bidi_data_sg && se_cmd->t_bidi_data_nents));
-		data_length += se_cmd->t_bidi_data_sg->length;
-	}
 	if ((command_size > (udev->cmdr_size / 2)) ||
 	    data_length > udev->data_size) {
 		pr_warn("TCMU: Request of size %zu/%zu is too big for %u/%zu "
@@ -511,11 +520,14 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
 	entry->req.iov_dif_cnt = 0;
 
 	/* Handle BIDI commands */
-	iov_cnt = 0;
-	alloc_and_scatter_data_area(udev, se_cmd->t_bidi_data_sg,
-		se_cmd->t_bidi_data_nents, &iov, &iov_cnt, false);
-	entry->req.iov_bidi_cnt = iov_cnt;
-
+	if (se_cmd->se_cmd_flags & SCF_BIDI) {
+		iov_cnt = 0;
+		iov++;
+		alloc_and_scatter_data_area(udev, se_cmd->t_bidi_data_sg,
+				se_cmd->t_bidi_data_nents, &iov, &iov_cnt,
+				false);
+		entry->req.iov_bidi_cnt = iov_cnt;
+	}
 	/* cmd's data_bitmap is what changed in process */
 	bitmap_xor(tcmu_cmd->data_bitmap, old_bitmap, udev->data_bitmap,
 			DATA_BLOCK_BITS);
-- 
1.8.3.1

[PATCHv5 2/2] tcmu: Fix wrongly calculating of the base_command_size

[lixiubo]

From: Xiubo Li <lixiubo@cmss.chinamobile.com>

The t_data_nents and t_bidi_data_nents are the numbers of the
segments, but it couldn't be sure the block size equals to size
of the segment.

For the worst case, all the blocks are discontiguous and there
will need the same number of iovecs, that's to say: blocks == iovs.
So here just set the number of iovs to block count needed by tcmu
cmd.

Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
Tested-by: Ilias Tsitsimpis <iliastsi@arrikto.com>
---
 drivers/target/target_core_user.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 65d475f..ede815c 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -408,6 +408,13 @@ static inline size_t tcmu_cmd_get_data_length(struct tcmu_cmd *tcmu_cmd)
 	return data_length;
 }
 
+static inline uint32_t tcmu_cmd_get_block_cnt(struct tcmu_cmd *tcmu_cmd)
+{
+	size_t data_length = tcmu_cmd_get_data_length(tcmu_cmd);
+
+	return data_length / DATA_BLOCK_SIZE;
+}
+
 static sense_reason_t
 tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
 {
@@ -435,8 +442,7 @@ static inline size_t tcmu_cmd_get_data_length(struct tcmu_cmd *tcmu_cmd)
 	 * expensive to tell how many regions are freed in the bitmap
 	*/
 	base_command_size = max(offsetof(struct tcmu_cmd_entry,
-				req.iov[se_cmd->t_bidi_data_nents +
-					se_cmd->t_data_nents]),
+				req.iov[tcmu_cmd_get_block_cnt(tcmu_cmd)]),
 				sizeof(struct tcmu_cmd_entry));
 	command_size = base_command_size
 		+ round_up(scsi_command_size(se_cmd->t_task_cdb), TCMU_OP_ALIGN_SIZE);
-- 
1.8.3.1

Re: [PATCHv5 2/2] tcmu: Fix wrongly calculating of the base_command_size

[Mike Christie]

On 03/27/2017 04:07 AM, lixiubo@cmss.chinamobile.com wrote:
> From: Xiubo Li <lixiubo@cmss.chinamobile.com>
> 
> The t_data_nents and t_bidi_data_nents are the numbers of the
> segments, but it couldn't be sure the block size equals to size
> of the segment.
> 
> For the worst case, all the blocks are discontiguous and there
> will need the same number of iovecs, that's to say: blocks == iovs.
> So here just set the number of iovs to block count needed by tcmu
> cmd.
> 
> Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
> Tested-by: Ilias Tsitsimpis <iliastsi@arrikto.com>
> ---
>  drivers/target/target_core_user.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 65d475f..ede815c 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -408,6 +408,13 @@ static inline size_t tcmu_cmd_get_data_length(struct tcmu_cmd *tcmu_cmd)
>  	return data_length;
>  }
>  
> +static inline uint32_t tcmu_cmd_get_block_cnt(struct tcmu_cmd *tcmu_cmd)
> +{
> +	size_t data_length = tcmu_cmd_get_data_length(tcmu_cmd);
> +
> +	return data_length / DATA_BLOCK_SIZE;
> +}
> +
>  static sense_reason_t
>  tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
>  {
> @@ -435,8 +442,7 @@ static inline size_t tcmu_cmd_get_data_length(struct tcmu_cmd *tcmu_cmd)
>  	 * expensive to tell how many regions are freed in the bitmap
>  	*/
>  	base_command_size = max(offsetof(struct tcmu_cmd_entry,
> -				req.iov[se_cmd->t_bidi_data_nents +
> -					se_cmd->t_data_nents]),
> +				req.iov[tcmu_cmd_get_block_cnt(tcmu_cmd)]),
>  				sizeof(struct tcmu_cmd_entry));
>  	command_size = base_command_size
>  		+ round_up(scsi_command_size(se_cmd->t_task_cdb), TCMU_OP_ALIGN_SIZE);
> 

Looks ok to me. Thanks.
Reviewed-by: Mike Christie <mchristi@redhat.com>

Re: [PATCHv5 0/2] tcmu: For bugs fix only

[Nicholas A. Bellinger]

Hi Xiubo & Co,

On Mon, 2017-03-27 at 17:07 +0800, lixiubo@cmss.chinamobile.com wrote:
> From: Xiubo Li <lixiubo@cmss.chinamobile.com>
> 
> Changed for V5:
> - This only includes #1 and #2. And for old #3, #4 are still reviewing.
> - #1, since the issue reported by Ilias is a separate new one, and will
>   create a new patch later.
> - #2, address the issue pointed out by Mike, thanks.
> - #1 and #2 have been tested by Ilias in BIDI case, thanks.
> 
> Changed for V4:
> - re-order the #3, #4 at the head.
> - merge most of the #5 to others.
> 
> Xiubo Li (2):
>   tcmu: Fix possible overwrite of t_data_sg's last iov[]
>   tcmu: Fix wrongly calculating of the base_command_size
> 
>  drivers/target/target_core_user.c | 44 +++++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 13 deletions(-)
> 

Applied to target-pending/master, along with CC's for 3.19.y stable.

Thanks Xiubo !

Btw, I fixed-up the ordering of the Reviewed-by + Tested-by +
Signed-off-by tags on the patches here:

https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git/commit/?id=ab22d2604c86ceb01bb2725c9860b88a7dd383bb
https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git/commit/?id=abe342a5b4b5aa579f6bf40ba73447c699e6b579

Just for future reference, the flow of these tags should reflect the
history of the patch.  Eg:

Reviewed-by: First reviewer <foo@bar.com>
Tested-by: First tester <foo2@bar2.com>
Reviewed-by: Second reviewer <foo3@bar3.com>
Signed-off-by: Patch Author <you@yourdomain.com>

and then once the subsystem maintainer merges it into his tree, they add
their own:

Signed-off-by: Subsystem Maintainer <superturboarray@linux-domain.org>

Beyond that, what is the status of the other two feature improvement
patches..?

Do you intend to post those (again) as v4.12-rc1 material..?

Re: [PATCHv5 0/2] tcmu: For bugs fix only

[Xiubo Li]

On 2017年03月30日 16:48, Nicholas A. Bellinger wrote:
> Hi Xiubo & Co,
>
> On Mon, 2017-03-27 at 17:07 +0800, lixiubo@cmss.chinamobile.com wrote:
>> From: Xiubo Li <lixiubo@cmss.chinamobile.com>
>>
>> Changed for V5:
>> - This only includes #1 and #2. And for old #3, #4 are still reviewing.
>> - #1, since the issue reported by Ilias is a separate new one, and will
>>    create a new patch later.
>> - #2, address the issue pointed out by Mike, thanks.
>> - #1 and #2 have been tested by Ilias in BIDI case, thanks.
>>
>> Changed for V4:
>> - re-order the #3, #4 at the head.
>> - merge most of the #5 to others.
>>
>> Xiubo Li (2):
>>    tcmu: Fix possible overwrite of t_data_sg's last iov[]
>>    tcmu: Fix wrongly calculating of the base_command_size
>>
>>   drivers/target/target_core_user.c | 44 +++++++++++++++++++++++++++------------
>>   1 file changed, 31 insertions(+), 13 deletions(-)
>>
> Applied to target-pending/master, along with CC's for 3.19.y stable.
>
> Thanks Xiubo !
>
> Btw, I fixed-up the ordering of the Reviewed-by + Tested-by +
> Signed-off-by tags on the patches here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git/commit/?id=ab22d2604c86ceb01bb2725c9860b88a7dd383bb
> https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git/commit/?id=abe342a5b4b5aa579f6bf40ba73447c699e6b579
>
> Just for future reference, the flow of these tags should reflect the
> history of the patch.  Eg:
>
> Reviewed-by: First reviewer <foo@bar.com>
> Tested-by: First tester <foo2@bar2.com>
> Reviewed-by: Second reviewer <foo3@bar3.com>
> Signed-off-by: Patch Author <you@yourdomain.com>
>
> and then once the subsystem maintainer merges it into his tree, they add
> their own:
>
> Signed-off-by: Subsystem Maintainer <superturboarray@linux-domain.org>

Sure, thanks very much.

> Beyond that, what is the status of the other two feature improvement
> patches..?
>
> Do you intend to post those (again) as v4.12-rc1 material..?
>
Since the #2 has changed in V5, and the old #3 will have a conflict with 
it.

The #3 and #4 are still in reviewing process, so I will post it again if 
they are all okay later.

Thanks,

BRs
Xiubo

Re: [PATCHv5 0/2] tcmu: For bugs fix only

[Bart Van Assche]

On Thu, 2017-03-30 at 01:48 -0700, Nicholas A. Bellinger wrote:
> Just for future reference, the flow of these tags should reflect the
> history of the patch.  Eg:
> 
> Reviewed-by: First reviewer <foo@bar.com>
> Tested-by: First tester <foo2@bar2.com>
> Reviewed-by: Second reviewer <foo3@bar3.com>
> Signed-off-by: Patch Author <you@yourdomain.com>
> 
> and then once the subsystem maintainer merges it into his tree, they add
> their own:
> 
> Signed-off-by: Subsystem Maintainer <superturboarray@linux-domain.org>

Hi Nic,

I agree that these tags should reflect the history of the patch. I think
that means that the patch author should be mentioned first, Reviewed-by /
Tested-by tags next and the subsystem maintainer sign-off last. At least,
that's how most other maintainers do it.

Bart.