On 04/27/2017 05:29 PM, Bart Van Assche wrote:
> On Thu, 2017-04-27 at 16:51 -0600, Jens Axboe wrote:
>> @@ -1114,10 +1121,16 @@ static int mtip_exec_internal_command(struct
>> mtip_port *port,
>> u32 opts,
>> unsigned long timeout)
>> {
>> - struct mtip_cmd_sg *command_sg;
>> DECLARE_COMPLETION_ONSTACK(wait);
>> struct mtip_cmd *int_cmd;
>> struct driver_data *dd = port->dd;
>> + struct request *rq;
>> + struct mtip_int_cmd icmd = {
>> + .fis_len = fis_len,
>> + .buffer = buffer,
>> + .buf_len = buf_len,
>> + .opts = opts
>> + };
>> int rv = 0;
>> unsigned long start;
>>
>> @@ -1132,6 +1145,8 @@ static int mtip_exec_internal_command(struct mtip_port
>> *port,
>> dbg_printk(MTIP_DRV_NAME "Unable to allocate tag for PIO
>> cmd\n");
>> return -EFAULT;
>> }
>> + rq = blk_mq_rq_from_pdu(int_cmd);
>> + rq->end_io_data = &icmd;
>>
>> set_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags);
>>
>> @@ -1158,30 +1173,10 @@ static int mtip_exec_internal_command(struct
>> mtip_port *port,
>> /* Copy the command to the command table */
>> memcpy(int_cmd->command, fis, fis_len*4);
>>
>> - /* Populate the SG list */
>> - int_cmd->command_header->opts =
>> - __force_bit2int cpu_to_le32(opts | fis_len);
>> - if (buf_len) {
>> - command_sg = int_cmd->command + AHCI_CMD_TBL_HDR_SZ;
>> -
>> - command_sg->info =
>> - __force_bit2int cpu_to_le32((buf_len-1) & 0x3FFFFF);
>> - command_sg->dba =
>> - __force_bit2int cpu_to_le32(buffer & 0xFFFFFFFF);
>> - command_sg->dba_upper =
>> - __force_bit2int cpu_to_le32((buffer >> 16) >> 16);
>> -
>> - int_cmd->command_header->opts |=
>> - __force_bit2int cpu_to_le32((1 << 16));
>> - }
>> -
>> - /* Populate the command header */
>> - int_cmd->command_header->byte_count = 0;
>> -
>> start = jiffies;
>>
>> - /* Issue the command to the hardware */
>> - mtip_issue_non_ncq_command(port, MTIP_TAG_INTERNAL);
>> + /* insert request and run queue */
>> + blk_execute_rq_nowait(rq->q, NULL, rq, true, NULL);
>>
>> /* Wait for the command to complete or timeout. */
>> rv = wait_for_completion_interruptible_timeout(&wait,
>
> Hello Jens,
>
> What will happen upon timeout? Will the end_io_data pointer be dereferenced if
> a timeout occurs? Can that cause the completion function to access data on the
> stack after it has been freed?
Good point - since we know get timeouts courtesy of blk-mq, I will kill
this individual timeout to avoid having any races there.
--
Jens Axboe
