netdev - Re: [PATCH net] net: dlink: handle dma_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251003094424.GF2878334@horms.kernel.org>
Date: Fri, 3 Oct 2025 10:44:24 +0100
From: Simon Horman <horms@...nel.org>
To: Yeounsu Moon <yyyynoom@...il.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dlink: handle dma_map_single() failure properly

On Fri, Oct 03, 2025 at 12:26:38AM +0900, Yeounsu Moon wrote:
> Add error handling by checking `dma_mapping_error()` and cleaning up
> the `skb` using the appropriate `dev_kfree_skb*()` variant.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Yeounsu Moon <yyyynoom@...il.com>
> Tested-on: D-Link DGE-550T Rev-A3
> ---
>  drivers/net/ethernet/dlink/dl2k.c | 49 ++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
> index 1996d2e4e3e2..a821c9921745 100644
> --- a/drivers/net/ethernet/dlink/dl2k.c
> +++ b/drivers/net/ethernet/dlink/dl2k.c
> @@ -508,6 +508,7 @@ static int alloc_list(struct net_device *dev)
>  	for (i = 0; i < RX_RING_SIZE; i++) {
>  		/* Allocated fixed size of skbuff */
>  		struct sk_buff *skb;
> +		dma_addr_t addr;
>  
>  		skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
>  		np->rx_skbuff[i] = skb;
> @@ -516,13 +517,19 @@ static int alloc_list(struct net_device *dev)
>  			return -ENOMEM;
>  		}
>  
> +		addr = dma_map_single(&np->pdev->dev, skb->data,
> +				      np->rx_buf_sz, DMA_FROM_DEVICE);
> +		if (dma_mapping_error(&np->pdev->dev, addr)) {
> +			dev_kfree_skb(skb);
> +			np->rx_skbuff[i] = NULL;
> +			free_list(dev);
> +			return -ENOMEM;
> +		}
>  		np->rx_ring[i].next_desc = cpu_to_le64(np->rx_ring_dma +
>  						((i + 1) % RX_RING_SIZE) *
>  						sizeof(struct netdev_desc));
>  		/* Rubicon now supports 40 bits of addressing space. */
> -		np->rx_ring[i].fraginfo =
> -		    cpu_to_le64(dma_map_single(&np->pdev->dev, skb->data,
> -					       np->rx_buf_sz, DMA_FROM_DEVICE));
> +		np->rx_ring[i].fraginfo = cpu_to_le64(addr);
>  		np->rx_ring[i].fraginfo |= cpu_to_le64((u64)np->rx_buf_sz << 48);
>  	}
>  

Thanks.

I agree that this fixes a problem.
And I agree that these problems were introduced by the cited commit,
at a time when pci_map_single() was used instead of dma_map_single().

But I wonder if it would be slightly nicer to make this change by
using the idiomatic pattern of an unwind ladder of goto labels.

In the case of alloc_list(), something like the following.
(Compile tested only!)

diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
index 1996d2e4e3e2..ba81373bbca8 100644
--- a/drivers/net/ethernet/dlink/dl2k.c
+++ b/drivers/net/ethernet/dlink/dl2k.c
@@ -508,25 +508,36 @@ static int alloc_list(struct net_device *dev)
 	for (i = 0; i < RX_RING_SIZE; i++) {
 		/* Allocated fixed size of skbuff */
 		struct sk_buff *skb;
+		dma_addr_t addr;
 
 		skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
 		np->rx_skbuff[i] = skb;
-		if (!skb) {
-			free_list(dev);
-			return -ENOMEM;
-		}
+		if (!skb)
+			goto err_free_list;
+
+		addr = dma_map_single(&np->pdev->dev, skb->data,
+				      np->rx_buf_sz, DMA_FROM_DEVICE);
+		if (dma_mapping_error(&np->pdev->dev, addr))
+			goto err_kfree_skb;
 
 		np->rx_ring[i].next_desc = cpu_to_le64(np->rx_ring_dma +
 						((i + 1) % RX_RING_SIZE) *
 						sizeof(struct netdev_desc));
 		/* Rubicon now supports 40 bits of addressing space. */
-		np->rx_ring[i].fraginfo =
-		    cpu_to_le64(dma_map_single(&np->pdev->dev, skb->data,
-					       np->rx_buf_sz, DMA_FROM_DEVICE));
+		np->rx_ring[i].fraginfo = cpu_to_le64(addr);
 		np->rx_ring[i].fraginfo |= cpu_to_le64((u64)np->rx_buf_sz << 48);
 	}
 
 	return 0;
+
+err_kfree_skb:
+	dev_kfree_skb(np->rx_skbuff[i]);
+	np->rx_skbuff[i] = NULL;
+err_free_list:
+	free_list(dev);
+
+	return -ENOMEM;
+
 }
 
 static void rio_hw_init(struct net_device *dev)