Hello Kuppuswamy, Dan,
On Fri, Mar 22, 2024 at 10:03:12AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > +
> > static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
> > enum pci_barno barno)
> > {
> > - int j;
> > - u32 val;
> > - int size;
> > + int j, bar_size, buf_size, iters, remain;
> > + void *write_buf __free(kfree) = NULL;
> > + void *read_buf __free(kfree) = NULL;
>
> Please check the following cleanup doc. Recommendation is to avoid above __free(kfree) = NULL declarations and directly define write_buf/read_buf.
>
> https://lore.kernel.org/lkml/171097196970.1011049.9726486429680041876.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/T/#m05d71a668ff0fad46c2055dbdcde55d7381780b7
I don't think that the docs say that using:
void *ptr __free(kfree) = NULL;
is always an anti-pattern.
"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."
I think it says that it is an anti-pattern IFF there are two cleanup
helpers in a function AND they have have a inter-dependency.
This patch just adds a single cleanup helper, so there can be no
inter-dependency problem.
This pattern is common in Linus's current tree, see e.g.
$ git grep -C 3 -E "__free(.*) = NULL"
If this was a problem, I don't think we would have seen
100 different instances of this pattern.
In other words, I think this patch is fine.
Dan, please correct me if I'm wrong.
Kind regards,
Niklas
