[v1] Fix handling of GPIO keys and LEDs on geode

[PATCH 3/4] software node: verify that property data is not on stack

Posted by Dmitry Torokhov 1 week, 5 days ago

When registering a software node, ensure that the property data is not
located on the stack, as it is expected to persist for the lifetime of
the node.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 51320837f3a9..45c319a08eb6 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -16,6 +16,7 @@
 #include <linux/kstrtox.h>
 #include <linux/list.h>
 #include <linux/property.h>
+#include <linux/sched/task_stack.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
@@ -917,6 +918,7 @@ EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
 int software_node_register(const struct software_node *node)
 {
 	struct swnode *parent = software_node_to_swnode(node->parent);
+	const struct property_entry *prop;
 
 	if (software_node_to_swnode(node))
 		return -EEXIST;
@@ -924,6 +926,13 @@ int software_node_register(const struct software_node *node)
 	if (node->parent && !parent)
 		return -EINVAL;
 
+	for (prop = node->properties; prop && prop->name; prop++) {
+		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
+			pr_err("%s: property data can't be on stack\n", __func__);
+			return -EINVAL;
+		}
+	}
+
 	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0));
 }
 EXPORT_SYMBOL_GPL(software_node_register);

-- 
2.53.0.1018.g2bb0e51243-goog

Re: [PATCH 3/4] software node: verify that property data is not on stack

Posted by Andy Shevchenko 1 week, 5 days ago

On Mon, Mar 23, 2026 at 05:39:39PM -0700, Dmitry Torokhov wrote:
> When registering a software node, ensure that the property data is not
> located on the stack, as it is expected to persist for the lifetime of
> the node.

...

> +#include <linux/sched/task_stack.h>

Looking at this name the use of it here doesn't sound right...

...

> +	for (prop = node->properties; prop && prop->name; prop++) {
> +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> +			pr_err("%s: property data can't be on stack\n", __func__);
> +			return -EINVAL;
> +		}
> +	}

And again same question, why we can't simply dup them as we do for (string)
arrays?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] software node: verify that property data is not on stack

Posted by Dmitry Torokhov 1 week, 5 days ago

On Tue, Mar 24, 2026 at 02:33:31PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 23, 2026 at 05:39:39PM -0700, Dmitry Torokhov wrote:
> > When registering a software node, ensure that the property data is not
> > located on the stack, as it is expected to persist for the lifetime of
> > the node.
> 
> ...
> 
> > +#include <linux/sched/task_stack.h>
> 
> Looking at this name the use of it here doesn't sound right...

Because ... ? object_is_on_stack() is defined there.

> 
> ...
> 
> > +	for (prop = node->properties; prop && prop->name; prop++) {
> > +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> > +			pr_err("%s: property data can't be on stack\n", __func__);
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> And again same question, why we can't simply dup them as we do for (string)
> arrays?

Because majority of users of software_node_register() deal with static
const properties so duping them is waste of memory.

Thanks.

-- 
Dmitry

Re: [PATCH 3/4] software node: verify that property data is not on stack

Posted by Andy Shevchenko 1 week, 4 days ago

On Tue, Mar 24, 2026 at 09:17:18AM -0700, Dmitry Torokhov wrote:
> On Tue, Mar 24, 2026 at 02:33:31PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 23, 2026 at 05:39:39PM -0700, Dmitry Torokhov wrote:
> > > When registering a software node, ensure that the property data is not
> > > located on the stack, as it is expected to persist for the lifetime of
> > > the node.

...

> > > +#include <linux/sched/task_stack.h>
> > 
> > Looking at this name the use of it here doesn't sound right...
> 
> Because ... ? object_is_on_stack() is defined there.

Because it's defined there. The key word here I see is 'task' in the name of
the header, without Ack from sched folks, I am not convinced we can use that at
any point in the kernel.

...

> > > +	for (prop = node->properties; prop && prop->name; prop++) {
> > > +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> > > +			pr_err("%s: property data can't be on stack\n", __func__);
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > 
> > And again same question, why we can't simply dup them as we do for (string)
> > arrays?
> 
> Because majority of users of software_node_register() deal with static
> const properties so duping them is waste of memory.

So, then why can't we do that for the references? Just then copy and free at
software unregistration if required.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] software node: verify that property data is not on stack

Posted by Dmitry Torokhov 1 week, 4 days ago

On Wed, Mar 25, 2026 at 01:51:02PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 24, 2026 at 09:17:18AM -0700, Dmitry Torokhov wrote:
> > On Tue, Mar 24, 2026 at 02:33:31PM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 23, 2026 at 05:39:39PM -0700, Dmitry Torokhov wrote:
> > > > When registering a software node, ensure that the property data is not
> > > > located on the stack, as it is expected to persist for the lifetime of
> > > > the node.
> 
> ...
> 
> > > > +#include <linux/sched/task_stack.h>
> > > 
> > > Looking at this name the use of it here doesn't sound right...
> > 
> > Because ... ? object_is_on_stack() is defined there.
> 
> Because it's defined there. The key word here I see is 'task' in the name of
> the header, without Ack from sched folks, I am not convinced we can use that at
> any point in the kernel.

It is fine to be used in drivers, and it is used in USB
(drivers/usb/core/hcd.c), SPI (drivers/spi/spi-mem.c) and others.

"task" is not magical, it simply refers to a process or a thread (user
or kernel). This particular macro just checks if the object is within
limits of stack area of currently executing thread.

> 
> ...
> 
> > > > +	for (prop = node->properties; prop && prop->name; prop++) {
> > > > +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> > > > +			pr_err("%s: property data can't be on stack\n", __func__);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	}
> > > 
> > > And again same question, why we can't simply dup them as we do for (string)
> > > arrays?
> > 
> > Because majority of users of software_node_register() deal with static
> > const properties so duping them is waste of memory.
> 
> So, then why can't we do that for the references? Just then copy and free at
> software unregistration if required.

We do not want to copy and free because that would be waste of memory in
majority of cases where software_node_register() is used.

When dealing with other kinds of properties it is pretty obvious where
the objectr itself lives, but when using PROPERTY_ENTRY_REF() and
PROPERTY_ENTYR_GPIO() it is very easy to miss (I did! and I made them)
that the reference args object is on stack as it is hidden behind a
macro.

So it is just a safeguard warning us ahead of time instead of needing to
figure out why a driver can't find GPIO even through the reference looks
right.

Thanks.

-- 
Dmitry

Re: [PATCH 3/4] software node: verify that property data is not on stack

Posted by Andy Shevchenko 1 week, 3 days ago

On Wed, Mar 25, 2026 at 09:24:57AM -0700, Dmitry Torokhov wrote:
> On Wed, Mar 25, 2026 at 01:51:02PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 24, 2026 at 09:17:18AM -0700, Dmitry Torokhov wrote:
> > > On Tue, Mar 24, 2026 at 02:33:31PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Mar 23, 2026 at 05:39:39PM -0700, Dmitry Torokhov wrote:

...

> > > > > +#include <linux/sched/task_stack.h>
> > > > 
> > > > Looking at this name the use of it here doesn't sound right...
> > > 
> > > Because ... ? object_is_on_stack() is defined there.
> > 
> > Because it's defined there. The key word here I see is 'task' in the name of
> > the header, without Ack from sched folks, I am not convinced we can use that at
> > any point in the kernel.
> 
> It is fine to be used in drivers, and it is used in USB
> (drivers/usb/core/hcd.c), SPI (drivers/spi/spi-mem.c) and others.
> 
> "task" is not magical, it simply refers to a process or a thread (user
> or kernel). This particular macro just checks if the object is within
> limits of stack area of currently executing thread.

Still, it's C which can't really protect the public headers from misuse.

...

> > > > > +	for (prop = node->properties; prop && prop->name; prop++) {
> > > > > +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> > > > > +			pr_err("%s: property data can't be on stack\n", __func__);
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +	}
> > > > 
> > > > And again same question, why we can't simply dup them as we do for (string)
> > > > arrays?
> > > 
> > > Because majority of users of software_node_register() deal with static
> > > const properties so duping them is waste of memory.
> > 
> > So, then why can't we do that for the references? Just then copy and free at
> > software unregistration if required.
> 
> We do not want to copy and free because that would be waste of memory in
> majority of cases where software_node_register() is used.
> 
> When dealing with other kinds of properties it is pretty obvious where
> the objectr itself lives, but when using PROPERTY_ENTRY_REF() and
> PROPERTY_ENTYR_GPIO() it is very easy to miss (I did! and I made them)
> that the reference args object is on stack as it is hidden behind a
> macro.
> 
> So it is just a safeguard warning us ahead of time instead of needing to
> figure out why a driver can't find GPIO even through the reference looks
> right.

Okay, but in the current form it lacks necessary information for understanding,
exempli gratia the property name.

-- 
With Best Regards,
Andy Shevchenko