Skip to contentSkip to conversation

Fix IOMMU overflow checking in s2io.c

PATCH Applied Andi Kleen 3 participants 3 messages Jun 18, 2008 – Jun 27, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Fix IOMMU overflow checking in s2io.c

Fix IOMMU overflow checking in s2io.c

s2io has IOMMU overflow checking, but unfortunately it is wrong.

It didn't use the standard macros, which meant that it only worked on POWER and SPARC because only those define DMA_ERROR_CODE. Convert it to use the standard macros instead.

I also commented two more bugs in the IOMMU handling. It assumes that 0 DMA addresses cannot happen, but that's not true in all IOMMU setups. The information if a buffer has been already mapped needs to be stored elsewhere.

Didn't fix those because it needs careful checking of the buffer handling by the maintainers.

Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by Andi Kleen <[email protected]>
drivers/net/s2io.c +12 -23
Index: linux/drivers/net/s2io.c
===================================================================
@@ -2625,9 +2625,7 @@ static int fill_rx_buffers(struct ring_i
 			rxdp1->Buffer0_ptr = pci_map_single
 			    (ring->pdev, skb->data, size - NET_IP_ALIGN,
 				PCI_DMA_FROMDEVICE);
-			if( (rxdp1->Buffer0_ptr == 0) ||
-				(rxdp1->Buffer0_ptr ==
-				DMA_ERROR_CODE))
+			if(pci_dma_mapping_error(rxdp1->Buffer0_ptr))
 				goto pci_map_failed;
 
 			rxdp->Control_2 =

@@ -2657,6 +2655,7 @@ static int fill_rx_buffers(struct ring_i
 			skb->data = (void *) (unsigned long)tmp;
 			skb_reset_tail_pointer(skb);
 
+			/* AK: check is wrong. 0 can be valid dma address */
 			if (!(rxdp3->Buffer0_ptr))
 				rxdp3->Buffer0_ptr =
 				   pci_map_single(ring->pdev, ba->ba_0,

@@ -2665,8 +2664,7 @@ static int fill_rx_buffers(struct ring_i
 				pci_dma_sync_single_for_device(ring->pdev,
 				(dma_addr_t) rxdp3->Buffer0_ptr,
 				    BUF0_LEN, PCI_DMA_FROMDEVICE);
-			if( (rxdp3->Buffer0_ptr == 0) ||
-				(rxdp3->Buffer0_ptr == DMA_ERROR_CODE))
+			if (pci_dma_mapping_error(rxdp3->Buffer0_ptr))
 				goto pci_map_failed;
 
 			rxdp->Control_2 = SET_BUFFER0_SIZE_3(BUF0_LEN);

@@ -2681,18 +2679,17 @@ static int fill_rx_buffers(struct ring_i
 				(ring->pdev, skb->data, ring->mtu + 4,
 						PCI_DMA_FROMDEVICE);
 
-				if( (rxdp3->Buffer2_ptr == 0) ||
-					(rxdp3->Buffer2_ptr == DMA_ERROR_CODE))
+				if (pci_dma_mapping_error(rxdp3->Buffer2_ptr))
 					goto pci_map_failed;
 
+				/* AK: check is wrong */
 				if (!rxdp3->Buffer1_ptr)
 					rxdp3->Buffer1_ptr =
 						pci_map_single(ring->pdev,
 						ba->ba_1, BUF1_LEN,
 						PCI_DMA_FROMDEVICE);
 
-				if( (rxdp3->Buffer1_ptr == 0) ||
-					(rxdp3->Buffer1_ptr == DMA_ERROR_CODE)) {
+				if (pci_dma_mapping_error(rxdp3->Buffer1_ptr)) {
 					pci_unmap_single
 						(ring->pdev,
 						(dma_addr_t)(unsigned long)

@@ -4264,16 +4261,14 @@ static int s2io_xmit(struct sk_buff *skb
 		txdp->Buffer_Pointer = pci_map_single(sp->pdev,
 					fifo->ufo_in_band_v,
 					sizeof(u64), PCI_DMA_TODEVICE);
-		if((txdp->Buffer_Pointer == 0) ||
-			(txdp->Buffer_Pointer == DMA_ERROR_CODE))
+		if (pci_dma_mapping_error(txdp->Buffer_Pointer))
 			goto pci_map_failed;
 		txdp++;
 	}
 
 	txdp->Buffer_Pointer = pci_map_single
 	    (sp->pdev, skb->data, frg_len, PCI_DMA_TODEVICE);
-	if((txdp->Buffer_Pointer == 0) ||
-		(txdp->Buffer_Pointer == DMA_ERROR_CODE))
+	if (pci_dma_mapping_error(txdp->Buffer_Pointer))
 		goto pci_map_failed;
 
 	txdp->Host_Control = (unsigned long) skb;

@@ -6884,10 +6879,8 @@ static int set_rxd_buffer_pointer(struct
 				pci_map_single( sp->pdev, (*skb)->data,
 					size - NET_IP_ALIGN,
 					PCI_DMA_FROMDEVICE);
-			if( (rxdp1->Buffer0_ptr == 0) ||
-				(rxdp1->Buffer0_ptr == DMA_ERROR_CODE)) {
+			if (pci_dma_mapping_error(rxdp1->Buffer0_ptr))
 				goto memalloc_failed;
-			}
 			rxdp->Host_Control = (unsigned long) (*skb);
 		}
 	} else if ((sp->rxd_mode == RXD_MODE_3B) && (rxdp->Host_Control == 0)) {

@@ -6913,15 +6906,12 @@ static int set_rxd_buffer_pointer(struct
 				pci_map_single(sp->pdev, (*skb)->data,
 					       dev->mtu + 4,
 					       PCI_DMA_FROMDEVICE);
-			if( (rxdp3->Buffer2_ptr == 0) ||
-				(rxdp3->Buffer2_ptr == DMA_ERROR_CODE)) {
+			if (pci_dma_mapping_error(rxdp3->Buffer2_ptr))
 				goto memalloc_failed;
-			}
 			rxdp3->Buffer0_ptr = *temp0 =
 				pci_map_single( sp->pdev, ba->ba_0, BUF0_LEN,
 						PCI_DMA_FROMDEVICE);
-			if( (rxdp3->Buffer0_ptr == 0) ||
-				(rxdp3->Buffer0_ptr == DMA_ERROR_CODE)) {
+			if (pci_dma_mapping_error(rxdp3->Buffer0_ptr)) {
 				pci_unmap_single (sp->pdev,
 					(dma_addr_t)rxdp3->Buffer2_ptr,
 					dev->mtu + 4, PCI_DMA_FROMDEVICE);

@@ -6933,8 +6923,7 @@ static int set_rxd_buffer_pointer(struct
 			rxdp3->Buffer1_ptr = *temp1 =
 				pci_map_single(sp->pdev, ba->ba_1, BUF1_LEN,
 						PCI_DMA_FROMDEVICE);
-			if( (rxdp3->Buffer1_ptr == 0) ||
-				(rxdp3->Buffer1_ptr == DMA_ERROR_CODE)) {
+			if (pci_dma_mapping_error(rxdp3->Buffer1_ptr)) {
 				pci_unmap_single (sp->pdev,
 					(dma_addr_t)rxdp3->Buffer0_ptr,
 					BUF0_LEN, PCI_DMA_FROMDEVICE);
drivers/net/s2io.h +0 -4
Index: linux/drivers/net/s2io.h
===================================================================
@@ -75,10 +75,6 @@ static int debug_level = ERR_DBG;
 /* DEBUG message print. */
 #define DBG_PRINT(dbg_level, args...)  if(!(debug_level<dbg_level)) printk(args)
 
-#ifndef DMA_ERROR_CODE
-#define DMA_ERROR_CODE          (~(dma_addr_t)0x0)
-#endif
-
 /* Protocol assist features of the NIC */
 #define L3_CKSUM_OK 0xFFFF
 #define L4_CKSUM_OK 0xFFFF
#1 lore raw

Andi,

Thanks for this fix. We will recreate a new patch which includes the fix mentioned in your comments and resubmit.

Ram

Quoted 7 lines

-----Original Message-----
From: Andi Kleen [mailto:[email protected]]
Sent: Wednesday, June 18, 2008 4:59 AM
To: [email protected]; [email protected]; [email protected]; Rastapur Santosh; Sivakumar Subramani; Sreenivasa Honnur
Subject: [PATCH] Fix IOMMU overflow checking in s2io.c

Quoted 6 lines

Fix IOMMU overflow checking in s2io.c

s2io has IOMMU overflow checking, but unfortunately it is wrong.

It didn't use the standard macros, which meant that it only worked on POWER and SPARC because only those define DMA_ERROR_CODE. Convert

it to

Quoted 9 lines

use the standard macros instead.

I also commented two more bugs in the IOMMU handling. It assumes that 0 DMA addresses cannot happen, but that's not true in all IOMMU setups.
The information if a buffer has been already mapped needs to be stored elsewhere.

Didn't fix those because it needs careful checking of the buffer

handling

Quoted 25 lines

by the maintainers.

Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by Andi Kleen <[email protected]>

Index: linux/drivers/net/s2io.c
===================================================================

drivers/net/s2io.c +1 -3
@@ -2625,9 +2625,7 @@ static int fill_rx_buffers(struct ring_i
 			rxdp1->Buffer0_ptr = pci_map_single
 			    (ring->pdev, skb->data, size - NET_IP_ALIGN,
 				PCI_DMA_FROMDEVICE);
-			if( (rxdp1->Buffer0_ptr == 0) ||
-				(rxdp1->Buffer0_ptr ==
-				DMA_ERROR_CODE))
+			if(pci_dma_mapping_error(rxdp1->Buffer0_ptr))
 				goto pci_map_failed;
 
 			rxdp->Control_2 =

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i · line 2658

Quoted 4 lines
drivers/net/s2io.c +1 -0
@@ -2657,6 +2655,7 @@ static int fill_rx_buffers(struct ring_i
 			skb->data = (void *) (unsigned long)tmp;
 			skb_reset_tail_pointer(skb);
 
+			/* AK: check is wrong. 0 can be valid dma

address */

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i · line 2664

Quoted 5 lines
drivers/net/s2io.c +0 -0
 			if (!(rxdp3->Buffer0_ptr))
 				rxdp3->Buffer0_ptr =
 				   pci_map_single(ring->pdev, ba->ba_0,

@@ -2665,8 +2664,7 @@ static int fill_rx_buffers(struct ring_i

pci_dma_sync_single_for_device(ring->pdev,

Quoted 9 lines
drivers/net/s2io.c +1 -2
 				(dma_addr_t) rxdp3->Buffer0_ptr,
 				    BUF0_LEN, PCI_DMA_FROMDEVICE);
-			if( (rxdp3->Buffer0_ptr == 0) ||
-				(rxdp3->Buffer0_ptr == DMA_ERROR_CODE))
+			if (pci_dma_mapping_error(rxdp3->Buffer0_ptr))
 				goto pci_map_failed;
 
 			rxdp->Control_2 = SET_BUFFER0_SIZE_3(BUF0_LEN);

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i · line 2685

Quoted 5 lines
drivers/net/s2io.c +0 -2
@@ -2681,18 +2679,17 @@ static int fill_rx_buffers(struct ring_i
 				(ring->pdev, skb->data, ring->mtu + 4,
 						PCI_DMA_FROMDEVICE);
 
-				if( (rxdp3->Buffer2_ptr == 0) ||
-					(rxdp3->Buffer2_ptr ==

DMA_ERROR_CODE))

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i · line 2682

Quoted 1 line
drivers/net/s2io.c +1 -0
+				if

(pci_dma_mapping_error(rxdp3->Buffer2_ptr))

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i · line 2691

Quoted 6 lines
drivers/net/s2io.c +1 -0
 					goto pci_map_failed;
 
+				/* AK: check is wrong */
 				if (!rxdp3->Buffer1_ptr)
 					rxdp3->Buffer1_ptr =

pci_map_single(ring->pdev,

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i

Quoted 5 lines
drivers/net/s2io.c +0 -2
 						ba->ba_1, BUF1_LEN,
 						PCI_DMA_FROMDEVICE);
 
-				if( (rxdp3->Buffer1_ptr == 0) ||
-					(rxdp3->Buffer1_ptr ==

DMA_ERROR_CODE)) {

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i · line 2692

Quoted 1 line
drivers/net/s2io.c +1 -0
+				if

(pci_dma_mapping_error(rxdp3->Buffer1_ptr)) {

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i

Quoted 3 lines
drivers/net/s2io.c +0 -0
 					pci_unmap_single
 						(ring->pdev,
 						(dma_addr_t)(unsigned

long)

Quoted 20 lines
drivers/net/s2io.c +2 -4
@@ -4264,16 +4261,14 @@ static int s2io_xmit(struct sk_buff *skb
 		txdp->Buffer_Pointer = pci_map_single(sp->pdev,
 					fifo->ufo_in_band_v,
 					sizeof(u64), PCI_DMA_TODEVICE);
-		if((txdp->Buffer_Pointer == 0) ||
-			(txdp->Buffer_Pointer == DMA_ERROR_CODE))
+		if (pci_dma_mapping_error(txdp->Buffer_Pointer))
 			goto pci_map_failed;
 		txdp++;
 	}
 
 	txdp->Buffer_Pointer = pci_map_single
 	    (sp->pdev, skb->data, frg_len, PCI_DMA_TODEVICE);
-	if((txdp->Buffer_Pointer == 0) ||
-		(txdp->Buffer_Pointer == DMA_ERROR_CODE))
+	if (pci_dma_mapping_error(txdp->Buffer_Pointer))
 		goto pci_map_failed;
 
 	txdp->Host_Control = (unsigned long) skb;

drivers/net/s2io.c static int set_rxd_buffer_pointer(struct · line 6888

Quoted 5 lines
drivers/net/s2io.c +0 -2
@@ -6884,10 +6879,8 @@ static int set_rxd_buffer_pointer(struct
 				pci_map_single( sp->pdev, (*skb)->data,
 					size - NET_IP_ALIGN,
 					PCI_DMA_FROMDEVICE);
-			if( (rxdp1->Buffer0_ptr == 0) ||
-				(rxdp1->Buffer0_ptr == DMA_ERROR_CODE))

{

drivers/net/s2io.c static int set_rxd_buffer_pointer(struct

Quoted 6 lines
drivers/net/s2io.c +1 -1
+			if (pci_dma_mapping_error(rxdp1->Buffer0_ptr))
 				goto memalloc_failed;
-			}
 			rxdp->Host_Control = (unsigned long) (*skb);
 		}
 	} else if ((sp->rxd_mode == RXD_MODE_3B) && (rxdp->Host_Control

==

drivers/net/s2io.c static int set_rxd_buffer_pointer(struct · line 6917

Quoted 7 lines

0)) {

drivers/net/s2io.c +0 -2
@@ -6913,15 +6906,12 @@ static int set_rxd_buffer_pointer(struct
 				pci_map_single(sp->pdev, (*skb)->data,
 					       dev->mtu + 4,
 					       PCI_DMA_FROMDEVICE);
-			if( (rxdp3->Buffer2_ptr == 0) ||
-				(rxdp3->Buffer2_ptr == DMA_ERROR_CODE))

{

drivers/net/s2io.c static int set_rxd_buffer_pointer(struct

Quoted 5 lines
drivers/net/s2io.c +1 -1
+			if (pci_dma_mapping_error(rxdp3->Buffer2_ptr))
 				goto memalloc_failed;
-			}
 			rxdp3->Buffer0_ptr = *temp0 =
 				pci_map_single( sp->pdev, ba->ba_0,

BUF0_LEN,

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i · line 2669

Quoted 3 lines
drivers/net/s2io.c +0 -2
 						PCI_DMA_FROMDEVICE);
-			if( (rxdp3->Buffer0_ptr == 0) ||
-				(rxdp3->Buffer0_ptr == DMA_ERROR_CODE))

{

drivers/net/s2io.c static int set_rxd_buffer_pointer(struct

Quoted 4 lines
drivers/net/s2io.c +1 -0
+			if (pci_dma_mapping_error(rxdp3->Buffer0_ptr)) {
 				pci_unmap_single (sp->pdev,
 					(dma_addr_t)rxdp3->Buffer2_ptr,
 					dev->mtu + 4,

PCI_DMA_FROMDEVICE);

drivers/net/s2io.c static int set_rxd_buffer_pointer(struct · line 6924

Quoted 3 lines
drivers/net/s2io.c +0 -0
@@ -6933,8 +6923,7 @@ static int set_rxd_buffer_pointer(struct
 			rxdp3->Buffer1_ptr = *temp1 =
 				pci_map_single(sp->pdev, ba->ba_1,

BUF1_LEN,

drivers/net/s2io.c static int fill_rx_buffers(struct ring_i

Quoted 3 lines
drivers/net/s2io.c +0 -2
 						PCI_DMA_FROMDEVICE);
-			if( (rxdp3->Buffer1_ptr == 0) ||
-				(rxdp3->Buffer1_ptr == DMA_ERROR_CODE))

{

Quoted 20 lines
drivers/net/s2io.c +1 -0
+			if (pci_dma_mapping_error(rxdp3->Buffer1_ptr)) {
 				pci_unmap_single (sp->pdev,
 					(dma_addr_t)rxdp3->Buffer0_ptr,
 					BUF0_LEN, PCI_DMA_FROMDEVICE);

Index: linux/drivers/net/s2io.h
===================================================================

drivers/net/s2io.h +0 -4
@@ -75,10 +75,6 @@ static int debug_level = ERR_DBG;
 /* DEBUG message print. */
 #define DBG_PRINT(dbg_level, args...)  if(!(debug_level<dbg_level))
 printk(args)
 
-#ifndef DMA_ERROR_CODE
-#define DMA_ERROR_CODE          (~(dma_addr_t)0x0)
-#endif
-
 /* Protocol assist features of the NIC */
 #define L3_CKSUM_OK 0xFFFF

#define L4_CKSUM_OK 0xFFFF

Applied
#2 lore raw
In reply to Andi Kleen
Quoted 15 lines · Andi Kleen

Fix IOMMU overflow checking in s2io.c

s2io has IOMMU overflow checking, but unfortunately it is wrong.

It didn't use the standard macros, which meant that it only worked
on POWER and SPARC because only those define DMA_ERROR_CODE. Convert it to use the standard macros instead.

I also commented two more bugs in the IOMMU handling. It assumes that 0 DMA addresses cannot happen, but that's not true in all IOMMU setups. The information if a buffer has been already mapped needs to be stored elsewhere.

Didn't fix those because it needs careful checking of the buffer handling by the maintainers.

Quoted 6 lines
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by Andi Kleen <[email protected]>

applied