On 2020/05/08 0:40, Douglas Gilbert wrote:
> This driver attempts to help the application client in the case of
> ILLEGAL REQUEST by using the field pointer information mechanism
> to point to the location in a cdb or parameter block that triggered
> an error. Some cases of the ILLEGAL REQUEST sense key being issued
> without field pointer information snuck into the recently added zone
> commands. There were also pre-existing cases that are picked up by
> this patch.
>
> The change is to use mk_sense_invalid_fld() rather than
> mk_sense_buffer() and supply the extra information to the former.
> Sometimes that is not so easy since the exact byte offset in the
> cdb for the family of WRITE commands, for example, is "up the stack"
> when some such errors are detected. In these cases incomplete field
> pointer information is passed backed to the level that can see the
> cbd_s at which point the sense data is rewritten in full.
>
> Uses the scsi_set_sense_field_pointer() library function to replace
> open coding of the same logic.
>
> This patch is on top of the patchset whose cover pages is:
> [PATCH 0/7] scsi_debug: Add ZBC support
> and
> [PATCH] scsi_debug: Fix compilation error on 32bits arch
> both by Damien Le Moal
>
> Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
> ---
> drivers/scsi/scsi_debug.c | 164 ++++++++++++++++++++++++++------------
> 1 file changed, 115 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 79a48dd1b9e4..420145af1e5c 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -61,7 +61,7 @@
>
> /* make sure inq_product_rev string corresponds to this version */
> #define SDEBUG_VERSION "0189" /* format to fit INQUIRY revision field */
> -static const char *sdebug_version_date = "20200421";
> +static const char *sdebug_version_date = "20200506";
>
> #define MY_NAME "scsi_debug"
>
> @@ -925,8 +925,7 @@ static void mk_sense_invalid_fld(struct scsi_cmnd *scp,
> int in_byte, int in_bit)
> {
> unsigned char *sbuff;
> - u8 sks[4];
> - int sl, asc;
> + int asc;
>
> sbuff = scp->sense_buffer;
> if (!sbuff) {
> @@ -937,29 +936,28 @@ static void mk_sense_invalid_fld(struct scsi_cmnd *scp,
> asc = c_d ? INVALID_FIELD_IN_CDB : INVALID_FIELD_IN_PARAM_LIST;
> memset(sbuff, 0, SCSI_SENSE_BUFFERSIZE);
> scsi_build_sense_buffer(sdebug_dsense, sbuff, ILLEGAL_REQUEST, asc, 0);
> - memset(sks, 0, sizeof(sks));
> - sks[0] = 0x80;
> - if (c_d)
> - sks[0] |= 0x40;
> - if (in_bit >= 0) {
> - sks[0] |= 0x8;
> - sks[0] |= 0x7 & in_bit;
> - }
> - put_unaligned_be16(in_byte, sks + 1);
> - if (sdebug_dsense) {
> - sl = sbuff[7] + 8;
> - sbuff[7] = sl;
> - sbuff[sl] = 0x2;
> - sbuff[sl + 1] = 0x6;
> - memcpy(sbuff + sl + 4, sks, 3);
> - } else
> - memcpy(sbuff + 15, sks, 3);
> + scsi_set_sense_field_pointer(sbuff, SCSI_SENSE_BUFFERSIZE, in_byte,
> + (in_bit < 0 ? 8 : in_bit), (bool)c_d);
> if (sdebug_verbose)
> sdev_printk(KERN_INFO, scp->device, "%s: [sense_key,asc,ascq"
> "]: [0x5,0x%x,0x0] %c byte=%d, bit=%d\n",
> my_name, asc, c_d ? 'C' : 'D', in_byte, in_bit);
> }
>
> +static bool have_sense_invalid_fld_cdb(struct scsi_cmnd *scp)
> +{
> + if (!scp->sense_buffer)
> + return false;
> + if (sdebug_dsense)
> + return ((scp->sense_buffer[1] & 0xf) == ILLEGAL_REQUEST &&
> + scp->sense_buffer[2] == INVALID_FIELD_IN_CDB &&
> + scp->sense_buffer[3] == 0);
> + else
No need for the else here.
> + return ((scp->sense_buffer[2] & 0xf) == ILLEGAL_REQUEST &&
> + scp->sense_buffer[12] == INVALID_FIELD_IN_CDB &&
> + scp->sense_buffer[13] == 0);
> +}
> +
> static void mk_sense_buffer(struct scsi_cmnd *scp, int key, int asc, int asq)
> {
> unsigned char *sbuff;
> @@ -2777,14 +2775,14 @@ static void zbc_inc_wp(struct sdebug_dev_info *devip,
> }
>
> static int check_zbc_access_params(struct scsi_cmnd *scp,
> - unsigned long long lba, unsigned int num, bool write)
> + unsigned long long lba, unsigned int num, bool data_out)
> {
> struct scsi_device *sdp = scp->device;
> struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
> struct sdeb_zone_state *zsp = zbc_zone(devip, lba);
> struct sdeb_zone_state *zsp_end = zbc_zone(devip, lba + num - 1);
>
> - if (!write) {
> + if (!data_out) {
> if (devip->zmodel == BLK_ZONED_HA)
> return 0;
> /* For host-managed, reads cannot cross zone types boundaries */
> @@ -2820,8 +2818,8 @@ static int check_zbc_access_params(struct scsi_cmnd *scp,
> }
> /* Cannot write full zones */
> if (zsp->z_cond == ZC5_FULL) {
> - mk_sense_buffer(scp, ILLEGAL_REQUEST,
> - INVALID_FIELD_IN_CDB, 0);
> + /* want sLBA position in cdb, fix up later */
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 0, -1);
> return check_condition_result;
> }
> /* Writes must be aligned to the zone WP */
> @@ -2848,9 +2846,10 @@ static int check_zbc_access_params(struct scsi_cmnd *scp,
> return 0;
> }
>
> +/* Last argument should only be true when data-out and media modifying */
> static inline int check_device_access_params
> (struct scsi_cmnd *scp, unsigned long long lba,
> - unsigned int num, bool write)
> + unsigned int num, bool modifying)
> {
> struct scsi_device *sdp = scp->device;
> struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
> @@ -2861,16 +2860,16 @@ static inline int check_device_access_params
> }
> /* transfer length excessive (tie in to block limits VPD page) */
> if (num > sdebug_store_sectors) {
> - /* needs work to find which cdb byte 'num' comes from */
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + /* want num offset in cdb, fix up later */
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 0, -1);
> return check_condition_result;
> }
> - if (write && unlikely(sdebug_wp)) {
> + if (modifying && unlikely(sdebug_wp)) {
> mk_sense_buffer(scp, DATA_PROTECT, WRITE_PROTECTED, 0x2);
> return check_condition_result;
> }
> if (sdebug_dev_is_zoned(devip))
> - return check_zbc_access_params(scp, lba, num, write);
> + return check_zbc_access_params(scp, lba, num, modifying);
>
> return 0;
> }
> @@ -3462,6 +3461,36 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
> write_lock(macc_lckp);
> ret = check_device_access_params(scp, lba, num, true);
> if (ret) {
> + if (have_sense_invalid_fld_cdb(scp)) {
> + bool is_zbc = (sdeb_zbc_model != 0);
> + int lba_o, num_o;
> +
> + switch (cmd[0]) {
> + case WRITE_16:
> + lba_o = 2;
> + num_o = 10;
> + break;
> + case WRITE_10:
> + case 0x53:
> + lba_o = 2;
> + num_o = 7;
> + break;
> + case WRITE_6:
> + lba_o = 1;
> + num_o = 4;
> + break;
> + case WRITE_12:
> + lba_o = 2;
> + num_o = 6;
> + break;
> + default: /* assume WRITE(32) */
> + lba_o = 20;
> + num_o = 28;
> + break;
> + }
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB,
> + (is_zbc ? lba_o : num_o), -1);
> + }
> write_unlock(macc_lckp);
> return ret;
> }
> @@ -3568,7 +3597,7 @@ static int resp_write_scat(struct scsi_cmnd *scp,
> sdev_printk(KERN_INFO, scp->device,
> "%s: %s: LB Data Offset field bad\n",
> my_name, __func__);
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, (is_16 ? 4 : 12), -1);
> return illegal_condition_result;
> }
> lbdof_blen = lbdof * lb_size;
> @@ -3577,7 +3606,7 @@ static int resp_write_scat(struct scsi_cmnd *scp,
> sdev_printk(KERN_INFO, scp->device,
> "%s: %s: LBA range descriptors don't fit\n",
> my_name, __func__);
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, (is_16 ? 8 : 16), -1);
> return illegal_condition_result;
> }
> lrdp = kzalloc(lbdof_blen, GFP_ATOMIC);
> @@ -3607,8 +3636,16 @@ static int resp_write_scat(struct scsi_cmnd *scp,
> if (num == 0)
> continue;
> ret = check_device_access_params(scp, lba, num, true);
> - if (ret)
> + if (ret) {
> + if (have_sense_invalid_fld_cdb(scp)) {
> + bool is_zbc = (sdeb_zbc_model != 0);
> + int off = is_zbc ? 0 : 8;
is_zbc is not really needed here, I think.
> +
> + mk_sense_invalid_fld(scp, SDEB_IN_DATA,
> + (up - lrdp) + off, -1);
> + }
> goto err_out_unlock;
> + }
> num_by = num * lb_size;
> ei_lba = is_16 ? 0 : get_unaligned_be32(up + 12);
>
> @@ -3703,7 +3740,7 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
> write_lock(macc_lckp);
>
> ret = check_device_access_params(scp, lba, num, true);
> - if (ret) {
> + if (ret) { /* illegal request fixup next level up */
> write_unlock(macc_lckp);
> return ret;
> }
> @@ -3755,6 +3792,7 @@ static int resp_write_same_10(struct scsi_cmnd *scp,
> u32 lba;
> u16 num;
> u32 ei_lba = 0;
> + int res;
> bool unmap = false;
>
> if (cmd[1] & 0x8) {
> @@ -3770,7 +3808,13 @@ static int resp_write_same_10(struct scsi_cmnd *scp,
> mk_sense_invalid_fld(scp, SDEB_IN_CDB, 7, -1);
> return check_condition_result;
> }
> - return resp_write_same(scp, lba, num, ei_lba, unmap, false);
> + res = resp_write_same(scp, lba, num, ei_lba, unmap, false);
> + if (have_sense_invalid_fld_cdb(scp)) {
> + bool is_zbc = (sdeb_zbc_model != 0);
> +
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, is_zbc ? 2 : 7, -1);
> + }
> + return res;
> }
>
> static int resp_write_same_16(struct scsi_cmnd *scp,
> @@ -3780,6 +3824,7 @@ static int resp_write_same_16(struct scsi_cmnd *scp,
> u64 lba;
> u32 num;
> u32 ei_lba = 0;
> + int res;
> bool unmap = false;
> bool ndob = false;
>
> @@ -3798,7 +3843,13 @@ static int resp_write_same_16(struct scsi_cmnd *scp,
> mk_sense_invalid_fld(scp, SDEB_IN_CDB, 10, -1);
> return check_condition_result;
> }
> - return resp_write_same(scp, lba, num, ei_lba, unmap, ndob);
> + res = resp_write_same(scp, lba, num, ei_lba, unmap, ndob);
> + if (have_sense_invalid_fld_cdb(scp)) {
> + bool is_zbc = (sdeb_zbc_model != 0);
> +
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, is_zbc ? 2 : 10, -1);
> + }
> + return res;
> }
>
> /* Note the mode field is in the same position as the (lower) service action
> @@ -3878,9 +3929,16 @@ static int resp_comp_write(struct scsi_cmnd *scp,
> (cmd[1] & 0xe0) == 0)
> sdev_printk(KERN_ERR, scp->device, "Unprotected WR "
> "to DIF device\n");
> - ret = check_device_access_params(scp, lba, num, false);
> - if (ret)
> + ret = check_device_access_params(scp, lba, num, true);
> + if (ret) {
> + if (have_sense_invalid_fld_cdb(scp)) {
> + bool is_zbc = (sdeb_zbc_model != 0);
> + int off = is_zbc ? 2 : 13;
Same here, is_zbc is not really needed.
> +
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, off, -1);
> + }
> return ret;
> + }
> dnum = 2 * num;
> arr = kcalloc(lb_size, dnum, GFP_ATOMIC);
> if (NULL == arr) {
> @@ -3959,8 +4017,17 @@ static int resp_unmap(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
> unsigned int num = get_unaligned_be32(&desc[i].blocks);
>
> ret = check_device_access_params(scp, lba, num, true);
> - if (ret)
> + if (ret) {
> + if (have_sense_invalid_fld_cdb(scp)) {
> + bool is_zbc = (sdeb_zbc_model != 0);
> + u8 *offp = (u8 *)&desc[i].lba +
> + (is_zbc ? 0 : 8);
> +
> + mk_sense_invalid_fld(scp, SDEB_IN_DATA,
> + (offp - buf), -1);
> + }
This pattern:
if (have_sense_invalid_fld_cdb(scp)) {
bool is_zbc = ...
mk_sense_invalid_fld(scp, SDEB_IN_DATA, ...values...);
}
is repeated multiple times. It would be nice to have it as a helper function
that sorts out all the values based on the scp. Even if that repeats some code
for parsing, overall, the code would be cleaner I think.
> goto out;
> + }
>
> unmap_region(sip, lba, num);
> }
> @@ -4230,7 +4297,7 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
> return check_condition_result;
> }
> a_num = is_bytchk3 ? 1 : vnum;
> - /* Treat following check like one for read (i.e. no write) access */
> + /* This is data-out but not media modifying, so last argument false */
> ret = check_device_access_params(scp, lba, a_num, false);
> if (ret)
> return ret;
> @@ -4367,8 +4434,7 @@ static int resp_report_zones(struct scsi_cmnd *scp,
> continue;
> break;
> default:
> - mk_sense_buffer(scp, ILLEGAL_REQUEST,
> - INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 14, 5);
> ret = check_condition_result;
> goto fini;
> }
> @@ -4458,12 +4524,12 @@ static int resp_open_zone(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>
> zsp = zbc_zone(devip, z_id);
> if (z_id != zsp->z_start) {
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 2, -1);
> res = check_condition_result;
> goto fini;
> }
> if (zbc_zone_is_conv(zsp)) {
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 2 /* z_id */, -1);
You added the /* z_id */ comment only here and not to the other cases for close
etc. So may be be consistent and remove it here ? I do not think it is really
needed.
> res = check_condition_result;
> goto fini;
> }
> @@ -4528,12 +4594,12 @@ static int resp_close_zone(struct scsi_cmnd *scp,
>
> zsp = zbc_zone(devip, z_id);
> if (z_id != zsp->z_start) {
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 2, -1);
> res = check_condition_result;
> goto fini;
> }
> if (zbc_zone_is_conv(zsp)) {
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 2, -1);
> res = check_condition_result;
> goto fini;
> }
> @@ -4601,12 +4667,12 @@ static int resp_finish_zone(struct scsi_cmnd *scp,
>
> zsp = zbc_zone(devip, z_id);
> if (z_id != zsp->z_start) {
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 2, -1);
> res = check_condition_result;
> goto fini;
> }
> if (zbc_zone_is_conv(zsp)) {
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 2, -1);
> res = check_condition_result;
> goto fini;
> }
> @@ -4676,12 +4742,12 @@ static int resp_rwp_zone(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>
> zsp = zbc_zone(devip, z_id);
> if (z_id != zsp->z_start) {
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 2, -1);
> res = check_condition_result;
> goto fini;
> }
> if (zbc_zone_is_conv(zsp)) {
> - mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
> + mk_sense_invalid_fld(scp, SDEB_IN_CDB, 2, -1);
> res = check_condition_result;
> goto fini;
> }
>
--
Damien Le Moal
Western Digital Research
