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.
> 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.
thanks,
greg k-h
