On 06/09/2011 12:30 AM, Christoph Hellwig wrote:
>> +void *transport_kmap_first_data_page(struct se_cmd *cmd)
>> +{
>> + struct se_mem *se_mem;
>> +
>> + BUG_ON(list_empty(&cmd->t_mem_list));
>> +
>> + se_mem = list_first_entry(&cmd->t_mem_list, struct se_mem, se_list);
>> +
>> + /*
>> + * 1st se_mem should point to a page, and we shouldn't need more than
>> + * that for this cmd
>> + */
>> + BUG_ON(cmd->data_length > PAGE_SIZE);
>
> Do we actually verify these conditions earlier on? Given that these
> are user controlled we should reject commands that we can't accept early
> on. We'll also need to make sure that data never straddles a page,
> which is something initiators could do by careful placement. I don't
> think we have to support that as no sane initiator would do it, but
> we'll need to check for, and reject it.
? I don't quite follow. Let's use handling REPORT LUNS
(target_core_device.c transport_core_report_lun_response()) as an example.
First, I don't see in the spec where the REPORT LUNS data-in allocation
length is limited. We currently limit to TRANSPORT_MAX_LUNS_PER_TPG
(256) but if this is innocently raised beyond 512, each being 8 bytes,
then the 4k page will be insufficient and it will break. Should we base
MAX_LUNS on our allocation size?
Second, we are allocating the pages, so how could straddling two pages
be an issue? We're not sharing any buffers with the initiator, only the
fabric, potentially. But what about the tcm_loop? I have a hunch all
these changes will have broken it, but maybe we fix it up at the end.
> Alternatively the command emulation could have local buffers and copy
> them to the S/G list, which works fine especially for commands with
> small output that can be on the stack.
Hmm, so this would be pretty close to keeping t_task_buf around
internally, but making sure that fabrics & backstores only dealt with
S/G lists that mapped to the kmalloced t_task_buf?
>> @@ -4758,7 +4720,7 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
>> * cmd->t_mem_list of struct se_mem->se_page
>> */
>> if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC)) {
>> - ret = transport_allocate_resources(cmd);
>> + ret = transport_generic_get_mem(cmd);
>
> This doesn't look correct. We only used to call
> transport_generic_get_mem when SCF_SCSI_DATA_SG_IO_CDB or
> SCF_SCSI_CONTROL_SG_IO_CDB where set here, and now we do it
> undconditionally. Given that we always set
> SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC for these the code does in fact seem
> dead.
iscsi always sets TO_MEM_NOALLOC, but that's because it calls
map_mem_to_cmd. Other fabrics still rely on the core to alloc memory.
Regarding the above code, so the 4 cases *were* DATA_SG, CONTROL_SG,
CONTROL_NONSG, and NON_DATA. This patch eliminated CONTROL_NONSG, the
first two already called get_mem, and it's safe to call it for NON_DATA
because data_length is 0 and transport_generic_get_mem will just return
success, so I think this is ok.
>> @@ -108,7 +108,6 @@ enum se_cmd_flags_table {
>> SCF_EMULATED_TASK_SENSE = 0x00000004,
>> SCF_SCSI_DATA_SG_IO_CDB = 0x00000008,
>> SCF_SCSI_CONTROL_SG_IO_CDB = 0x00000010,
>> - SCF_SCSI_CONTROL_NONSG_IO_CDB = 0x00000020,
>> SCF_SCSI_NON_DATA_CDB = 0x00000040,
>> SCF_SCSI_CDB_EXCEPTION = 0x00000080,
>> SCF_SCSI_RESERVATION_CONFLICT = 0x00000100,
>
> SCF_PASSTHROUGH_CONTIG_TO_SG should also be dead after your patch.
Aha, ok.
>> int (*cdb_none)(struct se_task *);
>> /*
>> - * For SCF_SCSI_CONTROL_NONSG_IO_CDB
>> - */
>> - int (*map_task_non_SG)(struct se_task *);
>> - /*
>> * For SCF_SCSI_DATA_SG_IO_CDB and SCF_SCSI_CONTROL_SG_IO_CDB
>> */
>> int (*map_task_SG)(struct se_task *);
>
> As a follow-on cdb_none should probably also go away and just be treated
> as a NULL-S/G list special case in map_task_SG.
Sounds good!
Thanks -- Regards -- Andy
--
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
