linux-kernel - 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: <DD8ORNMB155Q.1JK8F13D9FLNR@gmail.com>
Date: Fri, 03 Oct 2025 21:29:23 +0900
From: "Yeounsu Moon" <yyyynoom@...il.com>
To: "Simon Horman" <horms@...nel.org>
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 3, 2025 at 6:44 PM KST, Simon Horman wrote:
> 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().
Thank you for pointing that out. I spent some time making sure that the
`Fixes:` tag you mentioned was applied correctly.
>
> 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.
I agree that your approach is much cleaner. However, I structured the
code as intuitively and simply as possible, since making it more complex
could make the review harder. I'm curious about the opinions of other
reviewers and the maintainer. If an immediate change is required, I can 
submit a v2; otherwise; I'll first post the fix patch to `net` and incorporate
your suggestion in `net-next`.
>
> 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)