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: <20251008091346.GO3060232@horms.kernel.org>
Date: Wed, 8 Oct 2025 10:13:46 +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 Sun, Oct 05, 2025 at 02:22:43PM +0900, Yeounsu Moon wrote:
> Hello Simon.
> 
> I'm currenly re-writing the code as you suggested. I think `alloc_list()` 
> can easily adopt the `goto` pattern, but for others functions, it's not 
> that straightforward.
> 
> My question is whether a style combining `goto`, `continue`, and `break`
> would be acceptable in this context:
> 
> ```c
> 	if (np->cur_rx - np->old_rx >= RX_RING_SIZE) {
> 		printk(KERN_INFO "Try to recover rx ring exhausted...\n");
> 		/* Re-allocate skbuffs to fill the descriptor ring */
> 		for (; np->cur_rx - np->old_rx > 0; np->old_rx++) {
> 			struct sk_buff *skb;
> 			dma_addr_t addr;
> 			entry = np->old_rx % RX_RING_SIZE;
> 			/* Dropped packets don't need to re-allocate */
> 			if (np->rx_skbuff[entry])
> 				goto fill_entry;
> 
> 			skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
> 			if (skb == NULL)
> 				goto out_clear_fraginfo;
> 
> 			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 out_kfree_skb;
> 
> 			np->rx_skbuff[entry] = skb;
> 			np->rx_ring[entry].fraginfo = cpu_to_le64(addr);
> fill_entry:
> 			np->rx_ring[entry].fraginfo |=
> 			    cpu_to_le64((u64)np->rx_buf_sz << 48);
> 			np->rx_ring[entry].status = 0;
> 			continue;
> 
> out_kfree_skb:
> 			dev_kfree_skb_irq(skb);
> out_clear_fraginfo:
> 			np->rx_ring[entry].fraginfo = 0;
> 			printk(KERN_INFO
> 			       "%s: Still unable to re-allocate Rx skbuff.#%d\n"
> 			       , dev->name, entry);
> 			break;
> 		} /* end for */
> 	} /* end if */
> 	spin_unlock_irqrestore (&np->rx_lock, flags);
> 	np->timer.expires = jiffies + next_tick;
> 	add_timer(&np->timer);
> }
> ```
> 
> Or is there any better way to handle errors here?
> I'd appreciate your guidance.

Sorry for the slow response, I've been ill for the past few days.

I did also consider the option above. That is handling the
errors in the loop. And I can see some merit in that approach,
e.g. reduced scope of variables.

But I think the more idiomatic approach is to handle them 'here'.
That is, at the end of the function. So I would lean towards
that option.