* [PATCH] net: natsemi: ns83820: add checks for dma mapping error @ 2017-04-14 22:50 Alexey Khoroshilov 2017-04-17 19:17 ` David Miller 0 siblings, 1 reply; 5+ messages in thread From: Alexey Khoroshilov @ 2017-04-14 22:50 UTC (permalink / raw) To: David S. Miller; +Cc: Alexey Khoroshilov, netdev, linux-kernel, ldv-project 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: natsemi: ns83820: add checks for dma mapping error 2017-04-14 22:50 [PATCH] net: natsemi: ns83820: add checks for dma mapping error Alexey Khoroshilov @ 2017-04-17 19:17 ` David Miller 2017-04-22 13:06 ` [PATCH v2] " Alexey Khoroshilov 0 siblings, 1 reply; 5+ messages in thread From: David Miller @ 2017-04-17 19:17 UTC (permalink / raw) To: khoroshilov; +Cc: netdev, linux-kernel, ldv-project 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] net: natsemi: ns83820: add checks for dma mapping error 2017-04-17 19:17 ` David Miller @ 2017-04-22 13:06 ` Alexey Khoroshilov 2017-04-22 19:20 ` Francois Romieu 2017-04-24 18:04 ` David Miller 0 siblings, 2 replies; 5+ messages in thread From: Alexey Khoroshilov @ 2017-04-22 13:06 UTC (permalink / raw) To: David S. Miller; +Cc: Alexey Khoroshilov, netdev, linux-kernel, ldv-project 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] net: natsemi: ns83820: add checks for dma mapping error 2017-04-22 13:06 ` [PATCH v2] " Alexey Khoroshilov @ 2017-04-22 19:20 ` Francois Romieu 2017-04-24 18:04 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: Francois Romieu @ 2017-04-22 19:20 UTC (permalink / raw) To: Alexey Khoroshilov; +Cc: David S. Miller, netdev, linux-kernel, ldv-project 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] net: natsemi: ns83820: add checks for dma mapping error 2017-04-22 13:06 ` [PATCH v2] " Alexey Khoroshilov 2017-04-22 19:20 ` Francois Romieu @ 2017-04-24 18:04 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: David Miller @ 2017-04-24 18:04 UTC (permalink / raw) To: khoroshilov; +Cc: netdev, linux-kernel, ldv-project 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-24 18:05 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-14 22:50 [PATCH] net: natsemi: ns83820: add checks for dma mapping error Alexey Khoroshilov 2017-04-17 19:17 ` David Miller 2017-04-22 13:06 ` [PATCH v2] " Alexey Khoroshilov 2017-04-22 19:20 ` Francois Romieu 2017-04-24 18:04 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).