From: Mike Christie <mchristi@redhat.com>
To: lixiubo@cmss.chinamobile.com, agrover@redhat.com, nab@linux-iscsi.org
Cc: shli@kernel.org, sheng@yasker.org, linux-scsi@vger.kernel.org,
target-devel@vger.kernel.org, namei.unix@gmail.com,
bryantly@linux.vnet.ibm.com
Subject: Re: [PATCHv3 3/5] target/user: Fix possible overwrite of t_data_sg's last iov[]
Date: Tue, 21 Mar 2017 15:10:00 -0400 [thread overview]
Message-ID: <77af2a99-d51c-7212-acf1-cf61b50cfdc5@redhat.com> (raw)
In-Reply-To: <1490000964-15732-4-git-send-email-lixiubo@cmss.chinamobile.com>
On 03/20/2017 05:09 AM, lixiubo@cmss.chinamobile.com wrote:
> 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>
> ---
> drivers/target/target_core_user.c | 32 +++++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 8cbe196..780c30f 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -288,13 +288,14 @@ static inline void tcmu_free_cmd(struct tcmu_cmd *tcmu_cmd)
>
> static inline uint32_t tcmu_cmd_get_dbi_len(struct se_cmd *se_cmd)
> {
> - size_t data_length = se_cmd->data_length;
> + size_t data_length = round_up(se_cmd->data_length, DATA_BLOCK_SIZE);
> uint32_t dbi_len;
>
> if (se_cmd->se_cmd_flags & SCF_BIDI)
> - data_length += se_cmd->t_bidi_data_sg->length;
> + data_length += round_up(se_cmd->t_bidi_data_sg->length,
> + DATA_BLOCK_SIZE);
>
> - dbi_len = (data_length + DATA_BLOCK_SIZE - 1) / DATA_BLOCK_SIZE;
> + dbi_len = data_length / DATA_BLOCK_SIZE;
>
> return dbi_len;
> }
> @@ -609,10 +610,11 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
>
> mb = udev->mb_addr;
> cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
> - data_length = se_cmd->data_length;
> + 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 += se_cmd->t_bidi_data_sg->length;
> + data_length += round_up(se_cmd->t_bidi_data_sg->length,
> + DATA_BLOCK_SIZE);
> }
> if ((command_size > (udev->cmdr_size / 2)) ||
> data_length > udev->data_size) {
> @@ -690,15 +692,19 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
> entry->req.iov_dif_cnt = 0;
>
> /* Handle BIDI commands */
> - iov_cnt = 0;
> - ret = alloc_and_scatter_data_area(udev, tcmu_cmd,
> - se_cmd->t_bidi_data_sg, se_cmd->t_bidi_data_nents,
> - &iov, &iov_cnt, false);
> - if (ret) {
> - pr_err("tcmu: alloc and scatter bidi data failed\n");
> - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> + if (se_cmd->se_cmd_flags & SCF_BIDI) {
> + iov_cnt = 0;
> + iov++;
> + ret = alloc_and_scatter_data_area(udev, tcmu_cmd,
> + se_cmd->t_bidi_data_sg,
> + se_cmd->t_bidi_data_nents,
> + &iov, &iov_cnt, false);
> + if (ret) {
> + pr_err("tcmu: alloc and scatter bidi data failed\n");
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> + }
> + entry->req.iov_bidi_cnt = iov_cnt;
> }
> - entry->req.iov_bidi_cnt = iov_cnt;
>
> /* All offsets relative to mb_addr, not start of entry! */
> cdb_off = CMDR_OFF + cmd_head + base_command_size;
>
Looks ok to me. Thanks.
Reviewed-by: Mike Christie <mchristi@redhat.com>
next prev parent reply other threads:[~2017-03-21 19:10 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-20 9:09 [PATCHv3 0/5] Dynamic growing data area support lixiubo
2017-03-20 9:09 ` [PATCHv3 1/5] tcmu: Add dynamic growing data area feature support lixiubo
2017-03-20 9:09 ` [PATCHv3 2/5] tcmu: Add global data block pool support lixiubo
2017-03-20 9:09 ` [PATCHv3 3/5] target/user: Fix possible overwrite of t_data_sg's last iov[] lixiubo
2017-03-21 19:10 ` Mike Christie [this message]
2017-03-20 9:09 ` [PATCHv3 4/5] target/user: Fix wrongly calculating of the base_command_size lixiubo
2017-03-20 9:09 ` [PATCHv3 5/5] target/user: Clean up tcmu_queue_cmd_ring lixiubo
2017-03-21 5:24 ` [PATCHv3 0/5] Dynamic growing data area support Nicholas A. Bellinger
2017-03-21 5:34 ` Xiubo Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=77af2a99-d51c-7212-acf1-cf61b50cfdc5@redhat.com \
--to=mchristi@redhat.com \
--cc=agrover@redhat.com \
--cc=bryantly@linux.vnet.ibm.com \
--cc=linux-scsi@vger.kernel.org \
--cc=lixiubo@cmss.chinamobile.com \
--cc=nab@linux-iscsi.org \
--cc=namei.unix@gmail.com \
--cc=sheng@yasker.org \
--cc=shli@kernel.org \
--cc=target-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.