Received-SPF: pass (google.com: domain of linux-kernel+bounces-112129-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 22 Mar 2024 17:17:30 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: "Tian, Kevin" <kevin.tian@intel.com>, "Williams, Dan J"
	<dan.j.williams@intel.com>, "peterz@infradead.org" <peterz@infradead.org>,
	"torvalds@linux-foundation.org" <torvalds@linux-foundation.org>
CC: Bjorn Helgaas <bhelgaas@google.com>, "Weiny, Ira" <ira.weiny@intel.com>,
	Jonathan Cameron <jonathan.cameron@huawei.com>, "Brandeburg, Jesse"
	<jesse.brandeburg@intel.com>, Ilpo =?iso-8859-1?Q?J=E4rvinen?=
	<ilpo.jarvinen@linux.intel.com>, "Wunner, Lukas" <lukas.wunner@intel.com>,
	Jonathan Corbet <corbet@lwn.net>, "linux-pci@vger.kernel.org"
	<linux-pci@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "gregkh@linuxfoundation.org"
	<gregkh@linuxfoundation.org>, "linux-doc@vger.kernel.org"
	<linux-doc@vger.kernel.org>
Subject: RE: [PATCH] cleanup: Add usage and style documentation
Message-ID: <65fe1f9aadf51_2690d2948f@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <171097196970.1011049.9726486429680041876.stgit@dwillia2-xfh.jf.intel.com>
 <BN9PR11MB52764D455B465E7C69F3A4C38C312@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <BN9PR11MB52764D455B465E7C69F3A4C38C312@BN9PR11MB5276.namprd11.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?d1hyCasr4EK7bzOhmW4yHiNseQqqhtt21cuq9nEY5P1SdyyuMX/kZU9Csuci?=
 =?us-ascii?Q?QNsUo86EzEMvkIPYW+4/zOykrgHhTbCcknCjd0wniZ8FT98vhS6afsft+B0d?=
 =?us-ascii?Q?PyIlUA/kUJFDsu3G80ftV/JTa/dbxvl17u9tHSPKGy+seaeKvyUt5xdHLf6E?=
 =?us-ascii?Q?B8Dp3nJgXmCu5R/nmxJfRVDQhhMr+TywrK3wpjerWzMlh6bVpkXxpPeVV/NJ?=
 =?us-ascii?Q?/QQcxvHKpwnEMmGSZZvxgjVCu0K5OosfZxY5ACAvtJIwFYbn1+6jFSW7a7dk?=
 =?us-ascii?Q?2/3cyDqlbPj+zvQ1i9mvnfJoGBsVwDCWBRoTGFeBgXUWID6C8y7GrYpuSssM?=
 =?us-ascii?Q?/azQ9lo7ielaarlUL8EEPT4PkKY56Eh27HlFEeUlhYDZeMoFx+101QNBWkHh?=
 =?us-ascii?Q?cuhdTlOd0fBpwrZY09f79qoIkURKAenahOK4+5FDcmK8K9fF3raLNzMQ8imZ?=
 =?us-ascii?Q?49Yf+9PE3Id3K7Yb6y1RbT6qVI4x25CGKbOaI/bTfR5blkjQGL4rJF59o3U4?=
 =?us-ascii?Q?/GX2WraKfn+lASP8UXhiwtbunPme36jdeKZF8jThKLqpOCkQroFzTizMMzTN?=
 =?us-ascii?Q?I88fqvvo+bqDmu3fnXJL6KQJ6kbqMOjI4uHxyUo/dZ4zIo5Wu6KR4S83JBZM?=
 =?us-ascii?Q?teTXrNygrDSx0k59+ULeIzF1/Y8/UTM2zZBHy8M+E6+f5c3pGa/y8sFT2vMz?=
 =?us-ascii?Q?dU1rr+xyNRFlg6aAPdu4jILsx5iE8Rhd8DSzxHYW7VqiiELTrlNkhdIaLhRx?=
 =?us-ascii?Q?qAe9iYUSo1Fqi7NnOdRBjf5gRQ2U5is9ia7eQfg6N+8CaodXwmCL1ggWiFwh?=
 =?us-ascii?Q?uyqopswZ+8KlIvd+nxzQL/i2RfoEbWJk0nmIY4Den+oIGZOcjAJk03oGT9g7?=
 =?us-ascii?Q?/kjCm4671MdqIHsM+gDihpWLQKZjcH4fLLjSyKD9YOqngDVJIyRu+LxEetJs?=
 =?us-ascii?Q?mbBjt+WW86AHeQ6Lq951fZFSmcr1CZSyzpVdQJ9lmDXlIWg5DeiUb6MDvY5g?=
 =?us-ascii?Q?iIUY4x5yFzyAWf9jf1QvFI7NLSCCKh1vE1yazHYSxFMV31/Jo3qAPt6cfhSk?=
 =?us-ascii?Q?cii4q9oDrPEV03Vy1hF8W0nPNBzsaFG4lck+jQnFoiKL9LoTPgzdwTnv24uh?=
 =?us-ascii?Q?WvHYKPddanSEpzDA9UaV6n8LtMzSVjajI2PGz9rhd9foIkAa08QRkuYre/oE?=
 =?us-ascii?Q?SMTwVf2cRI7cImoHGrtbuc94IYo9hNL7G9K69T/P29ZDWiIYM8kIEeF9ZLjt?=
 =?us-ascii?Q?IyLsZSLO7fKV2hiDMb0a/Hv+AxkIMXp+sEAqQO3TVGvqRTe6b5BAdor6iz/F?=
 =?us-ascii?Q?n7eTUKtXiGRio+IIhgGUuBzFknMlBr0CFMMzgEa6zvUNcnf4BUzNmAmkT+Vp?=
 =?us-ascii?Q?93VV54ATgy8+e67cB5GIqIpzsko+KDD43/4eqGsYCEaemniyjqpHwN4UjlZR?=
 =?us-ascii?Q?4kfKD7nJkEBqwoLDChDl4j9RA73vbJEQFjxBgd+MbsMNBGCvVT3JRuJI/aBs?=
 =?us-ascii?Q?g5ezdO6eQYF12LmzL4eXta6/razr1KagE4anl230Evsav9dDgMiS7IH+kXTI?=
 =?us-ascii?Q?vhEmWZxYQc5ItQ9LUXfDV/Q96sjsIOoF1FXzRQ+7HC1Xika2N6ELzA6/U5Ok?=
 =?us-ascii?Q?rA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bd26b5bc-b00d-48ec-7b9c-08dc4acea2ac
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Mar 2024 00:17:33.4774
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /q8ltKH0m/4ReaGxOUZJcvt3T2or3nvpweWH0fr5N2I70NKnefb5FHA7y5bY02sQhBqDSJQ3k0PvFwVBw8eCOuaIMf+mnrAgeEoiqMO0WA0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6318
X-OriginatorOrg: intel.com

Tian, Kevin wrote:
> > From: Dan Williams <dan.j.williams@intel.com>
> > Sent: Thursday, March 21, 2024 6:05 AM
> > + *
> > + * 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:
> > + *
> 
> an example of dependent cleanup helpers might be helpful to
> better understand this expectation?

The simplest example I can think of to show the danger of the
"__free(...) = NULL" causing cleanup inter-dependency problems is the
following:

---
LIST_HEAD(list);
DEFINE_MUTEX(lock);

struct object {
        struct list_head node;
};

static struct object *alloc_add(void)
{
        struct object *obj;

        lockdep_assert_held(&lock);
        obj = kfree(sizeof(*obj), GFP_KERNEL);
        if (obj) {
                LIST_HEAD_INIT(&obj->node);
		list_add(obj->node, &list):
	}
        return obj;
}

static void remove_free(struct object *obj)
{
        lockdep_assert_held(&lock);
        list_del(&obj->node);
        kfree(obj);
}

DEFINE_FREE(remove_free, struct object *, if (_T) remove_free(_T))
static int init(void)
{
        struct object *obj __free(remove_free) = NULL;
        int err;

        guard(mutex)(lock);
        obj = alloc_add();

        if (!obj)
                return -ENOMEM;

        err = other_init(obj);
        if (err)
                return err; // remove_free() called without the lock!!

        no_free_ptr(obj);
        return 0;
}
---

The fix for this bug is to replace the "__free(...) = NULL" pattern and
move the assignment to the declaration.

        guard(mutex)(lock);
        struct object *obj __free(remove_free) = alloc_add();

..so the compiler can observe LIFO order on the unwind. Yes, no one
should write code like this, all of the init should happen before
assigning to a list, but hopefully it illustrates the point.