On 3/5/2014 11:38 PM, Nicholas A. Bellinger wrote:
On Wed, 2014-03-05 at 15:38 +0200, Sagi Grimberg wrote:
On 2/19/2014 5:50 PM, Sagi Grimberg wrote:
For transports which use generic new command these
buffers have yet to be allocated. Instead check afterwards
if command required prot buffers but none are provided, In
this case send CHECK_CONDITION response.
Also this way, target may support protection information
against legacy initiators (writes are inserted and reads
are stripped).
Signed-off-by: Sagi Grimberg <sagig@xxxxxxxxxxxx>
---
drivers/target/target_core_sbc.c | 3 ---
drivers/target/target_core_transport.c | 21 ++++++++++++++++++---
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 6939947..312f522 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -633,9 +633,6 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
{
u8 protect = cdb[1] >> 5;
- if (!cmd->t_prot_sg || !cmd->t_prot_nents)
- return true;
-
switch (dev->dev_attrib.pi_prot_type) {
case TARGET_DIF_TYPE3_PROT:
cmd->reftag_seed = 0xffffffff;
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 0f9e1ea..a45d628 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1365,6 +1365,13 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
target_put_sess_cmd(se_sess, se_cmd);
return 0;
}
+
+ rc = target_setup_cmd_from_cdb(se_cmd, cdb);
+ if (rc != 0) {
+ transport_generic_request_failure(se_cmd, rc);
+ return 0;
+ }
+
/*
* Save pointers for SGLs containing protection information,
* if present.
@@ -1374,11 +1381,19 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
se_cmd->t_prot_nents = sgl_prot_count;
}
- rc = target_setup_cmd_from_cdb(se_cmd, cdb);
- if (rc != 0) {
- transport_generic_request_failure(se_cmd, rc);
+ /*
+ * Fail if protection operation requiers protection
+ * information buffers but None are provided!
+ */
+ if ((!se_cmd->t_prot_sg || !se_cmd->t_prot_nents) &&
+ (se_cmd->prot_op != TARGET_PROT_NORMAL)) {
+ pr_err("ERROR: protection information was requested but "
+ "protection buffers weren't provided.\n");
+ transport_generic_request_failure(se_cmd,
+ TCM_INVALID_CDB_FIELD);
return 0;
}
+
It seems that loopback functionality was broken in this one (but it was
somewhat wrong to start with).
The thing is, the startup READs are not passed with protection buffers
and asks the LLD to insert/strip DIF (SCSI_PROT_READ_STRIP)
and a couple of READs later DIX is enabled and starts passing protection
information (SCSI_PROT_READ_PASS).
Since loopback initiator is not aware of this, startup READs will fail
in this commit. Same for disabling write_generate/read_verify.
I think that the loopback initiator should conform to the protection
operation in scsi_cmnd.
Meaning it should INSERT/STRIP/PASS according to what was stated in the
SCSI command:
SCSI_PROT_READ_STRIP: should allocate protection protection sg and
execute target_submit_cmd_map_sgls().
SCSI_PROT_WRITE_GENERATE: should allocate protection protection sg,
compute protection and execute target_submit_cmd_map_sgls().
SCSI_PROT_READ_INSERT: don't know if worth the bother for loopback.
SCSI_PROT_WRITE_STRIP: don't know if worth the bother for loopback.
SCSI_PROT_READ_PASS: stay the same.
SCSI_PROT_WRITE_PASS: stay the same.
The alternative is to avoid this patch but then we might miss DIF
support when working against legacy initiators.
Thoughts?
Mmmm, not sure about this one..
Considering that it's only the first two READ_10s that this effects
before normal SCSI_PROT_*_PASS operation kicks in, I'm not sure if it's
worth it to add these to tcm_loop..
Note that it won't be operational when disabling
read_verify/write_generate via /sys/block/$DEV/integrity/ as well.
Note that virtio-scsi + vhost-scsi
would need similar bits as well..
sgl allocation can be moved to target_core_base.h and be used by all,
and we can put DIF generation in target_core_sbc.c for all to use.
So I'd prefer to get target-core working for both types of cases, vs.
adding READ_STRIP + WRITE_GENERATE emulation logic to tcm_loop +
virtio-scsi LLDs.
MKP, any preference here..?
I see, I can live with that...
Just thought it would be nice to provide (half-way) protection also
against legacy initiators.
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
