[PATCH 2/4] target/user: Return an error if cmd data size is too large
Seth Forshee
seth.forshee at canonical.com
Wed Dec 7 14:57:19 UTC 2016
On Tue, Dec 06, 2016 at 12:40:53PM -0700, Tim Gardner wrote:
> From: Andy Grover <agrover at redhat.com>
>
> BugLink: http://bugs.launchpad.net/bugs/1646204
>
> Userspace should be implementing VPD B0 (Block Limits) to inform the
> initiator of max data size, but just in case we do get a too-large request,
> do what the spec says and return INVALID_CDB_FIELD.
>
> Make sure to unlock udev->cmdr_lock before returning.
>
> Signed-off-by: Andy Grover <agrover at redhat.com>
> Reviewed-by: Christoph Hellwig <hch at lst.de>
> Reviewed-by: Mike Christie <mchristi at redhat.com>
> Signed-off-by: Nicholas Bellinger <nab at linux-iscsi.org>
> (back ported from commit 554617b2bbe25c3fb3c80c28fe7a465884bb40b1)
> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
>
> Conflicts:
> drivers/target/target_core_user.c
> ---
> drivers/target/target_core_user.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 28debdf..c252528 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -393,6 +393,7 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
> uint32_t cmd_head;
> uint64_t cdb_off;
> bool copy_to_data_area;
> + size_t data_length;
>
> if (test_bit(TCMU_DEV_BIT_BROKEN, &udev->flags))
> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> @@ -417,11 +418,19 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
>
> mb = udev->mb_addr;
> cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
> - if ((command_size > (udev->cmdr_size / 2))
> - || tcmu_cmd->data_length > (udev->data_size - 1))
> - pr_warn("TCMU: Request of size %zu/%zu may be too big for %u/%zu "
> - "cmd/data ring buffers\n", command_size, tcmu_cmd->data_length,
> + 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;
> + }
This really backports more changes than are necessary. This part
duplicates code in tcmu_alloc_cmd(), which was moved here by
26418649eead "target/user: Introduce data_bitmap, replace
data_length/data_head/data_tail" because the data_length member of
tcmu_cmd was removed.
> + 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 "
> + "cmd/data ring buffers\n", command_size, data_length,
> udev->cmdr_size, udev->data_size);
> + spin_unlock_irq(&udev->cmdr_lock);
> + return TCM_INVALID_CDB_FIELD;
> + }
And so this part should be able to continue using tcmu_cmd->data_length
as before. All that's really needed is to change this chunk to look like
this:
if ((command_size > (udev->cmdr_size / 2)) ||
tcmu_cmd->data_length > (udev->data_size - 1)) {
pr_warn("TCMU: Request of size %zu/%zu may be too big for %u/%zu "
"cmd/data ring buffers\n", command_size, tcmu_cmd->data_length,
udev->cmdr_size, udev->data_size);
spin_unlock_irq(&udev->cmdr_lock);
return TCM_INVALID_CDB_FIELD;
}
(I'm not sure about the (udev->data_size - 1) -> udev->data_size part,
was also changed in 26418649eead without explanation so maybe an
off-by-one error.)
I don't think your backport actually breaks anything though, it just
backports more than is necessary.
More information about the kernel-team
mailing list