Message-ID: <171097196970.1011049.9726486429680041876.stgit@dwillia2-xfh.jf.intel.com>
Date: Wed, 20 Mar 2024 15:04:41 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: peterz@...radead.org, torvalds@...ux-foundation.org
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Ira Weiny <ira.weiny@...el.com>,
 Jonathan Cameron <jonathan.cameron@...wei.com>,
 Jesse Brandeburg <jesse.brandeburg@...el.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Lukas Wunner <lukas.wunner@...el.com>, Jonathan Corbet <corbet@....net>,
 linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
 gregkh@...uxfoundation.org, linux-doc@...r.kernel.org
Subject: [PATCH] cleanup: Add usage and style documentation

When proposing that PCI grow some new cleanup helpers for pci_dev_put()
and pci_dev_{lock,unlock} [1], Bjorn had some fundamental questions
about expectations and best practices. Upon reviewing an updated
changelog with those details he recommended adding them to documentation
in the header file itself.

Add that documentation and link it into the rendering for
Documentation/core-api/.

Link: http://lore.kernel.org/r/20240104183218.GA1820872@bhelgaas [1]
Cc: Bjorn Helgaas <bhelgaas@...gle.com>
Cc: Ira Weiny <ira.weiny@...el.com>
Cc: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Lukas Wunner <lukas.wunner@...el.com>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Jonathan Corbet <corbet@....net>
Signed-off-by: Dan Williams <dan.j.williams@...el.com>
---
Peter, Linus,

I am starting to see more usage of the cleanup helpers and some
style confusion or misunderstanding on best practices on how to use
them. As I mention above, Bjorn found the writeup I did for justifying
__free(pci_dev_put) and guard(pci_dev) useful, so here is an attempt to
uplevel and centralize those notes.

Linus, I include you directly since you have expressed some opinions on
how these helpers are used and want to capture that in a central
location.

This patch stops short of updating coding-style or checkpatch, but I
expect that it can later be used as a reference for that work.

 Documentation/core-api/cleanup.rst |    8 +++
 Documentation/core-api/index.rst   |    1 
 include/linux/cleanup.h            |  112 ++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+)
 create mode 100644 Documentation/core-api/cleanup.rst

diff --git a/Documentation/core-api/cleanup.rst b/Documentation/core-api/cleanup.rst
new file mode 100644
index 000000000000..527eb2f8ec6e
--- /dev/null
+++ b/Documentation/core-api/cleanup.rst
@@ -0,0 +1,8 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===========================
+Scope-based Cleanup Helpers
+===========================
+
+.. kernel-doc:: include/linux/cleanup.h
+   :doc: scope-based cleanup helpers
diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index 7a3a08d81f11..845fbd54948f 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -36,6 +36,7 @@ Library functionality that is used throughout the kernel.
    kobject
    kref
    assoc_array
+   cleanup
    xarray
    maple_tree
    idr
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..4620a475faee 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -4,6 +4,118 @@
 
 #include <linux/compiler.h>
 
+/**
+ * DOC: scope-based cleanup helpers
+ *
+ * The "goto error" pattern is notorious for introducing subtle resource
+ * leaks. It is tedious and error prone to add new resource acquisition
+ * constraints into code paths that already have several unwind
+ * conditions. The "cleanup" helpers enable the compiler to help with
+ * this tedium and can aid in maintaining FILO (first in last out)
+ * unwind ordering to avoid unintentional leaks.
+ *
+ * As drivers make up the majority of the kernel code base lets describe
+ * the Theory of Operation, Coding Style implications, and motivation
+ * for using these helpers through the example of cleaning up PCI
+ * drivers with DEFINE_FREE() and DEFINE_GUARD(), e.g.:
+ *
+ * .. code-block:: c
+ *
+ *	DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
+ *	DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
+ *
+ * The DEFINE_FREE(pci_dev_put, ...) definition allows for declaring
+ * variables like this:
+ *
+ * .. code-block:: c
+ *
+ *	struct pci_dev *dev __free(pci_dev_put) =
+ *		pci_get_slot(parent, PCI_DEVFN(0, 0));
+ *
+ * The above 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
+ * (i.e. without freeing it) on success, it can do:
+ *
+ * .. code-block:: c
+ *
+ *	return no_free_ptr(pdev);
+ *
+ * ...or:
+ *
+ * .. code-block:: c
+ *
+ *	return_ptr(pdev);
+ *
+ * Note that unwind order is dictated by declaration order. That
+ * contraindicates a pattern like the following:
+ *
+ * .. code-block:: c
+ *
+ *	int num, ret = 0;
+ *	struct pci_dev *bridge = ctrl->pcie->port;
+ *	struct pci_bus *parent = bridge->subordinate;
+ *	struct pci_dev *dev __free(pci_dev_put) = NULL;
+ *
+ *	pci_lock_rescan_remove();
+ *
+ *	dev = pci_get_slot(parent, PCI_DEVFN(0, 0));
+ *
+ * In this case @dev is declared in x-mas tree style in a preamble
+ * declaration block. That is problematic because it destroys the
+ * compiler's ability to infer proper unwind order. If other cleanup
+ * helpers appeared in such a function that depended on @dev being live
+ * to complete their unwind then using the "struct obj_type *obj
+ * __free(...) = NULL" style is an anti-pattern that potentially causes
+ * a use-after-free bug. Instead, the expectation is this conversion:
+ *
+ * .. code-block:: c
+ *
+ *	int num, ret = 0;
+ *	struct pci_dev *bridge = ctrl->pcie->port;
+ *	struct pci_bus *parent = bridge->subordinate;
+ *
+ *	pci_lock_rescan_remove();
+ *
+ *	struct pci_dev *dev __free(pci_dev_put) =
+ *		pci_get_slot(parent, PCI_DEVFN(0, 0));
+ *
+ * ...which implies that declaring variables in mid-function scope is
+ * not only allowed, but expected.
+ *
+ * The motivation for deploying DEFINE_FREE(pci_dev_put, ...) is that at
+ * the time of writing of this documentation there are ~590 instances of
+ * pci_dev_put(), ~70 of them with 10 lines of a goto implying that a
+ * significant number of gotos might be cleaned up for incremental
+ * maintenance burden relief.
+ *
+ * The guard() helper holds the associated lock for the remainder of the
+ * current scope in which it was invoked. So, for example:
+ *
+ * .. code-block:: c
+ *
+ *	func(...)
+ *	{
+ *		if (...) {
+ *			...
+ *			guard(pci_dev); // pci_dev_lock() invoked here
+ *			...
+ *		} // <- implied pci_dev_unlock() triggered here
+ *	}
+ *
+ * ...in other words, the lock is held for the remainder of the current
+ * scope not the remainder of "func()".
+ *
+ * At the time of writing there are 15 invocations of pci_dev_unlock() in
+ * the kernel with 5 within 10 lines of a goto.
+ *
+ * Conversions of existing code to use cleanup helpers should convert
+ * all resources so that no "goto" unwind statements remain. If not all
+ * resources are amenable to cleanup then additional refactoring is
+ * needed to build helper functions, or the function is simply not a
+ * good candidate for conversion.
+ */
+
 /*
  * DEFINE_FREE(name, type, free):
  *	simple helper macro that defines the required wrapper for a __free()

Powered by blists - more mailing lists