Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Felix Gu <ustc.gu@gmail.com>]

>  /**
> @@ -3398,22 +3395,14 @@ static void devm_spi_unregister(struct device *dev, void *res)
>  int devm_spi_register_controller(struct device *dev,
>  				 struct spi_controller *ctlr)
>  {
> -	struct spi_controller **ptr;
>  	int ret;
>  
> -	ptr = devres_alloc(devm_spi_unregister, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return -ENOMEM;
> -
>  	ret = spi_register_controller(ctlr);
> -	if (!ret) {
> -		*ptr = ctlr;
> -		devres_add(dev, ptr);
> -	} else {
> -		devres_free(ptr);
> -	}
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, devm_spi_unregister_controller, ctlr);
>  
> -	return ret;
>  }
>  EXPORT_SYMBOL_GPL(devm_spi_register_controller);
Hi Andy and Mark,
It seems has a potential corner case in this commit.

The issue is that devm_add_action_or_reset() triggers its callback 
immediately if the internal devres allocation fails. This creates the 
following sequence:
1. spi_register_controller(ctlr) succeeds.
2. devm_add_action_or_reset() is called but fails.
3. The "reset" triggers devm_spi_unregister_controller(ctlr) immediately.
4. This calls spi_unregister_controller(ctlr).
5. For controllers allocated via spi_alloc_host() (where 
   ctlr->devm_allocated is false), spi_unregister_controller() calls 
   put_device(&ctlr->dev).
6. This drops the reference count to zero and frees the ctlr structure.

However, the driver still holds the ctlr pointer and will typically 
attempt its own cleanup, leading to a use-after-free or double-free.

What are your thoughts on this? Does this analysis seem correct, or am
I missing a detail in how the refcounting is handled here?

Best regards,
Felix