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

[PATCH v2 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>
---
Changes in v2:
- added printing offending propety name in patch #3 (Andy)
- Link to v1: https://patch.msgid.link/20260323-property-gpio-fix-v1-0-9cb46e5fe7df@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                  | 10 ++++++++++
 include/linux/property.h               |  9 ++++++++-
 3 files changed, 36 insertions(+), 7 deletions(-)
---
base-commit: 3b058d1aeeeff27a7289529c4944291613b364e9
change-id: 20260315-property-gpio-fix-51586cffcd5d

Thanks.

-- 
Dmitry

[PATCH v2 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 v2 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

[PATCH v2 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 51320837f3a9..286eab02e8fb 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,14 @@ 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 ('%s')\n",
+			       __func__, prop->name);
+			return -EINVAL;
+		}
+	}
+
 	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0));
 }
 EXPORT_SYMBOL_GPL(software_node_register);

-- 
2.53.0.1018.g2bb0e51243-goog

[PATCH v2 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

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

[Andy Shevchenko]

On Sun, Mar 29, 2026 at 07:27:49PM -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

PROPERTY_ENTRY_REF()

> 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.

...

> +#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__))

Make it a single line

		 default: &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__))

AFAICS it has no continuation and fits the given format well.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Mar 29, 2026 at 07:27:50PM -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.

...

> +	for (prop = node->properties; prop && prop->name; prop++) {
> +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {

I read more about this... Any code that uses vmalloc() (or potentially may
switch to it from regular allocator with help of kvalloc() and similar) will
fail now. While it might be no issue right now, this may become a such. So
with this check in place you put a requirement that properties can only be
allocated from a kernel low memory heap and not vm.

> +			pr_err("%s: property data can't be on stack ('%s')\n",
> +			       __func__, prop->name);
> +			return -EINVAL;
> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Mar 29, 2026 at 07:27:51PM -0700, Dmitry Torokhov wrote:
> Now that the PROPERTY_ENTRY_REF macro can accept a pointer to struct

PROPERTY_ENTRY_REF()

> software_node_ref_args directly, we don't need to use the more
> cumbersome PROPERTY_ENTRY_REF_ARRAY_LEN(..., 1) variant.

...

> int __init geode_create_leds(const char *label, const struct geode_led *leds,

>  						       leds[i].pin,
>  						       GPIO_ACTIVE_LOW);

Wondering if we want to have a wrapper for the above like

#define PROPERTY_ENTRY_GPIO_REF(...)	\
	SOFTWARE_NODE_REFERENCE(...)

(naming most likely sucks, and yeah, it would make sense if name is shorter
 than the currently used one).

>  		props[i * 3 + 0] =
> -			PROPERTY_ENTRY_REF_ARRAY_LEN("gpios", &gpio_refs[i], 1);
> +			PROPERTY_ENTRY_REF("gpios", &gpio_refs[i]);

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Torokhov]

On Mon, Mar 30, 2026 at 01:33:47PM +0300, Andy Shevchenko wrote:
> On Sun, Mar 29, 2026 at 07:27:50PM -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.
> 
> ...
> 
> > +	for (prop = node->properties; prop && prop->name; prop++) {
> > +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> 
> I read more about this... Any code that uses vmalloc() (or potentially may
> switch to it from regular allocator with help of kvalloc() and similar) will
> fail now. While it might be no issue right now, this may become a such. So
> with this check in place you put a requirement that properties can only be
> allocated from a kernel low memory heap and not vm.

Can you tell me more about this? As far as I can see it will actually
have false negatives with CONFIG_VMAP_STACK, but should be OK not
trigger with vmalloced memory... But I am genuinely interested to know
more.

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Sun, Mar 29, 2026 at 07:27:48PM -0700, Dmitry Torokhov wrote:
> 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>

While we are discussing with Andy patches 2-4 maybe this one can be
picked up? It does fix (I hope)(I hope)  a real issue in the field.

Thanks.

-- 
Dmitry

[tip: x86/urgent] x86/platform/geode: Fix on-stack property data use-after-return bug

[tip-bot2 for Dmitry Torokhov]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     b981e9e94c687b7b19ae8820963f005b842cb2f2
Gitweb:        https://git.kernel.org/tip/b981e9e94c687b7b19ae8820963f005b842cb2f2
Author:        Dmitry Torokhov <dmitry.torokhov@gmail.com>
AuthorDate:    Sun, 29 Mar 2026 19:27:48 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 31 Mar 2026 09:55:26 +02:00

x86/platform/geode: Fix on-stack property data use-after-return bug

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")
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Daniel Scally <djrscally@gmail.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Hans de Goede <hansg@kernel.org>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: stable@vger.kernel.org
Link: https://patch.msgid.link/20260329-property-gpio-fix-v2-1-3cca5ba136d8@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 05189c5..1843ae3 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 @@ err_unregister_group:
 err_free_names:
 	while (--i >= 0)
 		kfree(swnodes[i].name);
+	kfree(gpio_refs);
+err_free_props:
 	kfree(props);
 err_free_swnodes:
 	kfree(swnodes);

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

[Andy Shevchenko]

On Mon, Mar 30, 2026 at 02:49:52PM -0700, Dmitry Torokhov wrote:
> On Mon, Mar 30, 2026 at 01:33:47PM +0300, Andy Shevchenko wrote:
> > On Sun, Mar 29, 2026 at 07:27:50PM -0700, Dmitry Torokhov wrote:

...

> > > +	for (prop = node->properties; prop && prop->name; prop++) {
> > > +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> > 
> > I read more about this... Any code that uses vmalloc() (or potentially may
> > switch to it from regular allocator with help of kvalloc() and similar) will
> > fail now. While it might be no issue right now, this may become a such. So
> > with this check in place you put a requirement that properties can only be
> > allocated from a kernel low memory heap and not vm.
> 
> Can you tell me more about this? As far as I can see it will actually
> have false negatives with CONFIG_VMAP_STACK, but should be OK not
> trigger with vmalloced memory... But I am genuinely interested to know
> more.

I dug into the history of this macro. It was added for the block and ide
subsystems to make sure that there is no buffer supplied that may not be DMAed.
As we know vmalloc():ed buffers may not be DMAed. In some commit messages
it was explicitly mentioned that this macro fails on vmalloc():ed memory.

Note, I haven't checked the actual behaviour by trying that on the HW.

-- 
With Best Regards,
Andy Shevchenko

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

[Ingo Molnar]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Sun, Mar 29, 2026 at 07:27:48PM -0700, Dmitry Torokhov wrote:
> > 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>
> 
> While we are discussing with Andy patches 2-4 maybe this one can be
> picked up? It does fix (I hope)(I hope)  a real issue in the field.

Agreed, I've queued it up in tip:x86/urgent.

Thanks,

	Ingo

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

[Andy Shevchenko]

On Tue, Mar 31, 2026 at 11:02:13AM +0300, Andy Shevchenko wrote:
> On Mon, Mar 30, 2026 at 02:49:52PM -0700, Dmitry Torokhov wrote:
> > On Mon, Mar 30, 2026 at 01:33:47PM +0300, Andy Shevchenko wrote:
> > > On Sun, Mar 29, 2026 at 07:27:50PM -0700, Dmitry Torokhov wrote:

...

> > > > +	for (prop = node->properties; prop && prop->name; prop++) {
> > > > +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> > > 
> > > I read more about this... Any code that uses vmalloc() (or potentially may
> > > switch to it from regular allocator with help of kvalloc() and similar) will
> > > fail now. While it might be no issue right now, this may become a such. So
> > > with this check in place you put a requirement that properties can only be
> > > allocated from a kernel low memory heap and not vm.
> > 
> > Can you tell me more about this? As far as I can see it will actually
> > have false negatives with CONFIG_VMAP_STACK, but should be OK not
> > trigger with vmalloced memory... But I am genuinely interested to know
> > more.
> 
> I dug into the history of this macro. It was added for the block and ide
> subsystems to make sure that there is no buffer supplied that may not be DMAed.
> As we know vmalloc():ed buffers may not be DMAed. In some commit messages
> it was explicitly mentioned that this macro fails on vmalloc():ed memory.
> 
> Note, I haven't checked the actual behaviour by trying that on the HW.

OTOH, the check itself covers only 16kB of memory range. I don't understand
how it can give true for anything outside that area...

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Torokhov]

On Tue, Mar 31, 2026 at 11:33:10AM +0300, Andy Shevchenko wrote:
> On Tue, Mar 31, 2026 at 11:02:13AM +0300, Andy Shevchenko wrote:
> > On Mon, Mar 30, 2026 at 02:49:52PM -0700, Dmitry Torokhov wrote:
> > > On Mon, Mar 30, 2026 at 01:33:47PM +0300, Andy Shevchenko wrote:
> > > > On Sun, Mar 29, 2026 at 07:27:50PM -0700, Dmitry Torokhov wrote:
> 
> ...
> 
> > > > > +	for (prop = node->properties; prop && prop->name; prop++) {
> > > > > +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> > > > 
> > > > I read more about this... Any code that uses vmalloc() (or potentially may
> > > > switch to it from regular allocator with help of kvalloc() and similar) will
> > > > fail now. While it might be no issue right now, this may become a such. So
> > > > with this check in place you put a requirement that properties can only be
> > > > allocated from a kernel low memory heap and not vm.
> > > 
> > > Can you tell me more about this? As far as I can see it will actually
> > > have false negatives with CONFIG_VMAP_STACK, but should be OK not
> > > trigger with vmalloced memory... But I am genuinely interested to know
> > > more.
> > 
> > I dug into the history of this macro. It was added for the block and ide
> > subsystems to make sure that there is no buffer supplied that may not be DMAed.
> > As we know vmalloc():ed buffers may not be DMAed. In some commit messages
> > it was explicitly mentioned that this macro fails on vmalloc():ed memory.
> > 
> > Note, I haven't checked the actual behaviour by trying that on the HW.
> 
> OTOH, the check itself covers only 16kB of memory range. I don't understand
> how it can give true for anything outside that area...
> 

You probably mean b4a0f533e597 ("dma-api: Teach the "DMA-from-stack"
check about vmapped stacks") but it says that for vmapped stacks
object_is_on_stack() will produce false negative (which is tolerable
here).

I am not sure why object_is_on_stack() was not extended to also handle
this. From the cursory glance it looks like a lot of callsites do not
detect vmapped stacks... Let's add Andy Lutomirski...

Thanks.


-- 
Dmitry

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

[Dmitry Torokhov]

On Sat, Apr 04, 2026 at 08:27:58PM -0700, Dmitry Torokhov wrote:
> On Tue, Mar 31, 2026 at 11:33:10AM +0300, Andy Shevchenko wrote:
> > On Tue, Mar 31, 2026 at 11:02:13AM +0300, Andy Shevchenko wrote:
> > > On Mon, Mar 30, 2026 at 02:49:52PM -0700, Dmitry Torokhov wrote:
> > > > On Mon, Mar 30, 2026 at 01:33:47PM +0300, Andy Shevchenko wrote:
> > > > > On Sun, Mar 29, 2026 at 07:27:50PM -0700, Dmitry Torokhov wrote:
> > 
> > ...
> > 
> > > > > > +	for (prop = node->properties; prop && prop->name; prop++) {
> > > > > > +		if (!prop->is_inline && object_is_on_stack(prop->pointer)) {
> > > > > 
> > > > > I read more about this... Any code that uses vmalloc() (or potentially may
> > > > > switch to it from regular allocator with help of kvalloc() and similar) will
> > > > > fail now. While it might be no issue right now, this may become a such. So
> > > > > with this check in place you put a requirement that properties can only be
> > > > > allocated from a kernel low memory heap and not vm.
> > > > 
> > > > Can you tell me more about this? As far as I can see it will actually
> > > > have false negatives with CONFIG_VMAP_STACK, but should be OK not
> > > > trigger with vmalloced memory... But I am genuinely interested to know
> > > > more.
> > > 
> > > I dug into the history of this macro. It was added for the block and ide
> > > subsystems to make sure that there is no buffer supplied that may not be DMAed.
> > > As we know vmalloc():ed buffers may not be DMAed. In some commit messages
> > > it was explicitly mentioned that this macro fails on vmalloc():ed memory.
> > > 
> > > Note, I haven't checked the actual behaviour by trying that on the HW.
> > 
> > OTOH, the check itself covers only 16kB of memory range. I don't understand
> > how it can give true for anything outside that area...
> > 
> 
> You probably mean b4a0f533e597 ("dma-api: Teach the "DMA-from-stack"
> check about vmapped stacks") but it says that for vmapped stacks
> object_is_on_stack() will produce false negative (which is tolerable
> here).
> 
> I am not sure why object_is_on_stack() was not extended to also handle
> this. From the cursory glance it looks like a lot of callsites do not
> detect vmapped stacks... Let's add Andy Lutomirski...

Grr... now for real...

-- 
Dmitry