[PATCH 0/4] Fix handling of GPIO keys and LEDs on geode

[PATCH 0/4] Fix handling of GPIO keys and LEDs on geode

[Dmitry Torokhov]

This series deal with breakage on geode caused by a recent conversion of
the board to use static device properties for configuring GPIO-connected
keys and LEDs. The issue was that PROPERTY_ENTRY_GPIO() would create a
temporary structure on stack for GPIO properties which would later be
discarded.

The first change patches the behavior using existing in kernel APIs so
that the bug can easily be fixed in stable kernels, and the other 3
improve the API and add safety checks.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Dmitry Torokhov (4):
      x86/geode: fix on-stack property data usage
      software node: allow passing reference args to PROPERTY_ENTRY_REF
      software node: verify that property data is not on stack
      x86/geode: use PROPERTY_ENTRY_REF for GPIO properties

 arch/x86/platform/geode/geode-common.c | 24 ++++++++++++++++++------
 drivers/base/swnode.c                  |  9 +++++++++
 include/linux/property.h               |  9 ++++++++-
 3 files changed, 35 insertions(+), 7 deletions(-)
---
base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900
change-id: 20260315-property-gpio-fix-51586cffcd5d

Thanks.

-- 
Dmitry

[PATCH 1/4] x86/geode: fix on-stack property data usage

[Dmitry Torokhov]

The PROPERTY_ENTRY_GPIO macro (and by extension PROPERTY_ENTRY_REF)
creates a temporary software_node_ref_args structure on the stack
when used in a runtime assignment. This results in the property
pointing to data that is invalid once the function returns.

Fix this by ensuring the GPIO reference data is not stored on stack and
using PROPERTY_ENTRY_REF_ARRAY_LEN() to point directly to the persistent
reference data.

Fixes: 298c9babadb8 ("x86/platform/geode: switch GPIO buttons and LEDs to software properties")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 arch/x86/platform/geode/geode-common.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/x86/platform/geode/geode-common.c b/arch/x86/platform/geode/geode-common.c
index 05189c5f7d2a..1843ae385e2d 100644
--- a/arch/x86/platform/geode/geode-common.c
+++ b/arch/x86/platform/geode/geode-common.c
@@ -28,8 +28,10 @@ static const struct software_node geode_gpio_keys_node = {
 	.properties = geode_gpio_keys_props,
 };
 
-static struct property_entry geode_restart_key_props[] = {
-	{ /* Placeholder for GPIO property */ },
+static struct software_node_ref_args geode_restart_gpio_ref;
+
+static const struct property_entry geode_restart_key_props[] = {
+	PROPERTY_ENTRY_REF_ARRAY_LEN("gpios", &geode_restart_gpio_ref, 1),
 	PROPERTY_ENTRY_U32("linux,code", KEY_RESTART),
 	PROPERTY_ENTRY_STRING("label", "Reset button"),
 	PROPERTY_ENTRY_U32("debounce-interval", 100),
@@ -64,8 +66,7 @@ int __init geode_create_restart_key(unsigned int pin)
 	struct platform_device *pd;
 	int err;
 
-	geode_restart_key_props[0] = PROPERTY_ENTRY_GPIO("gpios",
-							 &geode_gpiochip_node,
+	geode_restart_gpio_ref = SOFTWARE_NODE_REFERENCE(&geode_gpiochip_node,
 							 pin, GPIO_ACTIVE_LOW);
 
 	err = software_node_register_node_group(geode_gpio_keys_swnodes);
@@ -99,6 +100,7 @@ int __init geode_create_leds(const char *label, const struct geode_led *leds,
 	const struct software_node *group[MAX_LEDS + 2] = { 0 };
 	struct software_node *swnodes;
 	struct property_entry *props;
+	struct software_node_ref_args *gpio_refs;
 	struct platform_device_info led_info = {
 		.name	= "leds-gpio",
 		.id	= PLATFORM_DEVID_NONE,
@@ -127,6 +129,12 @@ int __init geode_create_leds(const char *label, const struct geode_led *leds,
 		goto err_free_swnodes;
 	}
 
+	gpio_refs = kzalloc_objs(*gpio_refs, n_leds);
+	if (!gpio_refs) {
+		err = -ENOMEM;
+		goto err_free_props;
+	}
+
 	group[0] = &geode_gpio_leds_node;
 	for (i = 0; i < n_leds; i++) {
 		node_name = kasprintf(GFP_KERNEL, "%s:%d", label, i);
@@ -135,9 +143,11 @@ int __init geode_create_leds(const char *label, const struct geode_led *leds,
 			goto err_free_names;
 		}
 
+		gpio_refs[i] = SOFTWARE_NODE_REFERENCE(&geode_gpiochip_node,
+						       leds[i].pin,
+						       GPIO_ACTIVE_LOW);
 		props[i * 3 + 0] =
-			PROPERTY_ENTRY_GPIO("gpios", &geode_gpiochip_node,
-					    leds[i].pin, GPIO_ACTIVE_LOW);
+			PROPERTY_ENTRY_REF_ARRAY_LEN("gpios", &gpio_refs[i], 1);
 		props[i * 3 + 1] =
 			PROPERTY_ENTRY_STRING("linux,default-trigger",
 					      leds[i].default_on ?
@@ -171,6 +181,8 @@ int __init geode_create_leds(const char *label, const struct geode_led *leds,
 err_free_names:
 	while (--i >= 0)
 		kfree(swnodes[i].name);
+	kfree(gpio_refs);
+err_free_props:
 	kfree(props);
 err_free_swnodes:
 	kfree(swnodes);

-- 
2.53.0.1018.g2bb0e51243-goog

[PATCH 2/4] software node: allow passing reference args to PROPERTY_ENTRY_REF

[Dmitry Torokhov]

When dynamically creating software nodes and properties for subsequent
use with software_node_register() current implementation of
PROPERTY_ENTRY_REF is not suitable because it creates a temporary
instance of struct software_node_ref_args on stack which will later
disappear, and software_node_register() only does shallow copy of
properties.

Fix this by allowing to pass address of reference arguments structure
directly into PROPERTY_ENTRY_REF(), so that caller can manage lifetime
of the object properly.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 include/linux/property.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/property.h b/include/linux/property.h
index e30ef23a9af3..942657e76993 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -471,12 +471,19 @@ struct property_entry {
 #define PROPERTY_ENTRY_STRING(_name_, _val_)				\
 	__PROPERTY_ENTRY_ELEMENT(_name_, str, STRING, _val_)
 
+#define __PROPERTY_ENTRY_REF_ARGS(_ref_, ...)				\
+	_Generic(_ref_,							\
+		 const struct software_node_ref_args *: _ref_,		\
+		 struct software_node_ref_args *: _ref_,		\
+		 default: &SOFTWARE_NODE_REFERENCE(_ref_,		\
+						   ##__VA_ARGS__))
+
 #define PROPERTY_ENTRY_REF(_name_, _ref_, ...)				\
 (struct property_entry) {						\
 	.name = _name_,							\
 	.length = sizeof(struct software_node_ref_args),		\
 	.type = DEV_PROP_REF,						\
-	{ .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), },	\
+	{ .pointer = __PROPERTY_ENTRY_REF_ARGS(_ref_, ##__VA_ARGS__) },	\
 }
 
 #define PROPERTY_ENTRY_BOOL(_name_)		\

-- 
2.53.0.1018.g2bb0e51243-goog

Re: [PATCH 2/4] software node: allow passing reference args to PROPERTY_ENTRY_REF

[Andy Shevchenko]

On Mon, Mar 23, 2026 at 05:39:38PM -0700, Dmitry Torokhov wrote:
> When dynamically creating software nodes and properties for subsequent
> use with software_node_register() current implementation of
> PROPERTY_ENTRY_REF is not suitable because it creates a temporary
> instance of struct software_node_ref_args on stack which will later
> disappear, and software_node_register() only does shallow copy of
> properties.

Isn't that the problem and we should take into account how to dup the reference
inside property core?

> Fix this by allowing to pass address of reference arguments structure
> directly into PROPERTY_ENTRY_REF(), so that caller can manage lifetime
> of the object properly.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] software node: allow passing reference args to PROPERTY_ENTRY_REF

[Dmitry Torokhov]

On Tue, Mar 24, 2026 at 02:30:54PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 23, 2026 at 05:39:38PM -0700, Dmitry Torokhov wrote:
> > When dynamically creating software nodes and properties for subsequent
> > use with software_node_register() current implementation of
> > PROPERTY_ENTRY_REF is not suitable because it creates a temporary
> > instance of struct software_node_ref_args on stack which will later
> > disappear, and software_node_register() only does shallow copy of
> > properties.
> 
> Isn't that the problem and we should take into account how to dup the reference
> inside property core?

Sorry, I actually need to drop the end of that sentence.
software_node_register() expects the caller to maintain the lifetime and
therefore does not do any kind of copying of properties. The majority of
users define static const properties and we do not want to make an extra
copy of them.

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

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

[Andy Shevchenko]

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

[Dmitry Torokhov]

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

[Andy Shevchenko]

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

[Dmitry Torokhov]

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

[Andy Shevchenko]

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

[PATCH 4/4] x86/geode: use PROPERTY_ENTRY_REF for GPIO properties

[Dmitry Torokhov]

Now that the PROPERTY_ENTRY_REF macro can accept a pointer to struct
software_node_ref_args directly, we don't need to use the more
cumbersome PROPERTY_ENTRY_REF_ARRAY_LEN(..., 1) variant.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 arch/x86/platform/geode/geode-common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/geode/geode-common.c b/arch/x86/platform/geode/geode-common.c
index 1843ae385e2d..6572ae388d7d 100644
--- a/arch/x86/platform/geode/geode-common.c
+++ b/arch/x86/platform/geode/geode-common.c
@@ -31,7 +31,7 @@ static const struct software_node geode_gpio_keys_node = {
 static struct software_node_ref_args geode_restart_gpio_ref;
 
 static const struct property_entry geode_restart_key_props[] = {
-	PROPERTY_ENTRY_REF_ARRAY_LEN("gpios", &geode_restart_gpio_ref, 1),
+	PROPERTY_ENTRY_REF("gpios", &geode_restart_gpio_ref),
 	PROPERTY_ENTRY_U32("linux,code", KEY_RESTART),
 	PROPERTY_ENTRY_STRING("label", "Reset button"),
 	PROPERTY_ENTRY_U32("debounce-interval", 100),
@@ -147,7 +147,7 @@ int __init geode_create_leds(const char *label, const struct geode_led *leds,
 						       leds[i].pin,
 						       GPIO_ACTIVE_LOW);
 		props[i * 3 + 0] =
-			PROPERTY_ENTRY_REF_ARRAY_LEN("gpios", &gpio_refs[i], 1);
+			PROPERTY_ENTRY_REF("gpios", &gpio_refs[i]);
 		props[i * 3 + 1] =
 			PROPERTY_ENTRY_STRING("linux,default-trigger",
 					      leds[i].default_on ?

-- 
2.53.0.1018.g2bb0e51243-goog