Conceptual bug on SoundWire probe/remove?

[Srinivas Kandagatla]

On 23/03/2022 19:45, 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;

may be add try_module_get(dev->driver->owner) here, which should prevent 
the module from unloading in the first place.

--srini

> 
> 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.
> 
> We probably need to avoid such references, or have a clean mechanism to 
> unbind, e.g. with all commands and interrupts stopped while the codec 
> driver .remove routine is handled.
> 
> Initial error reports at https://github.com/thesofproject/linux/issues/3531
> 
> Suggestions and comments welcome, thanks!
> -Pierre