mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Mike Isely at pobox <isely@pobox.com>
Cc: mike.isely@cobaltdigital.com, Daniel Scally <djrscally@gmail.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	linux-acpi@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/1] sofware node: Only the managing device can unreference managed software node
Date: Thu, 26 Feb 2026 09:16:47 +0200	[thread overview]
Message-ID: <aZ_zX-jQ7T8LVeSU@smile.fi.intel.com> (raw)
In-Reply-To: <5c5f39f4-9797-4d84-eaf2-3fb7a830cbbf@isely.net>

On Wed, Feb 25, 2026 at 02:16:39PM -0600, Mike Isely wrote:
> On Wed, 25 Feb 2026, Andy Shevchenko wrote:
> > On Wed, Feb 25, 2026 at 01:42:30PM -0600, Mike Isely wrote:
> > > On Wed, 25 Feb 2026, Andy Shevchenko wrote:
> > > > On Tue, Feb 24, 2026 at 01:19:22PM -0600, mike.isely@cobaltdigital.com wrote:

...

> > > > > A scenario exists where device_create_managed_software_node() is used
> > > > > to create an swnode instance that will be implicitly shared to a child
> > > > > device despite best intentions not to permit such sharing (per the
> > > > > comment in device_create_managed_software_node()).  I encountered this
> > > > > with the sfp kernel module when it was instantiated with properties
> > > > 
> > > > SFP? Or is it the name of the actual module in the kernel?
> > > 
> > > Actual kernel module name, sfp.ko, CONFIG_SFP in .config, named after 
> > > the piece of hardware it works with, an SFP cage.  This is logic which 
> > > monitors SFP cages for hotplug appearance / removal of SFP transceivers.  
> > > When a transceiver appears, the sfp kernel module will create a child 
> > > hwmon device instance to monitor various bits of metadata from the 
> > > transceiver.  When that transceiver goes away, the sfp kernel module 
> > > will tear down that child hwmon device instance.
> > > 
> > > The sfp kernel module needs resources configured to know where to 
> > > monitor; in our case that is set up dynamically by another locally 
> > > written kernel module (which iteracts with an FPGA we have where the SFP 
> > > hardware elements reside), and that kernel module will combine 
> > 
> > > devicetree information with some run-time information to generate the 
> > > properties handed off to the sfp kernel module instantiation.
> > 
> > What runtime information? Why this can't be done via DT overlay as others do?
> 
> I don't recall the specifics.  It might be calculation of a unit name.  
> The connectivity in this case is I2C so that should be a constant.  We 
> have some variants where the FPGA is PCIE-connected to the host and so 
> the memory map is a run-time calculation.  We have other drivers that 
> have to be instantiated with run-time computed properties.  So we handle 
> this as a general case.

But the configurations are semi-static, right? For the contents of FPGA we have
a specific manager that reloads the FPGA configuration.

Using swnode for dynamically calculated data seems weird. The data in swnodes
is usually static (const), I can't remember the case where we need to supply
run-time calculated values.

Since that, DT overlay approach seems fine, no?

> > > We use platform_device_register_full() to instantiate it, and that in turn 
> > > causes the swnode instance to be created.  The hwmon child instance 
> > > later created by the sfp module ends up inheriting those resources, 
> > > including the swnode instance, from the sfp parent device, and when the 
> > > hwmon instance is later torn down, we end up with the use-after-free 
> > > problem due to the swnode instance's reference count being incorrectly 
> > > decremented by that child device due to the managed flag being set.
> > > 
> > > If the term SFP is unfamiliar, an explanation can be found here:
> > > 
> > > https://en.wikipedia.org/wiki/Small_Form-factor_Pluggable
> > > 
> > > > > via a call to platform_device_register_full() - it will create hwmon
> > > > > child devices which get all property references.  Unfortunately with
> > > > > just a "managed" boolean in struct swnode handling this, then
> > > > > kobject_put() gets called for the managed aspect when the child device
> > > > > goes away instead of the parent.  This leads to premature freeing of
> > > > > the swnode structure, followed by use-after-free problems, heap
> > > > > corruption, and generally chaos / crashes / misbehavior in the kernel.
> > > > > 
> > > > > This commit changes that boolean into a pointer to the actual managing
> > > > > struct device, which is then checked against the struct device
> > > > > instance that is actually going away (via the usual call back into
> > > > > software_node_notify_remove()).  Thus the child device removal is
> > > > > ignored as it should, and we only do the kobject_put() when the actual
> > > > > managing struct device instance goes away.  We effectively carry a
> > > > > little bit more information now so that we can be sure to clean up
> > > > > only when the correct struct device instance is actually going away.
> > > > > 
> > > > > Note that while we are now keeping a pointer to a struct device here,
> > > > > this is safe to do because the pointer itself only stays in use while
> > > > > the pointed-to device remains valid.  (So no need to be concerned
> > > > > about additional reference counting.)
> > > > 
> > > > The term is called "object lifetime".
> > > 
> > > Meh

...

> Would SoB lines by *both* addresses (even though it's the same person)
> clear that hurdle?

Yes.

-- 
With Best Regards,
Andy Shevchenko



  reply	other threads:[~2026-02-26  7:16 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-24 19:19 [PATCH 0/1] software node: Use-after-free fix in drivers/base/swnode.c mike.isely
2026-02-24 19:19 ` [PATCH 1/1] sofware node: Only the managing device can unreference managed software node mike.isely
2026-02-25 11:22   ` Andy Shevchenko
2026-02-25 19:42     ` Mike Isely
2026-02-25 20:01       ` Andy Shevchenko
2026-02-25 20:16         ` Mike Isely
2026-02-26  7:16           ` Andy Shevchenko [this message]
2026-02-26 19:06             ` Mike Isely
2026-02-26 20:42               ` Andy Shevchenko
2026-02-27 17:55                 ` Mike Isely
2026-02-28 11:02                   ` Andy Shevchenko
2026-02-28 16:34                     ` Mike Isely
2026-02-25  9:46 ` [PATCH 0/1] software node: Use-after-free fix in drivers/base/swnode.c Andy Shevchenko
2026-02-25 18:59   ` Mike Isely
2026-02-25 19:17     ` Andy Shevchenko
2026-02-25 19:48       ` Mike Isely
2026-02-25 20:05         ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aZ_zX-jQ7T8LVeSU@smile.fi.intel.com \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=djrscally@gmail.com \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=isely@pobox.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mike.isely@cobaltdigital.com \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®