wan: dscc4: add checks for dma mapping errors
diff mbox series

Message ID 1501878204-24270-1-git-send-email-khoroshilov@ispras.ru
State New, archived
Headers show
Series
  • wan: dscc4: add checks for dma mapping errors
Related show

Commit Message

Alexey Khoroshilov Aug. 4, 2017, 8:23 p.m. UTC
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/wan/dscc4.c | 52 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 15 deletions(-)

Comments

David Miller Aug. 7, 2017, 9:06 p.m. UTC | #1
From: Alexey Khoroshilov <khoroshilov@ispras.ru>
Date: Fri,  4 Aug 2017 23:23:24 +0300

> 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>

This is a great example of why it can be irritating to see these
mechanical "bug fixes" for drivers very few people use and actually
test, which introduces new bugs.

> @@ -522,19 +522,27 @@ static inline int try_get_rx_skb(struct dscc4_dev_priv *dpriv,
>  	struct RxFD *rx_fd = dpriv->rx_fd + dirty;
>  	const int len = RX_MAX(HDLC_MAX_MRU);
>  	struct sk_buff *skb;
> -	int ret = 0;
> +	dma_addr_t addr;
>  
>  	skb = dev_alloc_skb(len);
>  	dpriv->rx_skbuff[dirty] = skb;

skb recorded here.

> +err_free_skb:
> +	dev_kfree_skb_any(skb);

Yet freed here in the error path.

dpriv->rx_skbuff[dirty] should not be set to 'skb' until all possibile
failure tests have passed.
Francois Romieu Aug. 7, 2017, 9:59 p.m. UTC | #2
Alexey Khoroshilov <khoroshilov@ispras.ru> :
> 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>

Please amend your subject line as:

Subject: [PATCH net-next v2 1/1] dscc4: add checks for dma mapping errors.

Rationale: davem is not supposed to guess the branch the patch should be
applied to.

[...]
> diff --git a/drivers/net/wan/dscc4.c b/drivers/net/wan/dscc4.c
> index 799830ffcae2..1a94f0a95b2c 100644
> --- a/drivers/net/wan/dscc4.c
> +++ b/drivers/net/wan/dscc4.c
> @@ -522,19 +522,27 @@ static inline int try_get_rx_skb(struct dscc4_dev_priv *dpriv,
>  	struct RxFD *rx_fd = dpriv->rx_fd + dirty;
>  	const int len = RX_MAX(HDLC_MAX_MRU);
>  	struct sk_buff *skb;
> -	int ret = 0;
> +	dma_addr_t addr;
>  
>  	skb = dev_alloc_skb(len);
>  	dpriv->rx_skbuff[dirty] = skb;
> -	if (skb) {
> -		skb->protocol = hdlc_type_trans(skb, dev);
> -		rx_fd->data = cpu_to_le32(pci_map_single(dpriv->pci_priv->pdev,
> -					  skb->data, len, PCI_DMA_FROMDEVICE));
> -	} else {
> -		rx_fd->data = 0;
> -		ret = -1;
> -	}
> -	return ret;
> +	if (!skb)
> +		goto err_out;
> +
> +	skb->protocol = hdlc_type_trans(skb, dev);
> +	addr = pci_map_single(dpriv->pci_priv->pdev,
> +			      skb->data, len, PCI_DMA_FROMDEVICE);
> +	if (pci_dma_mapping_error(dpriv->pci_priv->pdev, addr))
> +		goto err_free_skb;

Nit: please use a local 'struct pci_dev *pdev = dpriv->pci_priv->pdev;'

[...]
> @@ -1147,14 +1155,22 @@ static netdev_tx_t dscc4_start_xmit(struct sk_buff *skb,
>  	struct dscc4_dev_priv *dpriv = dscc4_priv(dev);
>  	struct dscc4_pci_priv *ppriv = dpriv->pci_priv;
>  	struct TxFD *tx_fd;
> +	dma_addr_t addr;
>  	int next;
>  
> +	addr = pci_map_single(ppriv->pdev, skb->data, skb->len,
> +			      PCI_DMA_TODEVICE);
> +	if (pci_dma_mapping_error(ppriv->pdev, addr)) {
> +		dev_kfree_skb_any(skb);
> +		dev->stats.tx_errors++;

It should read 'dev->stats.tx_dropped++'.

Patch
diff mbox series

diff --git a/drivers/net/wan/dscc4.c b/drivers/net/wan/dscc4.c
index 799830ffcae2..1a94f0a95b2c 100644
--- a/drivers/net/wan/dscc4.c
+++ b/drivers/net/wan/dscc4.c
@@ -522,19 +522,27 @@  static inline int try_get_rx_skb(struct dscc4_dev_priv *dpriv,
 	struct RxFD *rx_fd = dpriv->rx_fd + dirty;
 	const int len = RX_MAX(HDLC_MAX_MRU);
 	struct sk_buff *skb;
-	int ret = 0;
+	dma_addr_t addr;
 
 	skb = dev_alloc_skb(len);
 	dpriv->rx_skbuff[dirty] = skb;
-	if (skb) {
-		skb->protocol = hdlc_type_trans(skb, dev);
-		rx_fd->data = cpu_to_le32(pci_map_single(dpriv->pci_priv->pdev,
-					  skb->data, len, PCI_DMA_FROMDEVICE));
-	} else {
-		rx_fd->data = 0;
-		ret = -1;
-	}
-	return ret;
+	if (!skb)
+		goto err_out;
+
+	skb->protocol = hdlc_type_trans(skb, dev);
+	addr = pci_map_single(dpriv->pci_priv->pdev,
+			      skb->data, len, PCI_DMA_FROMDEVICE);
+	if (pci_dma_mapping_error(dpriv->pci_priv->pdev, addr))
+		goto err_free_skb;
+
+	rx_fd->data = cpu_to_le32(addr);
+	return 0;
+
+err_free_skb:
+	dev_kfree_skb_any(skb);
+err_out:
+	rx_fd->data = 0;
+	return -1;
 }
 
 /*
@@ -1147,14 +1155,22 @@  static netdev_tx_t dscc4_start_xmit(struct sk_buff *skb,
 	struct dscc4_dev_priv *dpriv = dscc4_priv(dev);
 	struct dscc4_pci_priv *ppriv = dpriv->pci_priv;
 	struct TxFD *tx_fd;
+	dma_addr_t addr;
 	int next;
 
+	addr = pci_map_single(ppriv->pdev, skb->data, skb->len,
+			      PCI_DMA_TODEVICE);
+	if (pci_dma_mapping_error(ppriv->pdev, addr)) {
+		dev_kfree_skb_any(skb);
+		dev->stats.tx_errors++;
+		return NETDEV_TX_OK;
+	}
+
 	next = dpriv->tx_current%TX_RING_SIZE;
 	dpriv->tx_skbuff[next] = skb;
 	tx_fd = dpriv->tx_fd + next;
 	tx_fd->state = FrameEnd | TO_STATE_TX(skb->len);
-	tx_fd->data = cpu_to_le32(pci_map_single(ppriv->pdev, skb->data, skb->len,
-				     PCI_DMA_TODEVICE));
+	tx_fd->data = cpu_to_le32(addr);
 	tx_fd->complete = 0x00000000;
 	tx_fd->jiffies = jiffies;
 	mb();
@@ -1889,14 +1905,20 @@  static struct sk_buff *dscc4_init_dummy_skb(struct dscc4_dev_priv *dpriv)
 	if (skb) {
 		int last = dpriv->tx_dirty%TX_RING_SIZE;
 		struct TxFD *tx_fd = dpriv->tx_fd + last;
+		dma_addr_t addr;
 
 		skb->len = DUMMY_SKB_SIZE;
 		skb_copy_to_linear_data(skb, version,
 					strlen(version) % DUMMY_SKB_SIZE);
 		tx_fd->state = FrameEnd | TO_STATE_TX(DUMMY_SKB_SIZE);
-		tx_fd->data = cpu_to_le32(pci_map_single(dpriv->pci_priv->pdev,
-					     skb->data, DUMMY_SKB_SIZE,
-					     PCI_DMA_TODEVICE));
+		addr = pci_map_single(dpriv->pci_priv->pdev,
+				      skb->data, DUMMY_SKB_SIZE,
+				      PCI_DMA_TODEVICE);
+		if (pci_dma_mapping_error(dpriv->pci_priv->pdev, addr)) {
+			dev_kfree_skb_any(skb);
+			return NULL;
+		}
+		tx_fd->data = cpu_to_le32(addr);
 		dpriv->tx_skbuff[last] = skb;
 	}
 	return skb;