| Message ID | 20231217184648.185236-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | of: Automate handling of of_node_put() | expand |
On Sun, Dec 17, 2023 at 06:46:45PM +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > The recent addition of scope based cleanup support to the kernel > provides a convenient tool to reduce the chances of leaking reference > counts where of_node_put() should have been called in an error path. > > This enables > struct device_node *child __free(device_node) = NULL; > > for_each_child_of_node(np, child) { > if (test) > return test; > } > > with no need for a manual call of of_node_put() > > In this simile example the gains are small but there are some very typo > complex error handling cases burried in these loops that wil be > greatly simplified by enabling early returns with out the need > for this manual of_node_put() call. Neat! I guess that now that the coccinelle check has fixed many, we can update it to the new way and start fixing them all again. We should update the coccinelle script with the new way. See scripts/coccinelle/iterators/for_each_child.cocci. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > include/linux/of.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/of.h b/include/linux/of.h > index 6a9ddf20e79a..50e882ee91da 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -13,6 +13,7 @@ > */ > #include <linux/types.h> > #include <linux/bitops.h> > +#include <linux/cleanup.h> > #include <linux/errno.h> > #include <linux/kobject.h> > #include <linux/mod_devicetable.h> > @@ -134,6 +135,7 @@ static inline struct device_node *of_node_get(struct device_node *node) > } > static inline void of_node_put(struct device_node *node) { } > #endif /* !CONFIG_OF_DYNAMIC */ > +DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T)) of_node_put() can be called with NULL, so do we need the "if (_T)"? Rob
On Wed, 20 Dec 2023 16:11:44 -0600 Rob Herring <robh@kernel.org> wrote: > On Sun, Dec 17, 2023 at 06:46:45PM +0000, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > The recent addition of scope based cleanup support to the kernel > > provides a convenient tool to reduce the chances of leaking reference > > counts where of_node_put() should have been called in an error path. > > > > This enables > > struct device_node *child __free(device_node) = NULL; > > > > for_each_child_of_node(np, child) { > > if (test) > > return test; > > } > > > > with no need for a manual call of of_node_put() > > > > In this simile example the gains are small but there are some very > > typo > > > complex error handling cases burried in these loops that wil be > > greatly simplified by enabling early returns with out the need > > for this manual of_node_put() call. > > Neat! > > I guess that now that the coccinelle check has fixed many, we can update > it to the new way and start fixing them all again. We should update the > coccinelle script with the new way. See > scripts/coccinelle/iterators/for_each_child.cocci. If the holiday season gets dull enough I'll take a look at updating that as well. Been a long time since I last messed with coccinelle. Given this is just a simplification rather than a fix, there would be no rush to convert things over but we definitely don't want the coccinelle script to generate lots of false positives. + we should perhaps add a script to try and catch the opposite (double free) as a result of using this automated cleanup. > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > include/linux/of.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/include/linux/of.h b/include/linux/of.h > > index 6a9ddf20e79a..50e882ee91da 100644 > > --- a/include/linux/of.h > > +++ b/include/linux/of.h > > @@ -13,6 +13,7 @@ > > */ > > #include <linux/types.h> > > #include <linux/bitops.h> > > +#include <linux/cleanup.h> > > #include <linux/errno.h> > > #include <linux/kobject.h> > > #include <linux/mod_devicetable.h> > > @@ -134,6 +135,7 @@ static inline struct device_node *of_node_get(struct device_node *node) > > } > > static inline void of_node_put(struct device_node *node) { } > > #endif /* !CONFIG_OF_DYNAMIC */ > > +DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T)) > > of_node_put() can be called with NULL, so do we need the "if (_T)"? Nope - should be fine to call it without. I was being lazy and didn't check :) > > Rob
> > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > --- > > > include/linux/of.h | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/include/linux/of.h b/include/linux/of.h > > > index 6a9ddf20e79a..50e882ee91da 100644 > > > --- a/include/linux/of.h > > > +++ b/include/linux/of.h > > > @@ -13,6 +13,7 @@ > > > */ > > > #include <linux/types.h> > > > #include <linux/bitops.h> > > > +#include <linux/cleanup.h> > > > #include <linux/errno.h> > > > #include <linux/kobject.h> > > > #include <linux/mod_devicetable.h> > > > @@ -134,6 +135,7 @@ static inline struct device_node *of_node_get(struct device_node *node) > > > } > > > static inline void of_node_put(struct device_node *node) { } > > > #endif /* !CONFIG_OF_DYNAMIC */ > > > +DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T)) > > > > of_node_put() can be called with NULL, so do we need the "if (_T)"? > > Nope - should be fine to call it without. I was being lazy and didn't check :) Seems there has been a lot of discussion of this for similar cases and consensus is to keep the if (_T) e.g. https://lore.kernel.org/all/6596edda327c8_8dc68294b2@dwillia2-xfh.jf.intel.com.notmuch/ > > > > > Rob > >
On Thu, 21 Dec 2023 10:54:34 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Wed, 20 Dec 2023 16:11:44 -0600 > Rob Herring <robh@kernel.org> wrote: > > > On Sun, Dec 17, 2023 at 06:46:45PM +0000, Jonathan Cameron wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > The recent addition of scope based cleanup support to the kernel > > > provides a convenient tool to reduce the chances of leaking reference > > > counts where of_node_put() should have been called in an error path. > > > > > > This enables > > > struct device_node *child __free(device_node) = NULL; > > > > > > for_each_child_of_node(np, child) { > > > if (test) > > > return test; > > > } > > > > > > with no need for a manual call of of_node_put() > > > > > > In this simile example the gains are small but there are some very > > > > typo > > > > > complex error handling cases burried in these loops that wil be > > > greatly simplified by enabling early returns with out the need > > > for this manual of_node_put() call. > > > > Neat! > > > > I guess that now that the coccinelle check has fixed many, we can update > > it to the new way and start fixing them all again. We should update the > > coccinelle script with the new way. See > > scripts/coccinelle/iterators/for_each_child.cocci. > > If the holiday season gets dull enough I'll take a look at updating that > as well. Been a long time since I last messed with coccinelle. > > Given this is just a simplification rather than a fix, there would be no rush > to convert things over but we definitely don't want the coccinelle script > to generate lots of false positives. + we should perhaps add a > script to try and catch the opposite (double free) as a result of > using this automated cleanup. Hi Rob, As things currently stand the script doesn't trigger on a struct device_node __free(device_node); (which is wrong anyway) or struct device_node __free(device_node) = NULL; So we at least don't cause a flurry of false positives via these changes. I'm not keen to add an upstream check to encourage conversion over to this new approach simply because there is no great rush to do it and it's easy enough to use grep to find potential targets today. Also strongly motivated by the fact I don't really have time to learn coccinelle (however useful that would be in the long run!) As such I'll tidy these up a bit and send out a non RFC version with cover letter additions to mention we don't cause false positives and that a coccinelle script to find candidates might make sense in the longer term. It may also make sense to add checks that we don't manually release the node on error paths without making sure to steal the pointer (which sets it to NULL to avoid problems). +CC various Coccinelle folk even though I'm proposing to not do any coccinelle scripting for now. Jonathan
diff --git a/include/linux/of.h b/include/linux/of.h index 6a9ddf20e79a..50e882ee91da 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -13,6 +13,7 @@ */ #include <linux/types.h> #include <linux/bitops.h> +#include <linux/cleanup.h> #include <linux/errno.h> #include <linux/kobject.h> #include <linux/mod_devicetable.h> @@ -134,6 +135,7 @@ static inline struct device_node *of_node_get(struct device_node *node) } static inline void of_node_put(struct device_node *node) { } #endif /* !CONFIG_OF_DYNAMIC */ +DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T)) /* Pointer for first entry in chain of all nodes. */ extern struct device_node *of_root;