Re: [PATCH net] net: dlink: handle dma_map_single() failure properly

Simon Horman
2025-10-03T09:44:27Z

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@gmail.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)