Tian, Kevin wrote:
> > From: Dan Williams <dan.j.williams@xxxxxxxxx>
> > 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.
