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