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: <20240617185956.GY8447@kernel.org>
Date: Mon, 17 Jun 2024 19:59:56 +0100
From: Simon Horman <horms@...nel.org>
To: Justin Lai <justinlai0215@...ltek.com>
Cc: Markus Elfring <Markus.Elfring@....de>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
	Hariprasad Kelam <hkelam@...vell.com>,
	Jiri Pirko <jiri@...nulli.us>, Larry Chiu <larry.chiu@...ltek.com>,
	Ping-Ke Shih <pkshih@...ltek.com>,
	Ratheesh Kannoth <rkannoth@...vell.com>
Subject: Re: [v20 02/13] rtase: Implement the .ndo_open function

On Mon, Jun 17, 2024 at 01:28:51PM +0000, Justin Lai wrote:
> 
> > >> How do you think about to increase the application of scope-based resource
> > management?
> > >> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/clean
> > >> up.h#L8
> > >
> > > Due to our tx and rx each having multiple queues that need to allocate
> > > descriptors, if any one of the queues fails to allocate,
> > > rtase_alloc_desc() will return an error. Therefore, using 'goto'
> > > here rather than directly returning seems to be reasonable.
> > 
> > Some goto chains can be replaced by further usage of advanced cleanup
> > techniques, can't they?
> > 
> > Regards,
> > Markus
> 
> rtase_alloc_desc() is used to allocate DMA memory. 
> I'd like to ask if it's better to keep our current method?

Hi Justin,

It may be the case that the techniques recently added by cleanup.h could be
used here. But, OTOH, it is the case that using goto for unwinding errors
is in keeping with current Networking driver best practices.

Regardless of the above, I would suggest that if an error occurs in
rtase_alloc_desc() then it release any resources it has allocated. Assuming
the elements of tp->tx_ring and tp->rx_ring are initialised to NULL when
rtase_alloc_desc is called, it looks like that can be achieved by
rtase_alloc_desc() calling rtase_free_desc().

Something like the following (completely untested!).
Please also note that there is probably no need to call netdev_err on
error, as the memory core should already log on error.

static int rtase_alloc_desc(struct rtase_private *tp)
{
	struct pci_dev *pdev = tp->pdev;
	u32 i;

	/* rx and tx descriptors needs 256 bytes alignment.
	 * dma_alloc_coherent provides more.
	 */
	for (i = 0; i < tp->func_tx_queue_num; i++) {
		tp->tx_ring[i].desc =
				dma_alloc_coherent(&pdev->dev,
						   RTASE_TX_RING_DESC_SIZE,
						   &tp->tx_ring[i].phy_addr,
						   GFP_KERNEL);
		if (!tp->tx_ring[i].desc)
			goto err;
	}

	for (i = 0; i < tp->func_rx_queue_num; i++) {
		tp->rx_ring[i].desc =
				dma_alloc_coherent(&pdev->dev,
						   RTASE_RX_RING_DESC_SIZE,
						   &tp->rx_ring[i].phy_addr,
						   GFP_KERNEL);
		if (!tp->rx_ring[i].desc)
			goto err;
		}
	}

	return 0;

err:
	rtase_free_desc(tp)
	return -ENOMEM;
}

And then rtase_alloc_desc can be called like this in rtase_open():

static int rtase_open(struct net_device *dev)
{
	struct rtase_private *tp = netdev_priv(dev);
	const struct pci_dev *pdev = tp->pdev;
	struct rtase_int_vector *ivec;
	u16 i = 0, j;
	int ret;

	ivec = &tp->int_vector[0];
	tp->rx_buf_sz = RTASE_RX_BUF_SIZE;

	ret = rtase_alloc_desc(tp);
	if (ret)
		return ret;

	ret = rtase_init_ring(dev);
	if (ret)
		goto err_free_all_allocated_mem;

...

err_free_all_allocated_mem:
	rtase_free_desc(tp);

	return ret;
}

This is would be in keeping with my understanding of best practices for
Networking drivers: that callers don't have to worry about cleaning up
resources allocated by functions that return an error.


I would also suggest reading Markus's advice with due care,
as it is not always aligned with best practice for Networking code.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ