Re: [PATCH] drm/rockchip: use the safe iterator when removing device links

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/rockchip: wrong unlocked list in rockchip_drm_match_remove()
--

commit e68a6141bb89fc9c57189db5bdc14a46b7da846f
Author: Wentao Liang <vulab@iscas.ac.cn>

drm/rockchip: use the safe iterator when removing device links

This commit changes rockchip_drm_match_remove() to use
list_for_each_entry_safe() when walking dev->links.consumers and calling
device_link_del().

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 3099408e9d050..9edfe4db93934 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -356,9 +356,9 @@ int rockchip_drm_endpoint_is_subdriver(struct device_node *ep)
>  
>  static void rockchip_drm_match_remove(struct device *dev)
>  {
> -	struct device_link *link;
> +	struct device_link *link, *tmp;
>  
> -	list_for_each_entry(link, &dev->links.consumers, s_node)
> +	list_for_each_entry_safe(link, tmp, &dev->links.consumers, s_node)
>  		device_link_del(link);

[Severity: High]
Does walking dev->links.consumers in rockchip_drm_match_remove() actually
remove the stateless device links created by rockchip_drm_match_add(), or
does it walk the wrong list without holding device_links_lock?

In rockchip_drm_match_add(), the links are created with the master device dev
as the consumer and each sub-driver device d as the supplier:

    device_link_add(dev, d, DL_FLAG_STATELESS);

In drivers/base/core.c:device_link_add(), s_node is added to
supplier->links.consumers (which is d->links.consumers) and c_node is added to
consumer->links.suppliers (which is dev->links.suppliers):

    list_add_tail_rcu(&link->s_node, &supplier->links.consumers);
    list_add_tail_rcu(&link->c_node, &consumer->links.suppliers);

Because dev is the consumer rather than the supplier, none of the links
created by rockchip_drm_match_add() are on dev->links.consumers, so they are
all leaked when rockchip_drm_platform_probe() fails or defers and when
rockchip_drm_platform_remove() runs.

In addition, dev->links.consumers and dev->links.suppliers are internal to the
driver core and protected by device_links_write_lock(). Because
device_link_del() only acquires and releases that lock inside each call,
iterating dev->links outside drivers/base/core.c leaves the list traversal and
tmp pointer unserialized against concurrent device link changes. If any
managed links (such as fw_devlink links) are present on the list, calling
device_link_del() on them will also trigger the WARN in
device_link_put_kref():

    static void device_link_put_kref(struct device_link *link)
    {
            if (device_link_test(link, DL_FLAG_STATELESS))
                    kref_put(&link->kref, __device_link_del);
            else if (!device_is_registered(link->consumer))
                    __device_link_del(&link->kref);
            else
                    WARN(1, "Unable to drop a managed device link reference\n");
    }

Should rockchip_drm_match_remove() instead iterate the sub-driver devices and
call the exported device_link_remove(dev, d) helper (or track the
struct device_link pointers returned by device_link_add())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104025346.1527550-1-vulab@iscas.ac.cn?part=1