From 58642c6d72f10e974fdbda18eb374d007155c7e9 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Tue, 29 May 2018 16:36:38 +0200 Subject: [PATCH 1/4] tests: drop duplicate PR-unsupported log messages The helper function -2 return paths already print this message, so the duplicate in the caller can be dropped. Signed-off-by: David Disseldorp --- test-tool/test_prin_read_keys_simple.c | 1 - test-tool/test_prin_report_caps.c | 1 - test-tool/test_prin_serviceaction_range.c | 1 - test-tool/test_prout_clear_simple.c | 1 - test-tool/test_prout_preempt.c | 1 - test-tool/test_prout_register_simple.c | 1 - test-tool/test_prout_reserve_access.c | 1 - test-tool/test_prout_reserve_ownership.c | 1 - test-tool/test_prout_reserve_simple.c | 1 - 9 files changed, 9 deletions(-) diff --git a/test-tool/test_prin_read_keys_simple.c b/test-tool/test_prin_read_keys_simple.c index 127925c..f268e96 100644 --- a/test-tool/test_prin_read_keys_simple.c +++ b/test-tool/test_prin_read_keys_simple.c @@ -39,7 +39,6 @@ test_prin_read_keys_simple(void) ret = prin_read_keys(sd, &task, NULL); if (ret == -2) { - logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE IN is not implemented."); CU_PASS("PERSISTENT RESERVE IN is not implemented."); return; } diff --git a/test-tool/test_prin_report_caps.c b/test-tool/test_prin_report_caps.c index 705cfb5..8e6c302 100644 --- a/test-tool/test_prin_report_caps.c +++ b/test-tool/test_prin_report_caps.c @@ -63,7 +63,6 @@ test_prin_report_caps_simple(void) /* register our reservation key with the target */ ret = prout_register_and_ignore(sd, key); if (ret == -2) { - logging(LOG_NORMAL, "[SKIPPED] PERSISTENT RESERVE OUT is not implemented."); CU_PASS("PERSISTENT RESERVE OUT is not implemented."); return; } diff --git a/test-tool/test_prin_serviceaction_range.c b/test-tool/test_prin_serviceaction_range.c index 4551c0a..e261d6a 100644 --- a/test-tool/test_prin_serviceaction_range.c +++ b/test-tool/test_prin_serviceaction_range.c @@ -40,7 +40,6 @@ test_prin_serviceaction_range(void) /* verify PRIN/READ_KEYS works -- XXX redundant -- remove this? */ ret = prin_read_keys(sd, &task, NULL); if (ret == -2) { - logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE IN is not implemented."); CU_PASS("PERSISTENT RESERVE IN is not implemented."); return; } diff --git a/test-tool/test_prout_clear_simple.c b/test-tool/test_prout_clear_simple.c index 8bcfe47..2203998 100644 --- a/test-tool/test_prout_clear_simple.c +++ b/test-tool/test_prout_clear_simple.c @@ -43,7 +43,6 @@ test_prout_clear_simple(void) /* register our reservation key with the target */ ret = prout_register_and_ignore(sd, key); if (ret == -2) { - logging(LOG_NORMAL, "[SKIPPED] PERSISTENT RESERVE OUT is not implemented."); CU_PASS("PERSISTENT RESERVE OUT is not implemented."); return; } diff --git a/test-tool/test_prout_preempt.c b/test-tool/test_prout_preempt.c index 201acca..90c0f76 100644 --- a/test-tool/test_prout_preempt.c +++ b/test-tool/test_prout_preempt.c @@ -54,7 +54,6 @@ test_prout_preempt_rm_reg(void) ret = prout_register_and_ignore(sd, k1); if (ret == -2) { - logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented."); CU_PASS("PERSISTENT RESERVE OUT is not implemented."); return; } diff --git a/test-tool/test_prout_register_simple.c b/test-tool/test_prout_register_simple.c index c705875..275f692 100644 --- a/test-tool/test_prout_register_simple.c +++ b/test-tool/test_prout_register_simple.c @@ -40,7 +40,6 @@ test_prout_register_simple(void) /* register our reservation key with the target */ ret = prout_register_and_ignore(sd, key); if (ret == -2) { - logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented."); CU_PASS("PERSISTENT RESERVE OUT is not implemented."); return; } diff --git a/test-tool/test_prout_reserve_access.c b/test-tool/test_prout_reserve_access.c index 4f17848..2da88e0 100644 --- a/test-tool/test_prout_reserve_access.c +++ b/test-tool/test_prout_reserve_access.c @@ -50,7 +50,6 @@ verify_persistent_reserve_access(struct scsi_device *sd1, struct scsi_device *sd /* register our reservation key with the target */ ret = prout_register_and_ignore(sd1, key); if (ret == -2) { - logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented."); CU_PASS("PERSISTENT RESERVE OUT is not implemented."); return; } diff --git a/test-tool/test_prout_reserve_ownership.c b/test-tool/test_prout_reserve_ownership.c index b14478c..1b47758 100644 --- a/test-tool/test_prout_reserve_ownership.c +++ b/test-tool/test_prout_reserve_ownership.c @@ -47,7 +47,6 @@ verify_persistent_reserve_ownership(struct scsi_device *sd1, struct scsi_device /* register our reservation key with the target */ ret = prout_register_and_ignore(sd1, key1); if (ret == -2) { - logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented."); CU_PASS("PERSISTENT RESERVE OUT is not implemented."); return; } diff --git a/test-tool/test_prout_reserve_simple.c b/test-tool/test_prout_reserve_simple.c index 14d4793..4f9ebbd 100644 --- a/test-tool/test_prout_reserve_simple.c +++ b/test-tool/test_prout_reserve_simple.c @@ -55,7 +55,6 @@ test_prout_reserve_simple(void) /* register our reservation key with the target */ ret = prout_register_and_ignore(sd, key); if (ret == -2) { - logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented."); CU_PASS("PERSISTENT RESERVE OUT is not implemented."); return; } From 31ab1e1ac9815bec9c63ab12799fe4aa6c5be091 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Thu, 31 May 2018 22:49:04 +0200 Subject: [PATCH 2/4] test-tool: add prin_read_keys() allocation_len parameter Accepting an Allocation Length parameter allows us to test for truncation of response data, as per SPC5r17 4.2.5.6: The device server shall terminate transfers to the Data-In Buffer when the number of bytes or blocks specified by the ALLOCATION LENGTH field have been transferred or when all available data have been transferred, whichever is less. With this change, all existing prin_read_keys() callers continue to use same ALLOCATION LENGTH value as earlier (16K). Signed-off-by: David Disseldorp --- test-tool/iscsi-support.c | 9 +++++---- test-tool/iscsi-support.h | 3 ++- test-tool/iscsi-test-cu.c | 2 +- test-tool/test_prin_read_keys_simple.c | 2 +- test-tool/test_prin_serviceaction_range.c | 4 ++-- test-tool/test_prout_clear_simple.c | 6 +++--- test-tool/test_prout_preempt.c | 4 ++-- 7 files changed, 16 insertions(+), 14 deletions(-) diff --git a/test-tool/iscsi-support.c b/test-tool/iscsi-support.c index 958dce9..f0446a6 100644 --- a/test-tool/iscsi-support.c +++ b/test-tool/iscsi-support.c @@ -667,17 +667,18 @@ prin_task(struct scsi_device *sdev, int service_action, } int -prin_read_keys(struct scsi_device *sdev, struct scsi_task **tp, - struct scsi_persistent_reserve_in_read_keys **rkp) +prin_read_keys(struct scsi_device *sdev, + struct scsi_task **tp, + struct scsi_persistent_reserve_in_read_keys **rkp, + uint16_t allocation_len) { - const int buf_sz = 16384; struct scsi_persistent_reserve_in_read_keys *rk = NULL; logging(LOG_VERBOSE, "Send PRIN/READ_KEYS"); *tp = scsi_cdb_persistent_reserve_in(SCSI_PERSISTENT_RESERVE_READ_KEYS, - buf_sz); + allocation_len); assert(*tp != NULL); *tp = send_scsi_command(sdev, *tp, NULL); diff --git a/test-tool/iscsi-support.h b/test-tool/iscsi-support.h index 283d597..5f79d8b 100644 --- a/test-tool/iscsi-support.h +++ b/test-tool/iscsi-support.h @@ -822,7 +822,8 @@ int all_zero(const unsigned char *buf, unsigned size); int prin_task(struct scsi_device *sdev, int service_action, int success_expected); int prin_read_keys(struct scsi_device *sdev, struct scsi_task **tp, - struct scsi_persistent_reserve_in_read_keys **rkp); + struct scsi_persistent_reserve_in_read_keys **rkp, + uint16_t allocation_len); int prout_register_and_ignore(struct scsi_device *sdev, unsigned long long key); int prout_register_key(struct scsi_device *sdev, diff --git a/test-tool/iscsi-test-cu.c b/test-tool/iscsi-test-cu.c index 3ec3b09..a4c4a0b 100644 --- a/test-tool/iscsi-test-cu.c +++ b/test-tool/iscsi-test-cu.c @@ -1069,7 +1069,7 @@ static int clear_pr(struct scsi_device *sdev) struct scsi_persistent_reserve_in_read_keys *rk; res = 0; - if (prin_read_keys(sdev, &pr_task, &rk) != 0) + if (prin_read_keys(sdev, &pr_task, &rk, 16384) != 0) goto out; res = -1; diff --git a/test-tool/test_prin_read_keys_simple.c b/test-tool/test_prin_read_keys_simple.c index f268e96..5797091 100644 --- a/test-tool/test_prin_read_keys_simple.c +++ b/test-tool/test_prin_read_keys_simple.c @@ -37,7 +37,7 @@ test_prin_read_keys_simple(void) logging(LOG_VERBOSE, LOG_BLANK_LINE); logging(LOG_VERBOSE, "Test Persistent Reserve IN READ_KEYS works."); - ret = prin_read_keys(sd, &task, NULL); + ret = prin_read_keys(sd, &task, NULL, 16384); if (ret == -2) { CU_PASS("PERSISTENT RESERVE IN is not implemented."); return; diff --git a/test-tool/test_prin_serviceaction_range.c b/test-tool/test_prin_serviceaction_range.c index e261d6a..4bc1094 100644 --- a/test-tool/test_prin_serviceaction_range.c +++ b/test-tool/test_prin_serviceaction_range.c @@ -38,11 +38,11 @@ test_prin_serviceaction_range(void) logging(LOG_VERBOSE, "Test Persistent Reserve IN Serviceaction range."); /* verify PRIN/READ_KEYS works -- XXX redundant -- remove this? */ - ret = prin_read_keys(sd, &task, NULL); + ret = prin_read_keys(sd, &task, NULL, 16384); if (ret == -2) { CU_PASS("PERSISTENT RESERVE IN is not implemented."); return; - } + } CU_ASSERT_EQUAL(ret, 0); /* verify that PRIN/SA={0,1,2,3} works ... */ diff --git a/test-tool/test_prout_clear_simple.c b/test-tool/test_prout_clear_simple.c index 2203998..c570945 100644 --- a/test-tool/test_prout_clear_simple.c +++ b/test-tool/test_prout_clear_simple.c @@ -45,10 +45,10 @@ test_prout_clear_simple(void) if (ret == -2) { CU_PASS("PERSISTENT RESERVE OUT is not implemented."); return; - } + } CU_ASSERT_EQUAL(ret, 0); - ret = prin_read_keys(sd, &tsk, &rk); + ret = prin_read_keys(sd, &tsk, &rk, 16384); CU_ASSERT_EQUAL(ret, 0); CU_ASSERT_NOT_EQUAL(rk, NULL); if (!rk) @@ -78,7 +78,7 @@ test_prout_clear_simple(void) ret = prin_verify_not_reserved(sd); CU_ASSERT_EQUAL(ret, 0); - ret = prin_read_keys(sd, &tsk, &rk); + ret = prin_read_keys(sd, &tsk, &rk, 16384); CU_ASSERT_EQUAL(ret, 0); CU_ASSERT_NOT_EQUAL(rk, NULL); if (!rk) diff --git a/test-tool/test_prout_preempt.c b/test-tool/test_prout_preempt.c index 90c0f76..0c11c33 100644 --- a/test-tool/test_prout_preempt.c +++ b/test-tool/test_prout_preempt.c @@ -78,7 +78,7 @@ test_prout_preempt_rm_reg(void) CU_ASSERT_EQUAL(ret, 0); /* confirm that k1 and k2 are registered */ - ret = prin_read_keys(sd, &tsk, &rk); + ret = prin_read_keys(sd, &tsk, &rk, 16384); CU_ASSERT_EQUAL_FATAL(ret, 0); CU_ASSERT_EQUAL(rk->num_keys, 2); @@ -99,7 +99,7 @@ test_prout_preempt_rm_reg(void) ret = test_iscsi_tur_until_good(sd2, &num_uas); CU_ASSERT_EQUAL(ret, 0); - ret = prin_read_keys(sd, &tsk, &rk); + ret = prin_read_keys(sd, &tsk, &rk, 16384); CU_ASSERT_EQUAL_FATAL(ret, 0); CU_ASSERT_EQUAL(rk->num_keys, 1); From c88e9715abbe5660cf14903c1e22c245eedeed33 Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Thu, 31 May 2018 23:10:28 +0200 Subject: [PATCH 3/4] lib/scsi: fix SCSI_PERSISTENT_RESERVE_READ_KEYS handling When unmarshalling a SCSI_PERSISTENT_RESERVE_READ_KEYS response, scsi_persistentreservein_datain_unmarshall() assumes that the ADDITIONAL LENGTH field represents the number of keys packed in the key array. This is incorrect as key array data buffer may be truncated while ADDITIONAL LENGTH is left in tact, as per SPC5r17 4.2.5.6: If the information being transferred to the Data-In Buffer includes fields containing counts ..., then the contents of these fields shall not be altered to reflect the truncation, if any, that results from an insufficient ALLOCATION LENGTH value, unless the standard that describes the Data-In Buffer format states otherwise. Determine the number of keys returned based on the minimum of the data-in length and the ADDITIONAL LENGTH value. Signed-off-by: David Disseldorp --- lib/scsi-lowlevel.c | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/lib/scsi-lowlevel.c b/lib/scsi-lowlevel.c index d68d990..5ddd709 100644 --- a/lib/scsi-lowlevel.c +++ b/lib/scsi-lowlevel.c @@ -962,7 +962,9 @@ scsi_receivecopyresults_datain_unmarshall(struct scsi_task *task) } } - +#ifndef MIN /* instead of including all of iscsi-private.h */ +#define MIN(a, b) (((a) < (b)) ? (a) : (b)) +#endif static void * scsi_persistentreservein_datain_unmarshall(struct scsi_task *task) { @@ -972,21 +974,44 @@ scsi_persistentreservein_datain_unmarshall(struct scsi_task *task) int i; switch (scsi_persistentreservein_sa(task)) { - case SCSI_PERSISTENT_RESERVE_READ_KEYS: - i = task_get_uint32(task, 4); + case SCSI_PERSISTENT_RESERVE_READ_KEYS: { + uint32_t cdb_keys_len; + uint32_t data_keys_len; + uint32_t keys_len; - rk = scsi_malloc(task, offsetof(struct scsi_persistent_reserve_in_read_keys, keys) + i); + if (task->datain.size < 8) { + return NULL; + } + + /* + * SPC5r17: 6.16.2 READ KEYS service action + * The ADDITIONAL LENGTH field indicates the number of bytes in + * the Reservation key list. The contents of the ADDITIONAL + * LENGTH field are not altered based on the allocation length. + */ + cdb_keys_len = task_get_uint32(task, 4); + data_keys_len = task->datain.size - 8; + /* + * Only process as many keys as permitted by the given + * ADDITIONAL LENGTH and data-in size limits. + */ + keys_len = MIN(cdb_keys_len, data_keys_len); + + rk = scsi_malloc(task, + offsetof(struct scsi_persistent_reserve_in_read_keys, + keys) + keys_len); if (rk == NULL) { return NULL; } rk->prgeneration = task_get_uint32(task, 0); - rk->additional_length = task_get_uint32(task, 4); + rk->additional_length = cdb_keys_len; - rk->num_keys = rk->additional_length / 8; + rk->num_keys = keys_len / 8; for (i = 0; i < (int)rk->num_keys; i++) { rk->keys[i] = task_get_uint64(task, 8 + i * 8); } return rk; + } case SCSI_PERSISTENT_RESERVE_READ_RESERVATION: { size_t alloc_sz; From ab0e40d6899b5706bfa77f3c6c2b469dbb7798db Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Thu, 31 May 2018 23:23:39 +0200 Subject: [PATCH 4/4] test-tool: add PrinReadKeys.Truncate test This test registers PR key and then sends a PR-In read-keys request with an ALLOCATION LENGTH value that forces response truncation. Ensure that the ADDITIONAL LENGTH is *not* modified as a result of the truncation. This test currently passes against TGT but fails against LIO (fix pending). Signed-off-by: David Disseldorp --- test-tool/Makefile.am | 1 + test-tool/iscsi-test-cu.c | 1 + test-tool/iscsi-test-cu.h | 1 + test-tool/test_prin_read_keys_truncate.c | 74 ++++++++++++++++++++++++ 4 files changed, 77 insertions(+) create mode 100644 test-tool/test_prin_read_keys_truncate.c diff --git a/test-tool/Makefile.am b/test-tool/Makefile.am index 13d8663..c658d0d 100644 --- a/test-tool/Makefile.am +++ b/test-tool/Makefile.am @@ -70,6 +70,7 @@ iscsi_test_cu_SOURCES = iscsi-test-cu.c \ test_preventallow_lun_reset.c \ test_preventallow_2_itnexuses.c \ test_prin_read_keys_simple.c \ + test_prin_read_keys_truncate.c \ test_prin_serviceaction_range.c \ test_prin_report_caps.c \ test_prout_register_simple.c \ diff --git a/test-tool/iscsi-test-cu.c b/test-tool/iscsi-test-cu.c index a4c4a0b..76a16e0 100644 --- a/test-tool/iscsi-test-cu.c +++ b/test-tool/iscsi-test-cu.c @@ -158,6 +158,7 @@ static CU_TestInfo tests_preventallow[] = { static CU_TestInfo tests_prin_read_keys[] = { { (char *)"Simple", test_prin_read_keys_simple }, + { (char *)"Truncate", test_prin_read_keys_truncate }, CU_TEST_INFO_NULL }; diff --git a/test-tool/iscsi-test-cu.h b/test-tool/iscsi-test-cu.h index adfc776..d88c6a7 100644 --- a/test-tool/iscsi-test-cu.h +++ b/test-tool/iscsi-test-cu.h @@ -115,6 +115,7 @@ void test_preventallow_lun_reset(void); void test_preventallow_2_itnexuses(void); void test_prin_read_keys_simple(void); +void test_prin_read_keys_truncate(void); void test_prin_serviceaction_range(void); void test_prin_report_caps_simple(void); diff --git a/test-tool/test_prin_read_keys_truncate.c b/test-tool/test_prin_read_keys_truncate.c new file mode 100644 index 0000000..1590b33 --- /dev/null +++ b/test-tool/test_prin_read_keys_truncate.c @@ -0,0 +1,74 @@ +/* -*- mode:c; tab-width:8; c-basic-offset:8; indent-tabs-mode:nil; -*- */ +/* + Copyright (C) 2013 by Lee Duncan + Copyright (C) 2018 David Disseldorp + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, see . +*/ + +#include +#include + +#include + +#include "iscsi.h" +#include "scsi-lowlevel.h" +#include "iscsi-support.h" +#include "iscsi-test-cu.h" + + +void +test_prin_read_keys_truncate(void) +{ + const unsigned long long key = rand_key(); + struct scsi_persistent_reserve_in_read_keys *rk = NULL; + int ret; + + logging(LOG_VERBOSE, LOG_BLANK_LINE); + logging(LOG_VERBOSE, "Test Persistent Reserve IN READ_KEYS works when " + "truncated."); + + ret = prout_register_and_ignore(sd, key); + if (ret == -2) { + CU_PASS("PERSISTENT RESERVE OUT is not implemented."); + return; + } + CU_ASSERT_EQUAL(ret, 0); + + /* + * alloc_len=8 restricts the response buffer to only accepting the + * PRGENERATION and ADDITIONAL LENGTH fields. + */ + ret = prin_read_keys(sd, &task, &rk, 8); + if (ret == -2) { + CU_PASS("PERSISTENT RESERVE IN is not implemented."); + prout_register_key(sd, 0, key); + return; + } + CU_ASSERT_EQUAL(ret, 0); + + /* + * SPC5r17: 6.16.2 READ KEYS service action + * The ADDITIONAL LENGTH field indicates the number of bytes in + * the Reservation key list. The contents of the ADDITIONAL + * LENGTH field are not altered based on the allocation length. + */ + CU_ASSERT_NOT_EQUAL(rk->additional_length, 0); + /* key array should have been truncated in the response */ + CU_ASSERT_EQUAL(rk->num_keys, 0); + + /* remove our key from the target */ + ret = prout_register_key(sd, 0, key); + CU_ASSERT_EQUAL(0, ret); +}