Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1623946 > unrolled thread

[PATCH] net: natsemi: ns83820: add checks for dma mapping error

Back to article view | Back to linux.kernel

Contents

  [PATCH] net: natsemi: ns83820: add checks for dma mapping error Alexey Khoroshilov <khoroshilov@ispras.ru> - 2017-04-15 01:00 +0200
    Re: [PATCH] net: natsemi: ns83820: add checks for dma mapping error David Miller <davem@davemloft.net> - 2017-04-17 21:20 +0200
      [PATCH v2] net: natsemi: ns83820: add checks for dma mapping error Alexey Khoroshilov <khoroshilov@ispras.ru> - 2017-04-22 15:10 +0200
        Re: [PATCH v2] net: natsemi: ns83820: add checks for dma mapping  error Francois Romieu <romieu@fr.zoreil.com> - 2017-04-22 21:30 +0200
        Re: [PATCH v2] net: natsemi: ns83820: add checks for dma mapping  error David Miller <davem@davemloft.net> - 2017-04-24 20:10 +0200

#1623946 — [PATCH] net: natsemi: ns83820: add checks for dma mapping error

The driver does not check if mapping dma memory succeed.
The patch adds the checks and failure handling.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/net/ethernet/natsemi/ns83820.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
index 729095db3e08..7d6d692ebb92 100644
--- a/drivers/net/ethernet/natsemi/ns83820.c
+++ b/drivers/net/ethernet/natsemi/ns83820.c
@@ -534,14 +534,19 @@ static inline int ns83820_add_rx_skb(struct ns83820 *dev, struct sk_buff *skb)
 		);
 #endif
 
+	buf = pci_map_single(dev->pci_dev, skb->data,
+			     REAL_RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
+	if (pci_dma_mapping_error(dev->pci_dev, buf)) {
+		kfree_skb(skb);
+		return 1;
+	}
+
 	sg = dev->rx_info.descs + (next_empty * DESC_SIZE);
 	BUG_ON(NULL != dev->rx_info.skbs[next_empty]);
 	dev->rx_info.skbs[next_empty] = skb;
 
 	dev->rx_info.next_empty = (next_empty + 1) % NR_RX_DESC;
 	cmdsts = REAL_RX_BUF_SIZE | CMDSTS_INTR;
-	buf = pci_map_single(dev->pci_dev, skb->data,
-			     REAL_RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
 	build_rx_desc(dev, sg, 0, buf, cmdsts, 0);
 	/* update link of previous rx */
 	if (likely(next_empty != dev->rx_info.next_rx))
@@ -1136,6 +1141,10 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
 	if (nr_frags)
 		len -= skb->data_len;
 	buf = pci_map_single(dev->pci_dev, skb->data, len, PCI_DMA_TODEVICE);
+	if (pci_dma_mapping_error(dev->pci_dev, buf)) {
+		dev_kfree_skb_any(skb);
+		return NETDEV_TX_OK;
+	}
 
 	first_desc = dev->tx_descs + (free_idx * DESC_SIZE);
 
-- 
2.7.4

[toc] | [next] | [standalone]

#1624815

From: Alexey Khoroshilov <khoroshilov@ispras.ru>
Date: Sat, 15 Apr 2017 01:50:50 +0300

> @@ -1136,6 +1141,10 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
>  	if (nr_frags)
>  		len -= skb->data_len;
>  	buf = pci_map_single(dev->pci_dev, skb->data, len, PCI_DMA_TODEVICE);
> +	if (pci_dma_mapping_error(dev->pci_dev, buf)) {
> +		dev_kfree_skb_any(skb);
> +		return NETDEV_TX_OK;
> +	}
>  
>  	first_desc = dev->tx_descs + (free_idx * DESC_SIZE);
>  

You need to also add this check for the skb_map_dma_frag() calls below
this line, and therefore you'll need to add unwind on such a failure.

[toc] | [prev] | [next] | [standalone]

#1628836 — [PATCH v2] net: natsemi: ns83820: add checks for dma mapping error

The driver does not check if mapping dma memory succeed.
The patch adds the checks and failure handling.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/net/ethernet/natsemi/ns83820.c | 42 +++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
index 729095db3e08..dfc64e1e31f9 100644
--- a/drivers/net/ethernet/natsemi/ns83820.c
+++ b/drivers/net/ethernet/natsemi/ns83820.c
@@ -534,14 +534,19 @@ static inline int ns83820_add_rx_skb(struct ns83820 *dev, struct sk_buff *skb)
 		);
 #endif
 
+	buf = pci_map_single(dev->pci_dev, skb->data,
+			     REAL_RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
+	if (pci_dma_mapping_error(dev->pci_dev, buf)) {
+		kfree_skb(skb);
+		return 1;
+	}
+
 	sg = dev->rx_info.descs + (next_empty * DESC_SIZE);
 	BUG_ON(NULL != dev->rx_info.skbs[next_empty]);
 	dev->rx_info.skbs[next_empty] = skb;
 
 	dev->rx_info.next_empty = (next_empty + 1) % NR_RX_DESC;
 	cmdsts = REAL_RX_BUF_SIZE | CMDSTS_INTR;
-	buf = pci_map_single(dev->pci_dev, skb->data,
-			     REAL_RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
 	build_rx_desc(dev, sg, 0, buf, cmdsts, 0);
 	/* update link of previous rx */
 	if (likely(next_empty != dev->rx_info.next_rx))
@@ -1068,6 +1073,7 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
 	int stopped = 0;
 	int do_intr = 0;
 	volatile __le32 *first_desc;
+	volatile __le32 *desc;
 
 	dprintk("ns83820_hard_start_xmit\n");
 
@@ -1136,11 +1142,13 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
 	if (nr_frags)
 		len -= skb->data_len;
 	buf = pci_map_single(dev->pci_dev, skb->data, len, PCI_DMA_TODEVICE);
+	if (pci_dma_mapping_error(dev->pci_dev, buf))
+		goto dma_error_first;
 
 	first_desc = dev->tx_descs + (free_idx * DESC_SIZE);
 
 	for (;;) {
-		volatile __le32 *desc = dev->tx_descs + (free_idx * DESC_SIZE);
+		desc = dev->tx_descs + (free_idx * DESC_SIZE);
 
 		dprintk("frag[%3u]: %4u @ 0x%08Lx\n", free_idx, len,
 			(unsigned long long)buf);
@@ -1160,6 +1168,8 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
 
 		buf = skb_frag_dma_map(&dev->pci_dev->dev, frag, 0,
 				       skb_frag_size(frag), DMA_TO_DEVICE);
+		if (dma_mapping_error(&dev->pci_dev->dev, buf))
+			goto dma_error;
 		dprintk("frag: buf=%08Lx  page=%08lx offset=%08lx\n",
 			(long long)buf, (long) page_to_pfn(frag->page),
 			frag->page_offset);
@@ -1183,6 +1193,32 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
 		netif_start_queue(ndev);
 
 	return NETDEV_TX_OK;
+
+dma_error:
+	do {
+		free_idx = (free_idx + NR_TX_DESC - 1) % NR_TX_DESC;
+		desc = dev->tx_descs + (free_idx * DESC_SIZE);
+		cmdsts = le32_to_cpu(desc[DESC_CMDSTS]);
+		len = cmdsts & CMDSTS_LEN_MASK;
+		buf = desc_addr_get(desc + DESC_BUFPTR);
+		if (desc == first_desc)
+			pci_unmap_single(dev->pci_dev,
+					buf,
+					len,
+					PCI_DMA_TODEVICE);
+		else
+			pci_unmap_page(dev->pci_dev,
+					buf,
+					len,
+					PCI_DMA_TODEVICE);
+		desc[DESC_CMDSTS] = cpu_to_le32(0);
+		mb();
+	} while (desc != first_desc);
+
+dma_error_first:
+	dev_kfree_skb_any(skb);
+	ndev->stats.tx_errors++;
+	return NETDEV_TX_OK;
 }
 
 static void ns83820_update_stats(struct ns83820 *dev)
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1628875 — Re: [PATCH v2] net: natsemi: ns83820: add checks for dma mapping error

Alexey Khoroshilov <khoroshilov@ispras.ru> :
[...]
> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
> index 729095db3e08..dfc64e1e31f9 100644
> --- a/drivers/net/ethernet/natsemi/ns83820.c
> +++ b/drivers/net/ethernet/natsemi/ns83820.c
[...]
> @@ -1183,6 +1193,32 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
>  		netif_start_queue(ndev);
>  
>  	return NETDEV_TX_OK;
> +
> +dma_error:
> +	do {
> +		free_idx = (free_idx + NR_TX_DESC - 1) % NR_TX_DESC;
> +		desc = dev->tx_descs + (free_idx * DESC_SIZE);
> +		cmdsts = le32_to_cpu(desc[DESC_CMDSTS]);
> +		len = cmdsts & CMDSTS_LEN_MASK;
> +		buf = desc_addr_get(desc + DESC_BUFPTR);
> +		if (desc == first_desc)
> +			pci_unmap_single(dev->pci_dev,
> +					buf,
> +					len,
> +					PCI_DMA_TODEVICE);
> +		else
> +			pci_unmap_page(dev->pci_dev,
> +					buf,
> +					len,
> +					PCI_DMA_TODEVICE);

(use tabs + spaces to indent: code should line up right after the parenthesis)

(premature line breaks imho)

(nevermind, both can be avoided :o) )

> +		desc[DESC_CMDSTS] = cpu_to_le32(0);
> +		mb();
> +	} while (desc != first_desc);
> +
> +dma_error_first:
> +	dev_kfree_skb_any(skb);
> +	ndev->stats.tx_errors++;
                    ^^^^^^^^^ -> should be tx_dropped
> +	return NETDEV_TX_OK;
>  }

You only need a single test in the error loop if you mimic the map loop.
Something like:

diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
index 729095d..5e2dbc9 100644
--- a/drivers/net/ethernet/natsemi/ns83820.c
+++ b/drivers/net/ethernet/natsemi/ns83820.c
@@ -1160,9 +1160,11 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
 
 		buf = skb_frag_dma_map(&dev->pci_dev->dev, frag, 0,
 				       skb_frag_size(frag), DMA_TO_DEVICE);
+		if (dma_mapping_error(&dev->pci_dev->dev, buf))
+			goto err_unmap_frags;
 		dprintk("frag: buf=%08Lx  page=%08lx offset=%08lx\n",
 			(long long)buf, (long) page_to_pfn(frag->page),
 			frag->page_offset);
 		len = skb_frag_size(frag);
 		frag++;
 		nr_frags--;
@@ -1181,8 +1184,27 @@ static netdev_tx_t ns83820_hard_start_xmit(struct sk_buff *skb,
 	/* Check again: we may have raced with a tx done irq */
 	if (stopped && (dev->tx_done_idx != tx_done_idx) && start_tx_okay(dev))
 		netif_start_queue(ndev);
-
+out:
 	return NETDEV_TX_OK;
+
+err_unmap_frags:
+	while (1) {
+		buf = desc_addr_get(desc + DESC_BUFPTR);
+		if (!--nr_frags)
+			break;
+
+		pci_unmap_page(dev->pci_dev, buf, len, PCI_DMA_TODEVICE);
+
+		free_idx = (free_idx - 1) % NR_TX_DESC;
+		desc = dev->tx_descs + (free_idx * DESC_SIZE);
+		len = le32_to_cpu(desc + DESC_CMDSTS) & CMDSTS_LEN_MASK;
+	}
+	pci_unmap_single(dev->pci_dev, buf, len, PCI_DMA_TODEVICE);
+
+err_free_skb:
+	dev_kfree_skb_any(skb);
+	ndev->stats.tx_dropped++;
+	goto out;
 }
 
 static void ns83820_update_stats(struct ns83820 *dev)


Thinking more about it, the driver seems rather unsafe if a failing
start_xmit closely follows a succeeding one. The driver should imho
map frags first *then* plug the remaining hole in the descriptor ring.
Until it does, the implicit assumption about descriptor ownership that
the error unroll loop relies on may be wrong.

-- 
Ueimor

[toc] | [prev] | [next] | [standalone]

#1629844 — Re: [PATCH v2] net: natsemi: ns83820: add checks for dma mapping error

From: Alexey Khoroshilov <khoroshilov@ispras.ru>
Date: Sat, 22 Apr 2017 16:06:05 +0300

> +		free_idx = (free_idx + NR_TX_DESC - 1) % NR_TX_DESC;

This is more simply stated as "(free_idx - 1) % NR_TX_DESC.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web