Re: [PATCH] SCSI: buslogic: Added check for DMA mapping errors (wasRe:[BusLogic] DMA-API: device driver failed to check map error)

From: Khalid Aziz
Date: Fri Sep 13 2013 - 11:55:32 EST


On 09/13/2013 09:42 AM, Tetsuo Handa wrote:
Khalid Aziz wrote:
Added check for DMA mapping errors for request sense data
buffer. Checking for mapping error can avoid potential wild
writes. This patch was prompted by the warning from
dma_unmap when kernel is compiled with CONFIG_DMA_API_DEBUG.

This patch fixes CONFIG_DMA_API_DEBUG warning.
But excuse me, is this error path correct?

@@ -309,16 +309,17 @@ static struct blogic_ccb *blogic_alloc_ccb(struct blogic_adapter *adapter)
blogic_dealloc_ccb deallocates a CCB, returning it to the Host Adapter's
free list. The Host Adapter's Lock should already have been acquired by the
caller.
*/

-static void blogic_dealloc_ccb(struct blogic_ccb *ccb)
+static void blogic_dealloc_ccb(struct blogic_ccb *ccb, int dma_unmap)
{
struct blogic_adapter *adapter = ccb->adapter;

scsi_dma_unmap(ccb->command);

blogic_dealloc_ccb() uses "ccb->command". But

- pci_unmap_single(adapter->pci_device, ccb->sensedata,
+ if (dma_unmap)
+ pci_unmap_single(adapter->pci_device, ccb->sensedata,
ccb->sense_datalen, PCI_DMA_FROMDEVICE);

ccb->command = NULL;
ccb->status = BLOGIC_CCB_FREE;
ccb->next = adapter->free_ccbs;
@@ -3177,13 +3179,21 @@ static int blogic_qcmd_lck(struct scsi_cmnd *command,
ccb->legacy_tag = queuetag;
}
}
memcpy(ccb->cdb, cdb, cdblen);
ccb->sense_datalen = SCSI_SENSE_BUFFERSIZE;
- ccb->sensedata = pci_map_single(adapter->pci_device,
+ sense_buf = pci_map_single(adapter->pci_device,
command->sense_buffer, ccb->sense_datalen,
PCI_DMA_FROMDEVICE);
+ if (dma_mapping_error(&adapter->pci_device->dev, sense_buf)) {
+ blogic_err("DMA mapping for sense data buffer failed\n",
+ adapter);
+ scsi_dma_map(command);
+ blogic_dealloc_ccb(ccb, 0);

when was "ccb->command = command;" called before calling blogic_dealloc_ccb()?

+ return SCSI_MLQUEUE_HOST_BUSY;
+ }
+ ccb->sensedata = sense_buf;
ccb->command = command;
command->scsi_done = comp_cb;
if (blogic_multimaster_type(adapter)) {
/*
Place the CCB in an Outgoing Mailbox. The higher levels

Also, what happens if "scsi_dma_map(command);" returned -ENOMEM ?
If you are calling scsi_dma_map() because blogic_dealloc_ccb() calls
scsi_dma_unmap(), why can't we do like

{
struct blogic_adapter *adapter = ccb->adapter;
ccb->command = NULL;
ccb->status = BLOGIC_CCB_FREE;
ccb->next = adapter->free_ccbs;
adapter->free_ccbs = ccb;
}

instead of

scsi_dma_map(command);
blogic_dealloc_ccb(ccb);

?


That is a typo. I was going to call just scsi_dma_unmap(), had copied down the previous down the previous scsi_dma_map() line, read the code further and decided I needed to call blogic_dealloc_ccb() instead so we don't leak CCBs, added the call to blogic_dealloc_ccb() and forgot to delete the previously added and unfinished line. scsi_dma_map() had already been called earlier which is why scsi_dma_unmap() needs to be called which is taken care of by blogic_dealloc_ccb().

I need to delete the call to scsi_dma_map() which was not supposed to be there and move "ccb->command = command;" up before the call to dma_mapping_error(). I will send out an updated patch.

Good catch. Thanks!

--
Khalid
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/