RE: [PATCH] cleanup: Add usage and style documentation

[Dan Williams <dan.j.williams@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.