On 3/24/22 06:15, Greg Kroah-Hartman wrote:
On Wed, Mar 23, 2022 at 02:45:59PM -0500, Pierre-Louis Bossart wrote:Hi, I could use feedback/guidance on a possible conceptual bug in the SoundWire probe and bus handling. When we probe a driver, the code does this: static int sdw_drv_probe(struct device *dev) { struct sdw_slave *slave = dev_to_sdw_dev(dev); struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); const struct sdw_device_id *id; const char *name; int ret; /* * fw description is mandatory to bind */ if (!dev->fwnode) return -ENODEV; if (!IS_ENABLED(CONFIG_ACPI) && !dev->of_node) return -ENODEV; id = sdw_get_device_id(slave, drv); if (!id) return -ENODEV; slave->ops = drv->ops;That is wrong and should never happen as you lost all reference counting. Please don't do that.
ok, so I think we all agree on the issue. It's not new code, it's been there since December 2017 and 4.16
The last line is the problematic one. If at some point, the user does an rmmod and unbinds the SoundWire codec driver, the .remove will be called and the 'drv' will no longer be valid, but we will still have a reference to drv->ops and use that pointer in the bus code, e.g. /* Update the Slave driver */ if (slave_notify && slave->ops && slave->ops->interrupt_callback) { slave_intr.sdca_cascade = sdca_cascade; slave_intr.control_port = clear; memcpy(slave_intr.port, &port_status, sizeof(slave_intr.port)); slave->ops->interrupt_callback(slave, &slave_intr); } I noodled with a potential fix in https://github.com/thesofproject/linux/pull/3534/commits/82d64fb0fd39b532263f060a8ec86e47e9ab305b where I force-reset this slave->ops pointer, but it is likely to be very racy.Just properly reference count things and this should be ok. You can NEVER just save off a pointer to a random thing that you do not own without increasing the reference count, otherwise bad things will happen. It's always been this way.
but I am not following the reference count recommendation in that specific case. If we increase a reference count in the probe, wouldn't that prevent the unbind/remove?
