[v3] net: dlink: handle copy_thresh allocation

[PATCH net v3 2/2] net: dlink: handle copy_thresh allocation failure

Posted by Yeounsu Moon 4 months, 3 weeks ago

The driver did not handle failure of `netdev_alloc_skb_ip_align()`.
If the allocation failed, dereferencing `skb->protocol` could lead to a
NULL pointer dereference.

This patch adds proper error handling by falling back to the `else` clause
when the allocation fails.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-on: D-Link DGE-550T Rev-A3
Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
---
 drivers/net/ethernet/dlink/dl2k.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
index faf8a9fc7ed1..cff90417c05c 100644
--- a/drivers/net/ethernet/dlink/dl2k.c
+++ b/drivers/net/ethernet/dlink/dl2k.c
@@ -965,14 +965,11 @@ receive_packet (struct net_device *dev)
 			struct sk_buff *skb;
 
 			/* Small skbuffs for short packets */
-			if (pkt_len > copy_thresh) {
-				dma_unmap_single(&np->pdev->dev,
-						 desc_to_dma(desc),
-						 np->rx_buf_sz,
-						 DMA_FROM_DEVICE);
-				skb_put(skb = np->rx_skbuff[entry], pkt_len);
-				np->rx_skbuff[entry] = NULL;
-			} else if ((skb = netdev_alloc_skb_ip_align(dev, pkt_len))) {
+			if (pkt_len <= copy_thresh) {
+				skb = netdev_alloc_skb_ip_align(dev, pkt_len);
+				if (!skb)
+					goto fallback_to_normal_path;
+
 				dma_sync_single_for_cpu(&np->pdev->dev,
 							desc_to_dma(desc),
 							np->rx_buf_sz,
@@ -985,6 +982,14 @@ receive_packet (struct net_device *dev)
 							   desc_to_dma(desc),
 							   np->rx_buf_sz,
 							   DMA_FROM_DEVICE);
+			} else {
+fallback_to_normal_path:
+				dma_unmap_single(&np->pdev->dev,
+						 desc_to_dma(desc),
+						 np->rx_buf_sz,
+						 DMA_FROM_DEVICE);
+				skb_put(skb = np->rx_skbuff[entry], pkt_len);
+				np->rx_skbuff[entry] = NULL;
 			}
 			skb->protocol = eth_type_trans (skb, dev);
 #if 0
-- 
2.51.0

Re: [PATCH net v3 2/2] net: dlink: handle copy_thresh allocation failure

Posted by Jakub Kicinski 4 months, 3 weeks ago

On Wed, 17 Sep 2025 03:33:05 +0900 Yeounsu Moon wrote:
> @@ -965,14 +965,11 @@ receive_packet (struct net_device *dev)
>  			struct sk_buff *skb;
>  
>  			/* Small skbuffs for short packets */
> -			if (pkt_len > copy_thresh) {
> -				dma_unmap_single(&np->pdev->dev,
> -						 desc_to_dma(desc),
> -						 np->rx_buf_sz,
> -						 DMA_FROM_DEVICE);
> -				skb_put(skb = np->rx_skbuff[entry], pkt_len);
> -				np->rx_skbuff[entry] = NULL;
> -			} else if ((skb = netdev_alloc_skb_ip_align(dev, pkt_len))) {
> +			if (pkt_len <= copy_thresh) {
> +				skb = netdev_alloc_skb_ip_align(dev, pkt_len);
> +				if (!skb)
> +					goto fallback_to_normal_path;

The goto looks pretty awkward.

	skb = NULL;
	if (pkt_len <= copy_thresh)
		skb = netdev_alloc_skb_ip_align(dev, pkt_len);
	if (!skb) {
		// existing non-copy path
	} else {
		// existing copybreak path
	}

Re: [PATCH net v3 2/2] net: dlink: handle copy_thresh allocation failure

Posted by Yeounsu Moon 4 months, 2 weeks ago

On Thu Sep 18, 2025 at 8:09 AM KST, Jakub Kicinski wrote:

Thank you for reviewing! and sorry for the delayed reply.
There have been quite a lot of things on my end recently.

> On Wed, 17 Sep 2025 03:33:05 +0900 Yeounsu Moon wrote:
>> @@ -965,14 +965,11 @@ receive_packet (struct net_device *dev)
>>  			struct sk_buff *skb;
>>  
>>  			/* Small skbuffs for short packets */
>> -			if (pkt_len > copy_thresh) {
>> -				dma_unmap_single(&np->pdev->dev,
>> -						 desc_to_dma(desc),
>> -						 np->rx_buf_sz,
>> -						 DMA_FROM_DEVICE);
>> -				skb_put(skb = np->rx_skbuff[entry], pkt_len);
>> -				np->rx_skbuff[entry] = NULL;
>> -			} else if ((skb = netdev_alloc_skb_ip_align(dev, pkt_len))) {
>> +			if (pkt_len <= copy_thresh) {
>> +				skb = netdev_alloc_skb_ip_align(dev, pkt_len);
>> +				if (!skb)
>> +					goto fallback_to_normal_path;
>
> The goto looks pretty awkward.
>
> 	skb = NULL;
> 	if (pkt_len <= copy_thresh)
> 		skb = netdev_alloc_skb_ip_align(dev, pkt_len);
> 	if (!skb) {
> 		// existing non-copy path
> 	} else {
> 		// existing copybreak path
> 	}

I totally agree with your point. However, the two cases handle `skb` and
`rx_skbuff` differently depending on the `copy_thresh` condition,
regardless of whether `skb` is NULL or not.

This patch is only intended to gracefully handle the failure case when `skb`
allocation fails.

	Yeounsu Moon

Re: [PATCH net v3 2/2] net: dlink: handle copy_thresh allocation failure

Posted by Andrew Lunn 4 months, 3 weeks ago

On Wed, Sep 17, 2025 at 03:33:05AM +0900, Yeounsu Moon wrote:
> The driver did not handle failure of `netdev_alloc_skb_ip_align()`.
> If the allocation failed, dereferencing `skb->protocol` could lead to a
> NULL pointer dereference.
> 
> This patch adds proper error handling by falling back to the `else` clause
> when the allocation fails.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Tested-on: D-Link DGE-550T Rev-A3
> Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew