+Adding folks from the original message. Unfortunately, Sashiko drops the people from the original message.
Tarun Sahu <[email protected]> writes: > Hello, > > These most of the errors are due to the device_add concurrently can add > the device and device_shutdown can access it. One of the reason I think > a device_add will be triggered while being shutdown is inserting the > physical PCI device or USB stick etc. > > But During shutdown, addition of a new device is not valid. So we can > add a check something like > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 76ba02c26aa5..c3795fa1cc26 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3650,6 +3650,13 @@ int device_add(struct device *dev) > if (!dev) > goto done; > > + if (unlikely(system_state == SYSTEM_HALT || > + system_state == SYSTEM_POWER_OFF || > + system_state == SYSTEM_RESTART)) { > + error = -ESHUTDOWN; > + goto done; > + } > + > if (!dev->p) { > error = device_private_init(dev); > if (error) > > We can also partially resolve the races partially by removing the manual > kfree(dev->p) from device_add() error path. As put_device free it > anyway when reference to devices reaches to 0. So if device_shutdown > path takes reference, dev->p will not be released. And few races will be > taken care of. But this will not resolve the deadlock and other races > like the above change. > > removing the kfree(dev->p) from device_add can be a independent patch. > like: > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 4d026682944f..76ba02c26aa5 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -2614,6 +2614,7 @@ static void device_release(struct kobject *kobj) > struct device *dev = kobj_to_dev(kobj); > struct device_private *p = dev->p; > > + dev->p = NULL; > /* > * Some platform devices are driven without driver attached > * and managed resources may have been acquired. Make sure > @@ -3824,8 +3825,6 @@ int device_add(struct device *dev) > parent_error: > put_device(parent); > name_error: > - kfree(dev->p); > - dev->p = NULL; > goto done; > } > EXPORT_SYMBOL_GPL(device_add); > > > ~Tarun > > > [email protected] writes: > >> Thank you for your contribution! Sashiko AI review found 4 potential >> issue(s) to consider: >> - [High] Modifying the core.async_shutdown module parameter during shutdown >> causes synchronous suppliers to skip waiting for asynchronous consumers. >> - [High] Unregistered consumer devices will cause a NULL pointer dereference >> during async shutdown dependency waiting. >> - [High] Adding a new device with async shutdown enabled while its >> synchronous supplier is being processed leads to an unresolvable deadlock. >> - [High] Dereferencing `dev->p->dead` without NULL checks causes a crash if >> a concurrent `device_add()` fails. >> -- >> >> commit cf98cdf76df805443cc885868cb3d9e13da5501c >> Author: David Jeffery <[email protected]> >> >> driver core: async device shutdown infrastructure >> >> This commit allows devices to opt into asynchronous shutdown, executing in >> parallel with unrelated devices after their dependencies shut down. >> Synchronous >> shutdown devices will wait for their asynchronous dependencies. The feature >> is >> enabled by default and can be toggled via the core.async_shutdown parameter. >> >>> diff --git a/drivers/base/core.c b/drivers/base/core.c >>> --- a/drivers/base/core.c >>> +++ b/drivers/base/core.c >> [ ... ] >>> +static bool wants_async_shutdown(struct device *dev) >>> +{ >>> + return async_shutdown && dev_async_shutdown(dev); >>> +} >>> + >>> +static int wait_for_device_shutdown(struct device *dev, void *data) >>> +{ >>> + bool async = *(bool *)data; >>> + >>> + if (async || wants_async_shutdown(dev)) >>> + wait_for_completion(&dev->p->complete); >>> + >>> + return 0; >>> +} >> >> [Severity: High] >> If a user-space process toggles the 0644 core.async_shutdown sysfs parameter >> from 1 to 0 during shutdown, could this cause synchronous suppliers to skip >> waiting for their asynchronous consumers? >> >> If start_async_shutdown_devices() schedules an async consumer, and then the >> parameter is toggled before the sync loop processes its supplier, the call to >> wants_async_shutdown(dev) here would evaluate to false. >> >> This would skip the wait_for_completion(), potentially causing the supplier >> (e.g., a PCI bus) to shut down while its consumer is still active. >> >> [Severity: High] >> Could this result in a NULL pointer dereference if the consumer is not yet >> registered? >> >> The device_link_add() function allows creating links to consumer devices that >> haven't been registered yet, meaning their dev->p is NULL because >> device_private_init() hasn't run. >> >> If wait_for_shutdown_dependencies() is called from the async pool, where the >> async argument is forced to true, the wait_for_completion() will >> unconditionally evaluate &dev->p->complete, crashing on the NULL pointer. >> >> [ ... ] >>> +static void __shutdown_one_device(struct device *dev) >>> { >>> if (dev->p->dead) >>> return; >> >> [Severity: High] >> Could this cause a NULL pointer dereference if a concurrent device_add() >> fails? >> >> If a concurrent device_add() fails after calling kobject_add(), its error >> path >> calls kobject_del() and then frees and nullifies dev->p. Evaluating >> dev->p->dead without checking if dev->p is valid could crash here. >> >> [ ... ] >>> +static void start_async_shutdown_devices(void) >>> +{ >>> + struct device *dev, *next, *ndev, *needs_put = NULL; >> [ ... ] >>> + needs_put = ndev; >>> + spin_lock(&devices_kset->list_lock); >>> + /* >>> + * If the next device has been marked dead while the >>> + * spinlock was released, it may no longer be on the >>> + * devices_kset list. Restart the list walk to be safe >>> + */ >>> + if (ndev && ndev->p->dead) >>> + goto restart; >> >> [Severity: High] >> Could ndev->p be NULL if a concurrent device_add() fails? >> >> If start_async_shutdown_devices() obtains a reference to next via >> get_device(), and concurrently device_add() fails and clears dev->p, >> reacquiring the lock and evaluating ndev->p->dead here could result in a NULL >> pointer dereference. >> >> [ ... ] >>> void device_shutdown(void) >>> { >> [ ... ] >>> - shutdown_one_device(dev); >>> + if (!wants_async_shutdown(dev) || !shutdown_device_async(dev)) { >>> + wait_for_shutdown_dependencies(dev, false); >>> + shutdown_one_device(dev); >>> + } >>> >>> spin_lock(&devices_kset->list_lock); >>> } >>> spin_unlock(&devices_kset->list_lock); >> >> [Severity: High] >> Can this sequence lead to an unresolvable deadlock if a new device is added >> concurrently? >> >> If a new device 'C' with DEV_FLAG_ASYNC_SHUTDOWN is added via hotplug >> after its synchronous supplier 'S' has already been popped from >> devices_kset by this loop, 'C' is appended to devices_kset. >> >> 'S' will call wait_for_shutdown_dependencies() and find 'C', blocking on >> wait_for_completion(&C->p->complete) because wants_async_shutdown(C) is true. >> >> Since 'C' was added after start_async_shutdown_devices() ran, it is not >> running in the async pool. Because the sync loop is blocked on 'S', it will >> never reach 'C' to start it, deadlocking the shutdown process permanently. >> >> -- >> Sashiko AI review ยท >> https://sashiko.dev/#/patchset/[email protected]?part=3
