Commit f30f0c76 authored by Pawel Wodkowski's avatar Pawel Wodkowski Committed by Daniel Verkamp
Browse files

scsi: refactor usage of iov from spdk_scsi_task 



This patch is preparation for fixing alloc_len overrun in SENSE 6/10 and
READCAP 6/10. To simplify code forbid usage of iov outside of
scsi/task.c.

This also drop SPDK_SCSI_TASK_ALLOC_BUFFER flag that obfuscate code. As
a replacement assume that if field alloc_len is non zero it mean that
iov.buffer is internally allocated. Functions
spdk_scsi_task_free_data(), spdk_scsi_task_set_data() and
spdk_scsi_task_alloc_data() manage this field.

Change-Id: Ife357a5bc36121f93a4c5d259b9a5a01559e7708
Signed-off-by: default avatarPawel Wodkowski <pawelx.wodkowski@intel.com>
parent 51c6917f

include/spdk/scsi.h

+33 −8
Original line number Diff line number Diff line
@@ -63,10 +63,6 @@ 


#define SPDK_SCSI_LUN_MAX_NAME_LENGTH		16



/* This flag indicated that data buffers are allocated

 * internally and hence need to be freed by the SCSI layer.*/

#define SPDK_SCSI_TASK_ALLOC_BUFFER  1



enum spdk_scsi_data_dir {

	SPDK_SCSI_DIR_NONE = 0,

	SPDK_SCSI_DIR_TO_DEV = 1,
@@ -125,7 +121,6 @@ struct spdk_scsi_task { 
	 *  transfer_len - i.e. SCSI INQUIRY.

	 */

	uint32_t data_transferred;

	uint32_t alloc_len;



	uint64_t offset;

	struct spdk_scsi_task *parent;
@@ -134,10 +129,18 @@ struct spdk_scsi_task { 


	uint8_t *cdb;



	/**

	 * \internal

	 * Size of internal buffer or zero when iov.iov_base is not internally managed.

	 */

	uint32_t alloc_len;

	/**

	 * \internal

	 * iov is internal buffer. Use iovs to access elements of IO.

	 */

	struct iovec iov;

	struct iovec *iovs;

	uint16_t iovcnt;

	uint8_t iov_flags;



	uint8_t sense_data[32];

	size_t sense_data_len;
@@ -256,8 +259,30 @@ int spdk_scsi_port_construct(struct spdk_scsi_port *port, uint64_t id, 
void spdk_scsi_task_construct(struct spdk_scsi_task *task, uint32_t *owner_task_ctr,

			      struct spdk_scsi_task *parent);

void spdk_scsi_task_put(struct spdk_scsi_task *task);

void spdk_scsi_task_alloc_data(struct spdk_scsi_task *task, uint32_t alloc_len,

			       uint8_t **data);



void spdk_scsi_task_free_data(struct spdk_scsi_task *task);

/**

 * Set internal buffer to given one. Caller is owner of that buffer.

 *

 * \param task Task struct

 * \param data Pointer to buffer

 * \param len Buffer length

 */

void spdk_scsi_task_set_data(struct spdk_scsi_task *task, void *data, uint32_t len);



/**

 * Allocate internal buffer of requested size. Caller is not owner of

 * returned buffer and must not free it. Caller is permitted to call

 * spdk_scsi_task_free_data() to free internal buffer if it is not required

 * anymore, but must assert that task is done and not used by library.

 * The count of io vectors must by one. Any previously allocated buffer will be

 * invalid after this call.

 *

 * \param task Task struct

 * \param alloc_len Size of allocated buffer.

 * \return Pointer to buffer or NULL on error.

 */

void *spdk_scsi_task_alloc_data(struct spdk_scsi_task *task, uint32_t alloc_len);

void spdk_scsi_task_build_sense_data(struct spdk_scsi_task *task, int sk, int asc,

				     int ascq);

void spdk_scsi_task_set_status(struct spdk_scsi_task *task, int sc, int sk, int asc,

lib/iscsi/iscsi.c

+6 −9
Original line number Diff line number Diff line
@@ -2602,7 +2602,7 @@ spdk_iscsi_send_datain(struct spdk_iscsi_conn *conn, 
	/* DATA PDU */

	rsp_pdu = spdk_get_pdu();

	rsph = (struct iscsi_bhs_data_in *)&rsp_pdu->bhs;

	rsp_pdu->data = task->scsi.iov.iov_base + offset;

	rsp_pdu->data = task->scsi.iovs[0].iov_base + offset;

	rsp_pdu->data_from_mempool = true;



	task_tag = task->scsi.id;
@@ -2844,7 +2844,7 @@ int spdk_iscsi_conn_handle_queued_tasks(struct spdk_iscsi_conn *conn) 
			assert(subtask != NULL);

			subtask->scsi.offset = task->current_datain_offset;

			subtask->scsi.length = DMIN32(SPDK_BDEV_LARGE_RBUF_MAX_SIZE, remaining_size);

			subtask->scsi.iov.iov_base = NULL;

			spdk_scsi_task_set_data(&subtask->scsi, NULL, 0);

			spdk_iscsi_queue_task(conn, subtask);

			task->current_datain_offset += subtask->scsi.length;

			conn->data_in_cnt++;
@@ -2866,7 +2866,7 @@ static int spdk_iscsi_op_scsi_read(struct spdk_iscsi_conn *conn, 
	task->scsi.parent = NULL;

	task->scsi.offset = 0;

	task->scsi.length = DMIN32(SPDK_BDEV_LARGE_RBUF_MAX_SIZE, task->scsi.transfer_len);

	task->scsi.iov.iov_base = NULL;

	spdk_scsi_task_set_data(&task->scsi, NULL, 0);



	remaining_size = task->scsi.transfer_len - task->scsi.length;

	task->current_datain_offset = 0;
@@ -2985,16 +2985,14 @@ spdk_iscsi_op_scsi(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) 
			else {

				/* we are doing the first partial write task */

				task->scsi.ref++;

				task->scsi.iov.iov_base = pdu->data;

				task->scsi.iov.iov_len = pdu->data_segment_len;

				spdk_scsi_task_set_data(&task->scsi, pdu->data, pdu->data_segment_len);

				task->scsi.length = pdu->data_segment_len;

			}

		}



		if (pdu->data_segment_len == transfer_len) {

			/* we are doing small writes with no R2T */

			task->scsi.iov.iov_len = transfer_len;

			task->scsi.iov.iov_base = pdu->data;

			spdk_scsi_task_set_data(&task->scsi, pdu->data, transfer_len);

			task->scsi.length = transfer_len;

		}

	} else {
@@ -4083,8 +4081,7 @@ static int spdk_iscsi_op_data(struct spdk_iscsi_conn *conn, 
	}

	subtask->scsi.offset = buffer_offset;

	subtask->scsi.length = pdu->data_segment_len;

	subtask->scsi.iov.iov_base = pdu->data;

	subtask->scsi.iov.iov_len = pdu->data_segment_len;

	spdk_scsi_task_set_data(&subtask->scsi, pdu->data, pdu->data_segment_len);

	spdk_iscsi_task_associate_pdu(subtask, pdu);



	if (task->next_expected_r2t_offset == transfer_len) {

lib/scsi/lun.c

+1 −1
Original line number Diff line number Diff line
@@ -194,7 +194,7 @@ complete_task_with_no_lun(struct spdk_scsi_task *task) 
		 *  PERIPHERAL DEVICE TYPE = 0x1F.

		 */

		allocation_len = from_be16(&task->cdb[3]);

		spdk_scsi_task_alloc_data(task, allocation_len, &data);

		data = spdk_scsi_task_alloc_data(task, allocation_len);

		data_len = 36;

		memset(data, 0, data_len);

		/* PERIPHERAL QUALIFIER(7-5) PERIPHERAL DEVICE TYPE(4-0) */

lib/scsi/scsi_bdev.c

+11 −10
Original line number Diff line number Diff line
@@ -1317,8 +1317,9 @@ spdk_bdev_scsi_task_complete(spdk_event_t event) 
			spdk_scsi_lun_clear_all(task->lun);

		}

	}

	if (bdev_io->type == SPDK_BDEV_IO_TYPE_READ) {

		task->iov.iov_base = bdev_io->u.read.iovs[0].iov_base;

	if (bdev_io->type == SPDK_BDEV_IO_TYPE_READ && task->iovs != bdev_io->u.read.iovs) {

		assert(task->iovcnt == bdev_io->u.read.iovcnt);

		memcpy(task->iovs, bdev_io->u.read.iovs, sizeof(task->iovs[0]) * task->iovcnt);

	}



	spdk_scsi_lun_complete_task(task->lun, task);
@@ -1510,7 +1511,7 @@ spdk_bdev_scsi_unmap(struct spdk_bdev *bdev, 
	uint8_t *data;

	uint16_t bdesc_data_len, bdesc_count;



	data = (uint8_t *)task->iov.iov_base;

	data = (uint8_t *)task->iovs[0].iov_base;



	/*

	 * The UNMAP BLOCK DESCRIPTOR DATA LENGTH field specifies the length in
@@ -1591,7 +1592,7 @@ spdk_bdev_scsi_process_block(struct spdk_bdev *bdev, 
		return spdk_bdev_scsi_readwrite(bdev, task, lba, xfer_len);



	case SPDK_SBC_READ_CAPACITY_10:

		spdk_scsi_task_alloc_data(task, 8, &data);

		data = spdk_scsi_task_alloc_data(task, 8);

		if (bdev->blockcnt - 1 > 0xffffffffULL) {

			memset(data, 0xff, 4);

		} else {
@@ -1605,7 +1606,7 @@ spdk_bdev_scsi_process_block(struct spdk_bdev *bdev, 
	case SPDK_SPC_SERVICE_ACTION_IN_16:

		switch (cdb[1] & 0x1f) { /* SERVICE ACTION */

		case SPDK_SBC_SAI_READ_CAPACITY_16:

			spdk_scsi_task_alloc_data(task, 32, &data);

			data = spdk_scsi_task_alloc_data(task, 32);

			to_be64(&data[0], bdev->blockcnt - 1);

			to_be32(&data[8], bdev->blocklen);

			/*
@@ -1669,7 +1670,7 @@ spdk_bdev_scsi_process_primary(struct spdk_bdev *bdev, 
	switch (cdb[0]) {

	case SPDK_SPC_INQUIRY:

		alloc_len = from_be16(&cdb[3]);

		spdk_scsi_task_alloc_data(task, alloc_len, &data);

		data = spdk_scsi_task_alloc_data(task, alloc_len);

		data_len = spdk_bdev_scsi_inquiry(bdev, task, cdb,

						  data, alloc_len);

		if (data_len < 0) {
@@ -1697,7 +1698,7 @@ spdk_bdev_scsi_process_primary(struct spdk_bdev *bdev, 
			break;

		}



		spdk_scsi_task_alloc_data(task, alloc_len, &data);

		data = spdk_scsi_task_alloc_data(task, alloc_len);

		data_len = spdk_bdev_scsi_report_luns(task->lun, sel, data, task->alloc_len);

		if (data_len < 0) {

			spdk_scsi_task_set_status(task, SPDK_SCSI_STATUS_CHECK_CONDITION,
@@ -1715,7 +1716,7 @@ spdk_bdev_scsi_process_primary(struct spdk_bdev *bdev, 


	case SPDK_SPC_MODE_SELECT_6:

	case SPDK_SPC_MODE_SELECT_10:

		data = task->iov.iov_base;

		data = task->iovs[0].iov_base;



		if (cdb[0] == SPDK_SPC_MODE_SELECT_6) {

			md = 4;
@@ -1792,7 +1793,7 @@ spdk_bdev_scsi_process_primary(struct spdk_bdev *bdev, 
		page = cdb[2] & 0x3f;

		subpage = cdb[3];



		spdk_scsi_task_alloc_data(task, alloc_len, &data);

		data = spdk_scsi_task_alloc_data(task, alloc_len);



		if (md == 6) {

			data_len = spdk_bdev_scsi_mode_sense6(bdev,
@@ -1838,7 +1839,7 @@ spdk_bdev_scsi_process_primary(struct spdk_bdev *bdev, 
		}



		alloc_len = cdb[4];

		spdk_scsi_task_alloc_data(task, alloc_len, &data);

		data = spdk_scsi_task_alloc_data(task, alloc_len);



		/* NO ADDITIONAL SENSE INFORMATION */

		sk = SPDK_SCSI_SENSE_NO_SENSE;

lib/scsi/task.c

+40 −8
Original line number Diff line number Diff line
@@ -59,11 +59,11 @@ spdk_scsi_task_put(struct spdk_scsi_task *task) 
				bdev_io->status = SPDK_BDEV_IO_STATUS_FAILED;

			}

			spdk_bdev_free_io(bdev_io);

		} else if (task->iov_flags & SPDK_SCSI_TASK_ALLOC_BUFFER) {

			spdk_free(task->iov.iov_base);

		}



		spdk_scsi_task_free_data(task);

		assert(task->owner_task_ctr != NULL);



		if (*(task->owner_task_ctr) > 0) {

			*(task->owner_task_ctr) -= 1;

		} else {
@@ -87,6 +87,7 @@ spdk_scsi_task_construct(struct spdk_scsi_task *task, uint32_t *owner_task_ctr, 
	/*

	 * Pre-fill the iov_buffers to point to the embedded iov

	 */

	assert(task->iov.iov_base == NULL);

	task->iovs = &task->iov;

	task->iovcnt = 1;
@@ -105,8 +106,19 @@ spdk_scsi_task_construct(struct spdk_scsi_task *task, uint32_t *owner_task_ctr, 
}



void

spdk_scsi_task_alloc_data(struct spdk_scsi_task *task, uint32_t alloc_len,

			  uint8_t **data)

spdk_scsi_task_free_data(struct spdk_scsi_task *task)

{

	if (task->alloc_len != 0) {

		spdk_free(task->iov.iov_base);

		task->alloc_len = 0;

	}



	task->iov.iov_base = NULL;

	task->iov.iov_len = 0;

}



void *

spdk_scsi_task_alloc_data(struct spdk_scsi_task *task, uint32_t alloc_len)

{

	/*

	 * SPDK iSCSI target depends on allocating at least 4096 bytes, even if
@@ -117,17 +129,37 @@ spdk_scsi_task_alloc_data(struct spdk_scsi_task *task, uint32_t alloc_len, 
	 *  care of only sending back the amount of data specified in the

	 *  allocation length.

	 */

	assert(task->alloc_len == 0);



	/* Only one buffer is managable */

	if (task->iovcnt != 1) {

		return NULL;

	}



	if (alloc_len < 4096) {

		alloc_len = 4096;

	}



	/* This is workaround for buffers shorter than 4kb */

	if (task->iov.iov_base == NULL) {

		task->iov.iov_base = spdk_zmalloc(alloc_len, 0, NULL);

		task->iov_flags |= SPDK_SCSI_TASK_ALLOC_BUFFER;

	}

		task->alloc_len = alloc_len;

	*data = task->iov.iov_base;

	memset(task->iov.iov_base, 0, task->alloc_len);

	}



	task->iov.iov_len = alloc_len;

	assert(&task->iov == task->iovs);



	return task->iov.iov_base;

}



void

spdk_scsi_task_set_data(struct spdk_scsi_task *task, void *data, uint32_t len)

{

	assert(task->iovcnt == 1);

	assert(task->alloc_len == 0);



	task->iovs[0].iov_base = data;

	task->iovs[0].iov_len = len;

}



void