linux-kernel - Re: [PATCH v5 8/9] PCI: Define scoped based management functions

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: <6595f9eec5986_be70229443@dwillia2-xfh.jf.intel.com.notmuch>
Date: Wed, 3 Jan 2024 16:21:02 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Bjorn Helgaas <helgaas@...nel.org>, Dan Williams
	<dan.j.williams@...el.com>
CC: Bjorn Helgaas <bhelgaas@...gle.com>, Ira Weiny <ira.weiny@...el.com>,
	Jonathan Cameron <jonathan.cameron@...wei.com>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@....com>, Shiju Jose <shiju.jose@...wei.com>,
	Yazen Ghannam <yazen.ghannam@....com>, Davidlohr Bueso <dave@...olabs.net>,
	Dave Jiang <dave.jiang@...el.com>, Alison Schofield
	<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, "Ard
 Biesheuvel" <ardb@...nel.org>, <linux-efi@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
	<linux-pci@...r.kernel.org>
Subject: Re: [PATCH v5 8/9] PCI: Define scoped based management functions

Bjorn Helgaas wrote:
> On Wed, Jan 03, 2024 at 02:38:57PM -0800, Dan Williams wrote:
> > Ira Weiny wrote:
> > > Users of pci_dev_get() can benefit from a scoped based put.  Also,
> > > locking a PCI device is often done within a single scope.
> > > 
> > > Define a pci_dev_put() free function and a PCI device lock guard.  These
> > > will initially be used in new CXL event processing code but is defined
> > > in a separate patch for others to pickup and use/backport easier.
> > 
> > Any heartburn if I take this through cxl.git with the rest in this
> > series? Patch 9 has a dependency on this one.
> 
> No real heartburn.  I was trying to figure out what this does
> since I'm not familiar with the "scoped based put" idea and
> 'git grep -i "scope.*base"' wasn't much help.
> 
> I would kind of like the commit log to say a little more about what
> the "scoped based put" (does that have too many past tenses in it?)
> means and how users of pci_dev_get() will benefit.

That is completely fair, and I should have checked to make sure that the
changelog clarified the impact of the change.

> I don't know what "locking a PCI device is often done within a single
> scope" is trying to tell me either.  What if it's *not* done within a
> single scope?
> 
> Does this change anything for callers of pci_dev_get() and
> pci_dev_put()?
> 
> Does this avoid a common programming error?  I just don't know what
> the benefit of this is yet.
> 
> I'm sure this is really cool stuff, but there's little documentation,
> few existing users, and I don't know what I need to look for when
> reviewing things.

Here a is a re-write of the changelog:

---
PCI: Introduce cleanup helpers for device reference counts and locks

The "goto error" pattern is notorious for introducing subtle resource
leaks. Use the new cleanup.h helpers for PCI device reference counts and
locks.

Similar to the new put_device() and device_lock() cleanup helpers,
__free(put_device) and guard(device), define the same for PCI devices,
__free(pci_dev_put) and guard(pci_dev).  These helpers eliminate the
need for "goto free;" and "goto unlock;" patterns. For example, A
'struct pci_dev *' instance declared as:

	struct pci_dev *pdev __free(pci_dev_put) = NULL;

...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev
goes out of scope (automatic variable scope). If a function wants to
invoke pci_dev_put() on error, but return @pdev on success, it can do:

	return no_free_ptr(pdev);

...or:

	return_ptr(pdev);

For potential cleanup opportunity there are 587 open-coded calls to
pci_dev_put() in the kernel with 65 instances within 10 lines of a goto
statement with the CXL driver threatening to add another one.

The guard() helper holds the associated lock for the remainder of the
current scope in which it was invoked. So, for example:

	func(...)
	{
		if (...) {
			...
			guard(pci_dev); /* pci_dev_lock() invoked here */
			...
		} /* <- implied pci_dev_unlock() triggered here */
	}

There are 15 invocations of pci_dev_unlock() in the kernel with 5
instances within 10 lines of a goto statement. Again, the CXL driver is
threatening to add another.

Introduce these helpers to preclude the addition of new more error prone
goto put; / goto unlock; sequences. For now, these helpers are used in
drivers/cxl/pci.c to allow ACPI error reports to be fed back into the
CXL driver associated with the PCI device identified in the report.

---

As for reviewing conversions that use these new helpers, one of the
gotchas I have found is that it is easy to make a mistake if a goto
still exists in the function after the conversion. So unless and until
all of the resources a function acquires, that currently need a goto to
unwind them on error, can be converted to cleanup.h based helpers it is
best not to mix styles.

I think the function documentation in include/linux/cleanup.h does a
decent job of explaining how to use the helpers. However, I am happy to
suggest some updates if you think it would help.