| Message ID | 1436738697-21652-1-git-send-email-simon.guinot@sequanux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
From: Simon Guinot <simon.guinot@sequanux.org> Date: Mon, 13 Jul 2015 00:04:57 +0200 > On some Armada 370-based NAS, I have experimented kernel bugs and > crashes when the mvneta Ethernet driver fails to refill Rx DMA buffers > due to memory shortage. > > With the actual code, if the memory allocation fails while refilling a > Rx DMA buffer, then the corresponding descriptor is let with the address > of an unmapped DMA buffer already passed to the network stack. Since the > driver still increments the non-occupied counter for Rx descriptor (if a > next packet is handled successfully), then the Ethernet controller is > allowed to reuse the unfilled Rx descriptor... > > As a fix, this patch first refills a Rx descriptor before handling the > stored data and unmapping the associated Rx DMA buffer. Additionally the > occupied and non-occupied counters for the Rx descriptors queues are now > both updated with the rx_done value: the number of descriptors ready to > be returned to the networking controller. Moreover note that there is no > point in using different values for this counters because both the > mvneta driver and the Ethernet controller are unable to handle holes in > the Rx descriptors queues. > > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> > Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP network unit") > Cc: <stable@vger.kernel.org> # v3.8+ > Tested-by: Yoann Sculo <yoann@sculo.fr> Failed memory allocations, are normal and if necessary kernel log messages will be triggered by the core memory allocator. So there is zero reason to do anything other than bump the drop counter in your driver. If I understand the rest of your logic, if the allocator fails, we will reuse the original SKB and place it back into the RX ring? Right? If not, that's the approach you should be using and it is what we advise all networking driver authors to do.
On Wed, Jul 15, 2015 at 03:52:56PM -0700, David Miller wrote: > From: Simon Guinot <simon.guinot@sequanux.org> > Date: Mon, 13 Jul 2015 00:04:57 +0200 > > > On some Armada 370-based NAS, I have experimented kernel bugs and > > crashes when the mvneta Ethernet driver fails to refill Rx DMA buffers > > due to memory shortage. > > > > With the actual code, if the memory allocation fails while refilling a > > Rx DMA buffer, then the corresponding descriptor is let with the address > > of an unmapped DMA buffer already passed to the network stack. Since the > > driver still increments the non-occupied counter for Rx descriptor (if a > > next packet is handled successfully), then the Ethernet controller is > > allowed to reuse the unfilled Rx descriptor... > > > > As a fix, this patch first refills a Rx descriptor before handling the > > stored data and unmapping the associated Rx DMA buffer. Additionally the > > occupied and non-occupied counters for the Rx descriptors queues are now > > both updated with the rx_done value: the number of descriptors ready to > > be returned to the networking controller. Moreover note that there is no > > point in using different values for this counters because both the > > mvneta driver and the Ethernet controller are unable to handle holes in > > the Rx descriptors queues. > > > > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org> > > Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP network unit") > > Cc: <stable@vger.kernel.org> # v3.8+ > > Tested-by: Yoann Sculo <yoann@sculo.fr> Hi David, > > Failed memory allocations, are normal and if necessary kernel log > messages will be triggered by the core memory allocator. So there is > zero reason to do anything other than bump the drop counter in your > driver. Sure. > > If I understand the rest of your logic, if the allocator fails, we > will reuse the original SKB and place it back into the RX ring? > Right? Yes that's what does the patch. Without this patch, in case of memory allocation error, the original buffer is both passed to the networking stack in a SKB _and_ let in the Rx ring. This leads to various kernel oops and crashes. Here are the problems with the original code: - Rx descriptor refilling is tried after passing the SKB to the networking stack. - In case of refilling error, the buffer (passed in the SKB) is also let in the Rx ring (as an unmapped DMA buffer). - And (always in case of refilling error), the non-occupied counter of the Rx queue (hardware register) is not incremented. The idea was maybe to prevent the networking controller to reuse the non-refilled descriptor. But since this counter is incremented as soon as a next descriptor is handled successfully, it is not correct. The patch: - Moves Rx descriptor refilling ahead of passing the SKB to the networking stack. - places the buffer back into the Rx ring in case of refilling error. - And updates correctly the non-occupied descriptors counter of the Rx queue. Simon
From: Simon Guinot <simon.guinot@sequanux.org> Date: Thu, 16 Jul 2015 02:25:30 +0200 > On Wed, Jul 15, 2015 at 03:52:56PM -0700, David Miller wrote: >> Failed memory allocations, are normal and if necessary kernel log >> messages will be triggered by the core memory allocator. So there is >> zero reason to do anything other than bump the drop counter in your >> driver. > > Sure. Ok so just get rid of this log message in your patch and resubmit, thanks.
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index ce5f7f9cff06..ac3da11e63a0 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -1455,7 +1455,7 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo, struct mvneta_rx_queue *rxq) { struct net_device *dev = pp->dev; - int rx_done, rx_filled; + int rx_done; u32 rcvd_pkts = 0; u32 rcvd_bytes = 0; @@ -1466,7 +1466,6 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo, rx_todo = rx_done; rx_done = 0; - rx_filled = 0; /* Fairness NAPI loop */ while (rx_done < rx_todo) { @@ -1477,7 +1476,6 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo, int rx_bytes, err; rx_done++; - rx_filled++; rx_status = rx_desc->status; rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE); data = (unsigned char *)rx_desc->buf_cookie; @@ -1517,6 +1515,14 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo, continue; } + /* Refill processing */ + err = mvneta_rx_refill(pp, rx_desc); + if (err) { + netdev_err(dev, "Linux processing - Can't refill\n"); + rxq->missed++; + goto err_drop_frame; + } + skb = build_skb(data, pp->frag_size > PAGE_SIZE ? 0 : pp->frag_size); if (!skb) goto err_drop_frame; @@ -1536,14 +1542,6 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo, mvneta_rx_csum(pp, rx_status, skb); napi_gro_receive(&pp->napi, skb); - - /* Refill processing */ - err = mvneta_rx_refill(pp, rx_desc); - if (err) { - netdev_err(dev, "Linux processing - Can't refill\n"); - rxq->missed++; - rx_filled--; - } } if (rcvd_pkts) { @@ -1556,7 +1554,7 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo, } /* Update rxq management counters */ - mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_filled); + mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_done); return rx_done; }