On Wed, 2012-10-31 at 09:16 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@xxxxxxxxxxxxxxx>
>
> Convert spc_emulate_modesense() to use a table of mode pages, rather
> than a switch statement. This makes it possible to add more pages
> sanely -- in particular we no longer need to make sure we keep the
> 0x3f (return all mode pages) case in sync.
>
> While we're touching this code, make our MODE SENSE emulation a bit
> better in a couple of ways:
> - When the initiator passes PC == 1 asking for changeable values,
> return all 0s to show we don't support setting anything.
> - Return a block descriptor for disk devices.
>
> Signed-off-by: Roland Dreier <roland@xxxxxxxxxxxxxxx>
> ---
> drivers/target/target_core_spc.c | 235 ++++++++++++++++++++++++++++----------
> 1 file changed, 176 insertions(+), 59 deletions(-)
>
<SNIP>
> static int spc_emulate_modesense(struct se_cmd *cmd)
> {
> struct se_device *dev = cmd->se_dev;
> char *cdb = cmd->t_task_cdb;
> - unsigned char *rbuf;
> + unsigned char *buf, *map_buf;
> int type = dev->transport->get_device_type(dev);
> int ten = (cmd->t_task_cdb[0] == MODE_SENSE_10);
> - u32 offset = ten ? 8 : 4;
> + bool dbd = !!(cdb[1] & 0x08);
> + bool llba = ten ? !!(cdb[1] & 0x10) : false;
> + u8 pc = cdb[2] >> 6;
> + u8 page = cdb[2] & 0x3f;
> + u8 subpage = cdb[3];
> int length = 0;
> - unsigned char buf[SE_MODE_PAGE_BUF];
> + int ret;
> + int i;
>
> - memset(buf, 0, SE_MODE_PAGE_BUF);
> + map_buf = transport_kmap_data_sg(cmd);
>
> - switch (cdb[2] & 0x3f) {
> - case 0x01:
> - length = spc_modesense_rwrecovery(&buf[offset]);
> - break;
> - case 0x08:
> - length = spc_modesense_caching(dev, &buf[offset]);
> - break;
> - case 0x0a:
> - length = spc_modesense_control(dev, &buf[offset]);
> - break;
> - case 0x3f:
> - length = spc_modesense_rwrecovery(&buf[offset]);
> - length += spc_modesense_caching(dev, &buf[offset+length]);
> - length += spc_modesense_control(dev, &buf[offset+length]);
> - break;
> - default:
> - pr_err("MODE SENSE: unimplemented page/subpage: 0x%02x/0x%02x\n",
> - cdb[2] & 0x3f, cdb[3]);
> - cmd->scsi_sense_reason = TCM_UNKNOWN_MODE_PAGE;
> - return -EINVAL;
> + /*
> + * If SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC is not set, then we
> + * know we actually allocated a full page. Otherwise, if the
> + * data buffer is too small, allocate a temporary buffer so we
> + * don't have to worry about overruns in all our INQUIRY
> + * emulation handling.
> + */
> + if (cmd->data_length < SE_MODE_PAGE_BUF &&
> + (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC)) {
> + buf = kzalloc(SE_MODE_PAGE_BUF, GFP_KERNEL);
> + if (!buf) {
> + transport_kunmap_data_sg(cmd);
> + cmd->scsi_sense_reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> + return -ENOMEM;
> + }
> + } else {
> + buf = map_buf;
> }
> - offset += length;
> -
> - if (ten) {
> - offset -= 2;
> - buf[0] = (offset >> 8) & 0xff;
> - buf[1] = offset & 0xff;
> - offset += 2;
> -
> - if ((cmd->se_lun->lun_access & TRANSPORT_LUNFLAGS_READ_ONLY) ||
> - (cmd->se_deve &&
> - (cmd->se_deve->lun_flags & TRANSPORT_LUNFLAGS_READ_ONLY)))
> - spc_modesense_write_protect(&buf[3], type);
> -
> - if ((dev->se_sub_dev->se_dev_attrib.emulate_write_cache > 0) &&
> - (dev->se_sub_dev->se_dev_attrib.emulate_fua_write > 0))
> - spc_modesense_dpofua(&buf[3], type);
> +
> + length = ten ? 2 : 1;
> +
The length (buffer offset) assignment here is off by one and causes
BLOCK DESCRIPTOR to be incorrectly written into the MEDIUM TYPE byte
area for 10 + 6 byte mode parameter headers according to SPC-4.
Here's an incremental patch that I'm applying into for-next that allows
sg_mode to work again.
I'll assume this is a untested bug, right..? Unless there is an
initiator that actually gets this wrong somehow..?
Roland..?
--nab
diff --git a/drivers/target/target_core_spc.c
b/drivers/target/target_core_spc.c
index 33022a3..c4ae3ad 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -871,7 +871,7 @@ static int spc_emulate_modesense(struct se_cmd *cmd)
buf = map_buf;
}
- length = ten ? 2 : 1;
+ length = ten ? 3 : 2;
/* DEVICE-SPECIFIC PARAMETER */
if ((cmd->se_lun->lun_access & TRANSPORT_LUNFLAGS_READ_ONLY) ||
--
sg_modes -vv --page=0x3f /dev/sdd
open /dev/sdd with flags=0x800
inquiry cdb: 12 00 00 00 24 00
LIO-ORG IBLOCK 4.0 peripheral_type: disk [0x0]
mode sense (10) cdb: 5a 00 3f 00 00 00 00 10 00 00
Mode parameter header from MODE SENSE(10):
Mode data length=72, medium type=0x00, WP=0, DpoFua=1, longlba=0
Block descriptor length=8
> Direct access device block descriptors:
Density code=0x0
00 09 50 f8 af 00 00 02 00
>> Read-Write error recovery, page_control: current
00 01 0a 00 00 00 00 00 00 00 00 00 00
>> Caching, page_control: current
00 08 12 04 00 00 00 00 00 00 00 00 00 20 00 00 00
10 00 00 00 00
>> Control, page_control: current
00 0a 0a 02 10 00 40 00 00 ff ff 00 1e
>> Informational exceptions control, page_control: current
00 1c 0a 00 00 00 00 00 00 00 00 00 00
--
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
