Re: [PATCH net-next v2 2/4] driver core: factor sync-state-only link cleanup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent lockless list modification of `links.defer_sync` causes list corruption, infinite loop, and use-after-free in `device_links_flush_sync_list`.
--

--- Patch [2]: [PATCH net-next v2 2/4] driver core: factor sync-state-only link cleanup ---
commit 1c995f67e6fafdae816a7fe7115a3a496dc0c968
Author: James Hilliard <james.hilliard1@gmail.com>

driver core: factor sync-state-only link cleanup

This commit factors out the existing sync-state-only link cleanup into a 
lock-held helper to reuse it from device_links_driver_bound(). It also adds 
a public wrapper so frameworks can drop temporary proxy links without 
affecting real dependency links.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4d026682944f2..627fcfa274f0e 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c

[ ... ]

> @@ -1305,6 +1305,57 @@ static void device_link_drop_managed(struct device_link *link)
> +void device_links_drop_sync_state_only(struct device *dev)
> +{
> +	LIST_HEAD(sync_list);
> +
> +	device_links_write_lock();
> +	__device_links_drop_sync_state_only(dev, &sync_list);
> +	device_links_write_unlock();
> +

[Severity: High]
This is a pre-existing issue, but can a concurrent unbind corrupt this list
iteration?

Looking at device_links_flush_sync_list(), it iterates over the sync_list
without holding the device links write lock:

drivers/base/core.c:device_links_flush_sync_list() {
    ...
    list_for_each_entry_safe(dev, tmp, list, links.defer_sync) {
        list_del_init(&dev->links.defer_sync);
        ...
        put_device(dev);
    }
}

Concurrently, if a device currently queued in the sync_list is unbound,
device_links_driver_cleanup() modifies the same list node while holding
device_links_write_lock():

drivers/base/core.c:device_links_driver_cleanup() {
    ...
    list_del_init(&dev->links.defer_sync);
    ...
}

If device_links_driver_cleanup() modifies the node that happens to be the
tmp cursor in the lockless list_for_each_entry_safe() loop, wouldn't
tmp->next point to itself due to the list_del_init() call?

This appears like it would cause an infinite loop processing the same device,
calling put_device(dev) on each iteration until the refcount underflows
and triggers a use-after-free.

> +	device_links_flush_sync_list(&sync_list, NULL);
> +}
> +EXPORT_SYMBOL_GPL(device_links_drop_sync_state_only);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260816-submit-phy-package-fwdevlink-v1-v2-0-23e55dd59fad@gmail.com?part=2