Re: [PATCH net] dibs: Unregister dibs_class after error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] dibs_unregister_client() can return -EBUSY after partially unregistering a client from some devices, leaving the client in an inconsistent state.
--

commit fa56c806dec36c4e7b30debb7ef4f13581270392
Author: Alexandra Winter <wintera@linux.ibm.com>

dibs: Unregister dibs_class after error

This commit ensures dibs_class is properly unregistered during dibs_init() if
dibs_loopback_init() fails. Previously, a failure would leave the class
registered, causing subsequent module load attempts to fail with -EEXIST.

> diff --git a/drivers/dibs/dibs_main.c b/drivers/dibs/dibs_main.c
> index 2b53a9d277dca..20c50997a7cf2 100644
> --- a/drivers/dibs/dibs_main.c
> +++ b/drivers/dibs/dibs_main.c
> @@ -251,13 +251,19 @@ static int __init dibs_init(void)
>  
>  	rc = class_register(&dibs_class);
>  	if (rc)
> -		return rc;
> +		goto err;
>  
>  	rc = dibs_loopback_init();
>  	if (rc)
> -		pr_err("%s fails with %d\n", __func__, rc);
> +		goto err_unregister;
>  
>  	return rc;
> +
> +err_unregister:
> +	class_unregister(&dibs_class);
> +err:
> +	pr_err("%s fails with %d\n", __func__, rc);
> +	return rc;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in
dibs_unregister_client() in this same file:

drivers/dibs/dibs_main.c:dibs_unregister_client() {
...
	list_for_each_entry(dibs, &dibs_dev_list.list, list) {
		spin_lock_irqsave(&dibs->lock, flags);
		max_dmbs = dibs->ops->max_dmbs();
		for (int i = 0; i < max_dmbs; ++i) {
			if (dibs->dmb_clientid_arr[i] == client->id) {
				WARN(1, "%s: attempt to unregister '%s' with registered dmb(s)\n",
				     __func__, client->name);
				rc = -EBUSY;
				goto err_reg_dmb;
			}
		}
		/* Stop forwarding IRQs and events */
		dibs->subs[client->id] = NULL;
		spin_unlock_irqrestore(&dibs->lock, flags);
		clients[client->id]->ops->del_dev(dibs);
		dibs->priv[client->id] = NULL;
	}
...
}

If dibs_unregister_client() successfully unregisters the client from the first
device in dibs_dev_list, but then encounters a device where the client still has
registered DMBs, it returns -EBUSY and aborts.

Since the function clears dibs->subs, calls del_dev(), and clears dibs->priv
for previous devices, does returning -EBUSY without rolling back those changes
leave the client in an inconsistent state?

If the caller retries dibs_unregister_client() later, could this cause
del_dev() to be called a second time on the already processed devices,
potentially leading to a use-after-free or double-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902143438.426664-1-wintera@linux.ibm.com?part=1