| Message ID | 1513358406-32503-1-git-send-email-al.kochet@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: arc_emac: fix arc_emac_rx() error paths | expand |
From: Alexander Kochetkov <al.kochet@gmail.com> Date: Fri, 15 Dec 2017 20:20:06 +0300 > arc_emac_rx() has some issues found by code review. > > In case netdev_alloc_skb_ip_align() or dma_map_single() failure > rx fifo entry will not be returned to EMAC. > > In case dma_map_single() failure previously allocated skb became > lost to driver. At the same time address of newly allocated skb > will not be provided to EMAC. > > Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> This patch adds quite a few bugs, here is one which shows this is not functionally tested: > - /* receive_skb only if new skb was allocated to avoid holes */ > - netif_receive_skb(skb); > - > - addr = dma_map_single(&ndev->dev, (void *)rx_buff->skb->data, > + addr = dma_map_single(&ndev->dev, (void *)skb->data, > EMAC_BUFFER_SIZE, DMA_FROM_DEVICE); Map the new SKB. > if (dma_mapping_error(&ndev->dev, addr)) { > if (net_ratelimit()) > - netdev_err(ndev, "cannot dma map\n"); > - dev_kfree_skb(rx_buff->skb); > + netdev_err(ndev, "cannot map dma buffer\n"); > + dev_kfree_skb(skb); > + /* Return ownership to EMAC */ > + rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE); > stats->rx_errors++; > + stats->rx_dropped++; > continue; > } > + > + /* unmap previosly mapped skb */ > + dma_unmap_single(&ndev->dev, dma_unmap_addr(rx_buff, addr), > + dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE); And then you unmap it. "addr" is the new DMA mapping, not the old one.
> 19 дек. 2017 г., в 18:22, David Miller <davem@davemloft.net> написал(а): > > From: Alexander Kochetkov <al.kochet@gmail.com> > Date: Fri, 15 Dec 2017 20:20:06 +0300 > >> arc_emac_rx() has some issues found by code review. >> >> In case netdev_alloc_skb_ip_align() or dma_map_single() failure >> rx fifo entry will not be returned to EMAC. >> >> In case dma_map_single() failure previously allocated skb became >> lost to driver. At the same time address of newly allocated skb >> will not be provided to EMAC. >> >> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> > > This patch adds quite a few bugs, here is one which shows this is not > functionally tested: May be I don’t understand correctly, but I don’t see bug here. Wrong dma mapping usage should immediately manifest itself (kernel instability, koops and so on). The patch was tested on rk3188 and work fine for me. Also I did simulations of netdev_alloc_skb_ip_align() and dma_map_single() faults and can confirm that new error handling work. Could someone else with ARC EMAC test the patch? I would be very grateful for the help. Florian or Eric, can you test it on your hardware? >> + >> + /* unmap previosly mapped skb */ >> + dma_unmap_single(&ndev->dev, dma_unmap_addr(rx_buff, addr), >> + dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE); > > And then you unmap it. "addr" is the new DMA mapping, not the old one. The old mapping should be unmapped here. It refer to old skb what contains already received packet. Because buffer doesn’t belong to EMAC anymore. That old mapping point to old skb buffer. And netif_receive_skb() will be called for old skb after that. The new mapping «addr" will be unmapped then EMAC receive new packet. Later by the code (after netif_receive_skb()) there are lines for saving «addr» value: rx_buff->skb = skb; dma_unmap_addr_set(rx_buff, addr, addr); dma_unmap_len_set(rx_buff, len, EMAC_BUFFER_SIZE); Regards, Alexander.
From: Alexander Kochetkov <al.kochet@gmail.com> Date: Tue, 19 Dec 2017 18:49:48 +0300 >> And then you unmap it. "addr" is the new DMA mapping, not the old one. > > The old mapping should be unmapped here. It refer to old skb what contains already > received packet. Because buffer doesn’t belong to EMAC anymore. > That old mapping point to old skb buffer. And netif_receive_skb() will be > called for old skb after that. I misread the code, sorry. I saw the dma_unmap_addr(xxx, addr) and just considered 'addr' as the local variable. It's not, it's in fact the TX struct member. Let me look at this some more. ;-)
On 12/19/2017 07:49 AM, Alexander Kochetkov wrote: > >> 19 дек. 2017 г., в 18:22, David Miller <davem@davemloft.net> написал(а): >> >> From: Alexander Kochetkov <al.kochet@gmail.com> >> Date: Fri, 15 Dec 2017 20:20:06 +0300 >> >>> arc_emac_rx() has some issues found by code review. >>> >>> In case netdev_alloc_skb_ip_align() or dma_map_single() failure >>> rx fifo entry will not be returned to EMAC. >>> >>> In case dma_map_single() failure previously allocated skb became >>> lost to driver. At the same time address of newly allocated skb >>> will not be provided to EMAC. >>> >>> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> >> >> This patch adds quite a few bugs, here is one which shows this is not >> functionally tested: > > May be I don’t understand correctly, but I don’t see bug here. > > Wrong dma mapping usage should immediately manifest itself (kernel > instability, koops and so on). The patch was tested on rk3188 and work fine for me. > Also I did simulations of netdev_alloc_skb_ip_align() and dma_map_single() > faults and can confirm that new error handling work. > > Could someone else with ARC EMAC test the patch? I would be very grateful for the help. > Florian or Eric, can you test it on your hardware? I don't actually have access to this hardware, the only change I did to this driver was to use a standard Device Tree property :)
diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c index b2e0051..0ea57fe 100644 --- a/drivers/net/ethernet/arc/emac_main.c +++ b/drivers/net/ethernet/arc/emac_main.c @@ -212,39 +212,48 @@ static int arc_emac_rx(struct net_device *ndev, int budget) continue; } - pktlen = info & LEN_MASK; - stats->rx_packets++; - stats->rx_bytes += pktlen; - skb = rx_buff->skb; - skb_put(skb, pktlen); - skb->dev = ndev; - skb->protocol = eth_type_trans(skb, ndev); - - dma_unmap_single(&ndev->dev, dma_unmap_addr(rx_buff, addr), - dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE); - - /* Prepare the BD for next cycle */ - rx_buff->skb = netdev_alloc_skb_ip_align(ndev, - EMAC_BUFFER_SIZE); - if (unlikely(!rx_buff->skb)) { + /* Prepare the BD for next cycle. netif_receive_skb() + * only if new skb was allocated and mapped to avoid holes + * in the RX fifo. + */ + skb = netdev_alloc_skb_ip_align(ndev, EMAC_BUFFER_SIZE); + if (unlikely(!skb)) { + if (net_ratelimit()) + netdev_err(ndev, "cannot allocate skb\n"); + /* Return ownership to EMAC */ + rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE); stats->rx_errors++; - /* Because receive_skb is below, increment rx_dropped */ stats->rx_dropped++; continue; } - /* receive_skb only if new skb was allocated to avoid holes */ - netif_receive_skb(skb); - - addr = dma_map_single(&ndev->dev, (void *)rx_buff->skb->data, + addr = dma_map_single(&ndev->dev, (void *)skb->data, EMAC_BUFFER_SIZE, DMA_FROM_DEVICE); if (dma_mapping_error(&ndev->dev, addr)) { if (net_ratelimit()) - netdev_err(ndev, "cannot dma map\n"); - dev_kfree_skb(rx_buff->skb); + netdev_err(ndev, "cannot map dma buffer\n"); + dev_kfree_skb(skb); + /* Return ownership to EMAC */ + rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE); stats->rx_errors++; + stats->rx_dropped++; continue; } + + /* unmap previosly mapped skb */ + dma_unmap_single(&ndev->dev, dma_unmap_addr(rx_buff, addr), + dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE); + + pktlen = info & LEN_MASK; + stats->rx_packets++; + stats->rx_bytes += pktlen; + skb_put(rx_buff->skb, pktlen); + rx_buff->skb->dev = ndev; + rx_buff->skb->protocol = eth_type_trans(rx_buff->skb, ndev); + + netif_receive_skb(rx_buff->skb); + + rx_buff->skb = skb; dma_unmap_addr_set(rx_buff, addr, addr); dma_unmap_len_set(rx_buff, len, EMAC_BUFFER_SIZE);
arc_emac_rx() has some issues found by code review. In case netdev_alloc_skb_ip_align() or dma_map_single() failure rx fifo entry will not be returned to EMAC. In case dma_map_single() failure previously allocated skb became lost to driver. At the same time address of newly allocated skb will not be provided to EMAC. Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> --- drivers/net/ethernet/arc/emac_main.c | 53 ++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 22 deletions(-)