Il 06/09/2012 21:29, Nicholas A. Bellinger ha scritto:
> On Thu, 2012-09-06 at 17:13 +0200, Paolo Bonzini wrote:
>> This patch started with the aim of fixing START STOP UNIT to a PSCSI
>> device. Right now, commands with a zero-size payload are skipped
>> completely. This is wrong; such commands should be passed down to the
>> device and processed normally. As a hint of this, we have a hack to
>> clear a unit attention state on a zero-size REQUEST SENSE.
>
> The existing code for zero-size handling in transport_generic_new_cmd()
> is correct for virtual backends (eg: TRANSPORT_PLUGIN_VHBA_*), but as
> you've witnessed not correct for pSCSI passthrough ops.
It is not completely correct for virtual backends, for example I think
it will always return success for 0-block reads or writes, even if the
start LBA is out of range. This is also something that I saw with
PSCSI, and is fixed by these patches.
Also, even though it handles zero-size, it doesn't handle a CDB with a
small but nonzero allocation length. If you have such a CDB, you can
overflow the sglist. If the fabric uses
transport_generic_map_mem_to_cmd, you may corrupt adjacent memory.
Besides, the cut-and-pasted REQUEST SENSE handling is a bit gross. :)
Fixing this properly, as it turns out, also fixes zero-size handling of
PSCSI.
> The only cases where an pSCSI backend ever uses transport_kmap_data_sg()
> today are:
>
> - During REPORT_LUNS emulation
>
> - During the MODE_SENSE hack in pscsi_transport_complete() to set the
> proper WriteProtected bit based upon configfs fabric attribute
>
> so I'd rather see two special case checks for zero-size CDBs with pSCSI,
> over adding these changes to transport_k[un]map_data_sg().
This would not fix the root cause, which is bad handling of short-sized
allocation lengths.
>> Luckily, transport_kmap_data_sg is not called on the I/O path, so we can
>> simply allocate a one-page bounce buffer there, which indeed also takes
>> care of zero-sized transfers.
>> ---
>> drivers/target/target_core_transport.c | 62 ++++++++++++++-----------------
>> 1 files changed, 28 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>> index 09028af..a77c8aa 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -2181,20 +2181,32 @@ EXPORT_SYMBOL(transport_generic_map_mem_to_cmd);
>>
>> void *transport_kmap_data_sg(struct se_cmd *cmd)
>> {
>> + u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
>> struct scatterlist *sg = cmd->t_data_sg;
>> struct page **pages;
>> int i;
>>
>> - BUG_ON(!sg);
>> + BUG_ON(!sg && npages > 0);
>> +
>> /*
>> * We need to take into account a possible offset here for fabrics like
>> * tcm_loop who may be using a contig buffer from the SCSI midlayer for
>> * control CDBs passed as SGLs via transport_generic_map_mem_to_cmd()
>> + *
>> + * This could cause overflows if the buffer is too small for the caller.
>> + * For example, the REQUEST_SENSE handler expects 8 bytes, but it is
>> + * possible to send a CDB with a small allocation length (e.g. 4 bytes).
>> + * In this case, we could have a single-page sglist with a large offset,
>> + * so that buf[7] is already inaccessible.
>> + *
>> + * But transport_kmap_data_sg is not called on the I/O path, so we can
>> + * simply allocate a one-page bounce buffer here. This also takes care
>> + * of the case of zero-sized transfers.
>> */
>> - if (!cmd->t_data_nents)
>> - return NULL;
>> - else if (cmd->t_data_nents == 1)
>> - return kmap(sg_page(sg)) + sg->offset;
>> + if (npages <= 1) {
>> + cmd->t_data_vmap = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> + return cmd->t_data_vmap;
>> + }
>>
>> /* >1 page. use vmap */
>> pages = kmalloc(sizeof(*pages) * cmd->t_data_nents, GFP_KERNEL);
>> @@ -2217,14 +2229,18 @@ EXPORT_SYMBOL(transport_kmap_data_sg);
>>
>> void transport_kunmap_data_sg(struct se_cmd *cmd)
>> {
>> - if (!cmd->t_data_nents) {
>> - return;
>> - } else if (cmd->t_data_nents == 1) {
>> - kunmap(sg_page(cmd->t_data_sg));
>> - return;
>> - }
>> + u32 npages = DIV_ROUND_UP(cmd->data_length, PAGE_SIZE);
>> + if (npages <= 1) {
>> + if (npages) {
>> + struct scatterlist *sg = cmd->t_data_sg;
>> + u8 *dest = kmap(sg_page(sg));
>> + memcpy(dest + sg->offset, cmd->t_data_vmap, sg->length);
>> + kunmap(sg_page(sg));
>> + }
>> + kfree(cmd->t_data_vmap);
>> + } else
>> + vunmap(cmd->t_data_vmap);
>>
>> - vunmap(cmd->t_data_vmap);
>> cmd->t_data_vmap = NULL;
>> }
>> EXPORT_SYMBOL(transport_kunmap_data_sg);
>> @@ -2290,28 +2306,6 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
>> if (ret < 0)
>> goto out_fail;
>> }
>> - /*
>> - * If this command doesn't have any payload and we don't have to call
>> - * into the fabric for data transfers, go ahead and complete it right
>> - * away.
>> - */
>> - if (!cmd->data_length) {
>> - spin_lock_irq(&cmd->t_state_lock);
>> - cmd->t_state = TRANSPORT_COMPLETE;
>> - cmd->transport_state |= CMD_T_ACTIVE;
>> - spin_unlock_irq(&cmd->t_state_lock);
>> -
>> - if (cmd->t_task_cdb[0] == REQUEST_SENSE) {
>> - u8 ua_asc = 0, ua_ascq = 0;
>> -
>> - core_scsi3_ua_clear_for_request_sense(cmd,
>> - &ua_asc, &ua_ascq);
>> - }
>> -
>> - INIT_WORK(&cmd->work, target_complete_ok_work);
>> - queue_work(target_completion_wq, &cmd->work);
>> - return 0;
>> - }
>>
>
> This code needs still needs to get called for all virtual backends of
> type !TRANSPORT_PLUGIN_PHBA_PDEV.
It is superseded by the new code in transport_kmap_data_sg.
Paolo
--
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
