diff mbox series

[v2,net] lan743x: Fix RX Kernel Panic

Message ID 1552325979-27180-1-git-send-email-Bryan.Whitehead@microchip.com
State Accepted
Delegated to: David Miller
Headers show
Series [v2,net] lan743x: Fix RX Kernel Panic | expand

Commit Message

Bryan Whitehead March 11, 2019, 5:39 p.m. UTC 
It has been noticed that running the speed test at
www.speedtest.net occasionally causes a kernel panic.

Investigation revealed that under this test RX buffer allocation
sometimes fails and returns NULL. But the lan743x driver did
not handle this case.

This patch fixes this issue by attempting to allocate a buffer
before sending the new rx packet to the OS. If the allocation
fails then the new rx packet is dropped and the existing buffer
is reused in the DMA ring.

Updates for v2:
    Additional 2 locations where allocation was not checked,
        has been changed to reuse existing buffer.

Fixes: 23f0703c125b ("lan743x: Add main source files for new lan743x driver")
Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 46 +++++++++++++++++++--------
 1 file changed, 32 insertions(+), 14 deletions(-)

Comments

David Miller March 11, 2019, 6:36 p.m. UTC | #1 
From: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Date: Mon, 11 Mar 2019 13:39:39 -0400

> @@ -2290,7 +2306,9 @@ static int lan743x_rx_ring_init(struct lan743x_rx *rx)
>  
>  	rx->last_head = 0;
>  	for (index = 0; index < rx->ring_size; index++) {
> -		ret = lan743x_rx_allocate_ring_element(rx, index);
> +		struct sk_buff *new_skb = lan743x_rx_allocate_skb(rx);
> +
> +		ret = lan743x_rx_init_ring_element(rx, index, new_skb);

Need to check new_skb for NULL.

Again....
Bryan Whitehead March 11, 2019, 6:50 p.m. UTC | #2 
> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Monday, March 11, 2019 2:37 PM
> To: Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>
> Cc: netdev@vger.kernel.org; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH v2 net] lan743x: Fix RX Kernel Panic
> 
> From: Bryan Whitehead <Bryan.Whitehead@microchip.com>
> Date: Mon, 11 Mar 2019 13:39:39 -0400
> 
> > @@ -2290,7 +2306,9 @@ static int lan743x_rx_ring_init(struct lan743x_rx
> *rx)
> >
> >  	rx->last_head = 0;
> >  	for (index = 0; index < rx->ring_size; index++) {
> > -		ret = lan743x_rx_allocate_ring_element(rx, index);
> > +		struct sk_buff *new_skb = lan743x_rx_allocate_skb(rx);
> > +
> > +		ret = lan743x_rx_init_ring_element(rx, index, new_skb);
> 
> Need to check new_skb for NULL.
> 
> Again....

David,

In this case the function lan743x_rx_init_ring_element does not assume that
new_skb is not NULL. It does check for NULL and returns error in that case.

It does this because allocation use to be inside lan743x_rx_init_ring_element,
when its name was lan743x_rx_allocate_ring_element.

I pulled the allocation part out to solve the other problem, but functionally it
Is exactly the same and will clean up appropriately if allocation returns NULL.

Regards,
Bryan
David Miller March 11, 2019, 7:18 p.m. UTC | #3 
From: <Bryan.Whitehead@microchip.com>
Date: Mon, 11 Mar 2019 18:50:27 +0000

> In this case the function lan743x_rx_init_ring_element does not assume that
> new_skb is not NULL. It does check for NULL and returns error in that case.

Aha, I see, thanks for pointing that out and correcting me.  Let me
review this some more.
David Miller March 11, 2019, 7:20 p.m. UTC | #4 
From: David Miller <davem@davemloft.net>
Date: Mon, 11 Mar 2019 12:18:12 -0700 (PDT)

> From: <Bryan.Whitehead@microchip.com>
> Date: Mon, 11 Mar 2019 18:50:27 +0000
> 
>> In this case the function lan743x_rx_init_ring_element does not assume that
>> new_skb is not NULL. It does check for NULL and returns error in that case.
> 
> Aha, I see, thanks for pointing that out and correcting me.  Let me
> review this some more.

Applied and queued up for -stable, thanks.
Bryan Whitehead March 11, 2019, 7:22 p.m. UTC | #5 
> Applied and queued up for -stable, thanks.
Thanks David
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 4d1b4a2..2876bab 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1902,7 +1902,17 @@  static int lan743x_rx_next_index(struct lan743x_rx *rx, int index)
 	return ((++index) % rx->ring_size);
 }
 
-static int lan743x_rx_allocate_ring_element(struct lan743x_rx *rx, int index)
+static struct sk_buff *lan743x_rx_allocate_skb(struct lan743x_rx *rx)
+{
+	int length = 0;
+
+	length = (LAN743X_MAX_FRAME_SIZE + ETH_HLEN + 4 + RX_HEAD_PADDING);
+	return __netdev_alloc_skb(rx->adapter->netdev,
+				  length, GFP_ATOMIC | GFP_DMA);
+}
+
+static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index,
+					struct sk_buff *skb)
 {
 	struct lan743x_rx_buffer_info *buffer_info;
 	struct lan743x_rx_descriptor *descriptor;
@@ -1911,9 +1921,7 @@  static int lan743x_rx_allocate_ring_element(struct lan743x_rx *rx, int index)
 	length = (LAN743X_MAX_FRAME_SIZE + ETH_HLEN + 4 + RX_HEAD_PADDING);
 	descriptor = &rx->ring_cpu_ptr[index];
 	buffer_info = &rx->buffer_info[index];
-	buffer_info->skb = __netdev_alloc_skb(rx->adapter->netdev,
-					      length,
-					      GFP_ATOMIC | GFP_DMA);
+	buffer_info->skb = skb;
 	if (!(buffer_info->skb))
 		return -ENOMEM;
 	buffer_info->dma_ptr = dma_map_single(&rx->adapter->pdev->dev,
@@ -2060,8 +2068,19 @@  static int lan743x_rx_process_packet(struct lan743x_rx *rx)
 		/* packet is available */
 		if (first_index == last_index) {
 			/* single buffer packet */
+			struct sk_buff *new_skb = NULL;
 			int packet_length;
 
+			new_skb = lan743x_rx_allocate_skb(rx);
+			if (!new_skb) {
+				/* failed to allocate next skb.
+				 * Memory is very low.
+				 * Drop this packet and reuse buffer.
+				 */
+				lan743x_rx_reuse_ring_element(rx, first_index);
+				goto process_extension;
+			}
+
 			buffer_info = &rx->buffer_info[first_index];
 			skb = buffer_info->skb;
 			descriptor = &rx->ring_cpu_ptr[first_index];
@@ -2081,7 +2100,7 @@  static int lan743x_rx_process_packet(struct lan743x_rx *rx)
 			skb_put(skb, packet_length - 4);
 			skb->protocol = eth_type_trans(skb,
 						       rx->adapter->netdev);
-			lan743x_rx_allocate_ring_element(rx, first_index);
+			lan743x_rx_init_ring_element(rx, first_index, new_skb);
 		} else {
 			int index = first_index;
 
@@ -2094,26 +2113,23 @@  static int lan743x_rx_process_packet(struct lan743x_rx *rx)
 			if (first_index <= last_index) {
 				while ((index >= first_index) &&
 				       (index <= last_index)) {
-					lan743x_rx_release_ring_element(rx,
-									index);
-					lan743x_rx_allocate_ring_element(rx,
-									 index);
+					lan743x_rx_reuse_ring_element(rx,
+								      index);
 					index = lan743x_rx_next_index(rx,
 								      index);
 				}
 			} else {
 				while ((index >= first_index) ||
 				       (index <= last_index)) {
-					lan743x_rx_release_ring_element(rx,
-									index);
-					lan743x_rx_allocate_ring_element(rx,
-									 index);
+					lan743x_rx_reuse_ring_element(rx,
+								      index);
 					index = lan743x_rx_next_index(rx,
 								      index);
 				}
 			}
 		}
 
+process_extension:
 		if (extension_index >= 0) {
 			descriptor = &rx->ring_cpu_ptr[extension_index];
 			buffer_info = &rx->buffer_info[extension_index];
@@ -2290,7 +2306,9 @@  static int lan743x_rx_ring_init(struct lan743x_rx *rx)
 
 	rx->last_head = 0;
 	for (index = 0; index < rx->ring_size; index++) {
-		ret = lan743x_rx_allocate_ring_element(rx, index);
+		struct sk_buff *new_skb = lan743x_rx_allocate_skb(rx);
+
+		ret = lan743x_rx_init_ring_element(rx, index, new_skb);
 		if (ret)
 			goto cleanup;
 	}