Skip to contentSkip to conversation

scsi: hisi_sas: add internal abort to hisi_sas_abort_task()

bug report Dan Carpenter 2 participants 2 messages Oct 12, 2016 SCSI

lore
Share & tools
lore
#0 lore raw

Hello John Garry,

The patch dc8a49cabc73: "scsi: hisi_sas: add internal abort to hisi_sas_abort_task()" from Aug 24, 2016, leads to the following static checker warning:

	drivers/scsi/hisi_sas/hisi_sas_main.c:848 hisi_sas_abort_task()
	error: we previously assumed 'slot' could be null (see line 847)

drivers/scsi/hisi_sas/hisi_sas_main.c

   809          spin_unlock_irqrestore(&task->task_state_lock, flags);
   810          sas_dev->dev_status = HISI_SAS_DEV_EH;
   811          if (task->lldd_task && task->task_proto & SAS_PROTOCOL_SSP) {
                    ^^^^^^^^^^^^^^^

We assume that ->lldd_task can be NULL.

   812                  struct scsi_cmnd *cmnd = task->uldd_task;
   813                  struct hisi_sas_slot *slot = task->lldd_task;
   814                  u32 tag = slot->idx;
   815  
   816                  int_to_scsilun(cmnd->device->lun, &lun);
   817                  tmf_task.tmf = TMF_ABORT_TASK;
   818                  tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
   819  
   820                  rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun,
   821                                                    &tmf_task);
   822  
   823                  /* if successful, clear the task and callback forwards.*/
   824                  if (rc == TMF_RESP_FUNC_COMPLETE) {
   825                          if (task->lldd_task) {
   826                                  struct hisi_sas_slot *slot;
   827  
   828                                  slot = &hisi_hba->slot_info
   829                                          [tmf_task.tag_of_task_to_be_managed];
   830                                  spin_lock_irqsave(&hisi_hba->lock, flags);
   831                                  hisi_hba->hw->slot_complete(hisi_hba, slot, 1);
   832                                  spin_unlock_irqrestore(&hisi_hba->lock, flags);

833 }
834 }
835
836 hisi_sas_internal_task_abort(hisi_hba, device,
837 HISI_SAS_INT_ABT_CMD, tag);
838 } else if (task->task_proto & SAS_PROTOCOL_SATA ||
839 task->task_proto & SAS_PROTOCOL_STP) {
840 if (task->dev->dev_type == SAS_SATA_DEV) {
841 hisi_sas_internal_task_abort(hisi_hba, device,
842 HISI_SAS_INT_ABT_DEV, 0);
843 rc = TMF_RESP_FUNC_COMPLETE;
844 }
845 } else if (task->task_proto & SAS_PROTOCOL_SMP) {
846 /* SMP */
847 struct hisi_sas_slot *slot = task->lldd_task;

We assign it to slot.

   848                  u32 tag = slot->idx;
                                  ^^^^^^^^^

slot dereferenced without checking.

849
850 hisi_sas_internal_task_abort(hisi_hba, device,
851 HISI_SAS_INT_ABT_CMD, tag);
852 }

regards,
dan carpenter

#1 lore raw
Quoted 53 lines · Dan Carpenter

Hello John Garry,

The patch dc8a49cabc73: "scsi: hisi_sas: add internal abort to hisi_sas_abort_task()" from Aug 24, 2016, leads to the following static checker warning:

	drivers/scsi/hisi_sas/hisi_sas_main.c:848 hisi_sas_abort_task()
	error: we previously assumed 'slot' could be null (see line 847)

drivers/scsi/hisi_sas/hisi_sas_main.c

    809          spin_unlock_irqrestore(&task->task_state_lock, flags);
    810          sas_dev->dev_status = HISI_SAS_DEV_EH;
    811          if (task->lldd_task && task->task_proto & SAS_PROTOCOL_SSP) {
                     ^^^^^^^^^^^^^^^

We assume that ->lldd_task can be NULL.

    812                  struct scsi_cmnd *cmnd = task->uldd_task;
    813                  struct hisi_sas_slot *slot = task->lldd_task;
    814                  u32 tag = slot->idx;
    815
    816                  int_to_scsilun(cmnd->device->lun, &lun);
    817                  tmf_task.tmf = TMF_ABORT_TASK;
    818                  tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
    819
    820                  rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun,
    821                                                    &tmf_task);
    822
    823                  /* if successful, clear the task and callback forwards.*/
    824                  if (rc == TMF_RESP_FUNC_COMPLETE) {
    825                          if (task->lldd_task) {
    826                                  struct hisi_sas_slot *slot;
    827
    828                                  slot = &hisi_hba->slot_info
    829                                          [tmf_task.tag_of_task_to_be_managed];
    830                                  spin_lock_irqsave(&hisi_hba->lock, flags);
    831                                  hisi_hba->hw->slot_complete(hisi_hba, slot, 1);
    832                                  spin_unlock_irqrestore(&hisi_hba->lock, flags);
    833                          }
    834                  }
    835
    836                  hisi_sas_internal_task_abort(hisi_hba, device,
    837                                               HISI_SAS_INT_ABT_CMD, tag);
    838          } else if (task->task_proto & SAS_PROTOCOL_SATA ||
    839                  task->task_proto & SAS_PROTOCOL_STP) {
    840                  if (task->dev->dev_type == SAS_SATA_DEV) {
    841                          hisi_sas_internal_task_abort(hisi_hba, device,
    842                                                       HISI_SAS_INT_ABT_DEV, 0);
    843                          rc = TMF_RESP_FUNC_COMPLETE;
    844                  }
    845          } else if (task->task_proto & SAS_PROTOCOL_SMP) {
    846                  /* SMP */
    847                  struct hisi_sas_slot *slot = task->lldd_task;
Quoted 1 line

We assign it to slot.

I will check this.

Thanks,
John

Quoted 17 lines
    848                  u32 tag = slot->idx;
                                   ^^^^^^^^^

slot dereferenced without checking.

    849
    850                  hisi_sas_internal_task_abort(hisi_hba, device,
    851                                               HISI_SAS_INT_ABT_CMD, tag);
    852          }

regards,
dan carpenter

.