LinuxLists.cc - [PATCH] net: natsemi: ns83820: add checks for dma mapping error

2017-04-14 22:51:05

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

2017-04-17 19:17:50

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

From: Alexey Khoroshilov <[email protected]>
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.

2017-04-22 13:06:33

by Alexey Khoroshilov

[permalink] [raw]

Subject: [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 <[email protected]>
---
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

2017-04-22 19:20:35

by Francois Romieu

[permalink] [raw]

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

Alexey Khoroshilov <[email protected]> :
[...]
> 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

2017-04-24 18:05:08

by David Miller

[permalink] [raw]

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

From: Alexey Khoroshilov <[email protected]>
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.