[PATCH net-next v17 3/6] driver core: add device_schedule_reprobe()

[PATCH net-next v17 3/6] driver core: add device_schedule_reprobe()

[Daniel Golle]

Drivers that need a deferred re-probe of their own device open-code a
work item in module text. iwlwifi (iwl_trans_schedule_reprobe(), for a
firmware crash a lighter restart cannot fix) and hci_h5
(h5_btrtl_resume(), RTL devices lose their firmware state over suspend)
both end that work function with put_device(); kfree();
module_put(THIS_MODULE);, where a concurrent rmmod can free the module
text the epilogue is still executing. Neither gates the re-probe on the
device still being bound to the driver that scheduled it, which a driver
cannot do from outside because device_reprobe() takes the device lock
internally.

Add device_schedule_reprobe(), which detaches and re-probes a device
after a caller-specified delay. The work function is built-in text, so a
caller needs no module reference. The binding is recorded as the driver
pointer plus a copy of its name: the pointer is only ever compared,
never dereferenced, and the name copy rejects a freed struct
device_driver address the allocator later hands to a different driver.
The first user is the mxl862xx devlink flash path added later in this
series.

Nothing is locked or validated in the caller's context, so the helper
may be called with the device lock held, as the PM callbacks, ->remove()
and ->shutdown() hold it. Buses that take the parent lock to bind are
refused with -EINVAL: that lock has to be taken before @dev's own, so
the parent would have to be recorded before either is held, where
device_move() can replace it without taking any device lock.
usb_bus_type is the only such bus and no caller needs it today.

The detach is skipped once probing is blocked, which device_shutdown()
and dpm_prepare() both set before they touch any device, and the work is
freezable so one pending across suspend runs after resume. Beyond that
gate this is device_reprobe() deferred and __device_release_driver() is
unchanged, so it carries device_reprobe()'s pre-existing limitations:
the detach and the re-attach are not one locked operation, so an
administrative unbind between them may be undone, and detaching a device
that has managed consumers unbinds them as any release does, so a
re-probe a concurrent device_shutdown() overtakes may run ->remove() in
place of ->shutdown(). None of this is specific to the helper.

Assisted-by: LLM
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v17:
 - drop the abort_if_blocked flag and the bool return of
   __device_release_driver(), leaving that function unchanged: the flag
   left the device-links state half torn down when it fired and did not
   cover the consumers unbound in the same window, so the shutdown-vs-
   release window it targeted is documented as pre-existing to every
   unbind path instead (found by Sashiko AI review)
 - record the bound driver's name beside the pointer and compare both,
   so a freed struct device_driver address reused by another driver is
   not mistaken for the original binding (found by Sashiko AI review)
 - kernel-doc: add a Context line and state the pre-existing limitations
   shared with device_reprobe() (found by Sashiko AI review)
v16:
 - commit message: device_shutdown() blocks probing only once
   wait_for_device_probe() has returned, so a re-probe already past the
   test detaches the device instead of leaving it bound for its
   ->shutdown()
 - take no lock in the caller's context and drop the parent snapshot,
   refusing buses that need the parent lock instead: the caller-context
   device lock inverted against the devlink instance lock on the flash
   path and against a synchronous work cancel on the rescue path, and a
   pinned parent can be freed by device_move() (found by Sashiko AI
   review)
 - abandon the release when probing is blocked while the device links
   loop has the locks dropped, rather than calling that window
   pre-existing: a deferred re-probe is the one unbind that may be
   abandoned, so it is the one that can close it (found by Sashiko AI
   review)
 - commit message: describe what this patch changes rather than bugs in
   drivers it does not convert, and name the first user (found by
   Sashiko AI review)
 - kernel-doc: drop the promise that an administrative unbind always
   wins, which unbind_store() does not guarantee (found by Sashiko AI
   review)
v15:
 - skip the detach while probing is blocked instead of adding a
   per-device shutdown_done flag: device_shutdown() blocks probing
   before its walk starts, so the flag left a window where the work
   detached a device that then neither re-attached nor got its
   ->shutdown() call (found by Sashiko AI review)
 - validate the device and snapshot the parent, its locking requirement
   and the bound driver under the device lock, so an unregister racing
   the allocation can neither leave a freed parent pinned nor pair a
   NULL parent with a request to lock it (found by Sashiko AI review)
 - keep -EPROBE_DEFER out of the re-probe error path, where
   dev_err_probe() would record the message as the device's deferred
   probe reason (found by Sashiko AI review)
 - kernel-doc: a stale re-probe leaves an unbound device unbound, which
   an unbind followed by a rebind within the delay does not (found by
   Sashiko AI review)
v14: no changes
v13:
 - queue the work on system_freezable_wq, so a re-probe pending across
   system suspend can neither detach a device the PM core has suspended
   nor race its late suspend callbacks; it runs after resume instead
   (found by Sashiko AI review)
 - record at scheduling time whether the parent needs locking, instead
   of reading dev->bus in the work, which may be gone with its module
   once the device has been unregistered (found by Sashiko AI review)
 - let __device_release_driver() report whether it released the driver,
   so an administrative unbind that wins the race inside the device
   links loop is not undone by the re-attach (found by Sashiko AI
   review)
 - use dev_err_probe() for the re-probe error path, so a re-probe
   deferred at resume no longer logs a spurious error (Hans de Goede,
   on the standalone posting of this helper)
 - describe the parent pinning and locking in the commit message, as in
   the standalone posting

v12:
 - pin the parent device across the deferred work; a reference on the
   child alone left device_reprobe_work_fn() dereferencing a freed
   dev->parent under __device_driver_lock() when the device was
   unregistered before the work ran (found by Sashiko AI review)
 - take the parent lock across device_attach() on buses that require
   it, matching bus_rescan_devices_helper() (found by Sashiko AI review)

v11: new patch: add device_schedule_reprobe() to the driver core (posted
     earlier as an RFC) so mxl862xx can schedule its post-flash and
     post-drain re-probe through the core instead of open-coding a work
     item
---
 drivers/base/dd.c      | 115 +++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |   2 +
 2 files changed, 117 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f6525a7ee8c5..a26a6b0eaef5 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1436,3 +1436,118 @@ void driver_detach(const struct device_driver *drv)
 		put_device(dev);
 	}
 }
+
+struct device_reprobe {
+	struct delayed_work work;
+	const struct device_driver *drv;
+	const char *drv_name;
+	struct device *dev;
+};
+
+static void device_reprobe_work_fn(struct work_struct *work)
+{
+	struct device_reprobe *rp = container_of(work, struct device_reprobe,
+						 work.work);
+	struct device *dev = rp->dev;
+	bool detached = false;
+	int ret;
+
+	device_lock(dev);
+	/*
+	 * rp->drv is only compared, never dereferenced: the driver it points
+	 * to may have been unregistered and freed. The saved name rejects a
+	 * freed address the allocator has since handed to another driver.
+	 */
+	if (!defer_all_probes && !dev->p->dead && dev->driver == rp->drv &&
+	    !strcmp(dev->driver->name, rp->drv_name)) {
+		__device_release_driver(dev, NULL);
+		detached = true;
+	}
+	device_unlock(dev);
+
+	if (detached) {
+		ret = device_attach(dev);
+		if (ret < 0 && ret != -EPROBE_DEFER)
+			dev_err_probe(dev, ret,
+				      "re-probe failed, device left unbound\n");
+	}
+
+	put_device(dev);
+	kfree(rp->drv_name);
+	kfree(rp);
+}
+
+/**
+ * device_schedule_reprobe - schedule a deferred detach and re-probe
+ * @dev: device to detach and re-probe
+ * @delay_ms: delay in milliseconds before the re-probe runs
+ *
+ * Schedule a detach and re-probe of @dev after @delay_ms milliseconds,
+ * from built-in driver-core work rather than a driver-owned work item,
+ * so the bound driver may call it without pinning its own module. The
+ * binding is recorded as the driver pointer plus a copy of its name; the
+ * pointer is only ever compared, never dereferenced, and the name copy
+ * guards against a freed &struct device_driver address the allocator
+ * later hands to a different driver.
+ *
+ * The re-probe is skipped when the work runs if @dev has since been
+ * removed, is no longer bound, is bound to a different driver, or probing
+ * has been blocked for a system shutdown. The work is freezable, so one
+ * pending across system suspend runs once the system has resumed. A
+ * failed re-probe leaves @dev unbound, as a failed initial probe would.
+ *
+ * This is device_reprobe() deferred, and shares its limitations;
+ * __device_release_driver() is unchanged. The detach and the re-attach
+ * are not one locked operation, so an administrative unbind arriving
+ * between them may be undone, and the attach half runs the normal probe
+ * path with no shutdown re-check of its own. If @dev has managed
+ * consumers, detaching it unbinds them as any driver release does, so a
+ * re-probe a concurrent device_shutdown() overtakes may run ->remove()
+ * in place of ->shutdown(). None of this is specific to this helper.
+ *
+ * Buses that take the parent lock to bind (only usb_bus_type) are refused
+ * with -EINVAL: the parent would have to be recorded before either lock
+ * is held, where device_move() can replace it.
+ *
+ * Context: May sleep (allocates with %GFP_KERNEL). May be called from any
+ * process context, @dev's own device lock held included, but not from
+ * @dev's ->probe(), which the scheduled work would detach.
+ *
+ * Returns: 0 on success, -EINVAL if @dev is not a registered device
+ * bound to a driver or sits on a bus which takes the parent lock to
+ * bind, -ENOMEM on allocation failure.
+ */
+int device_schedule_reprobe(struct device *dev, unsigned int delay_ms)
+{
+	const struct device_driver *drv;
+	struct device_reprobe *rp;
+
+	drv = READ_ONCE(dev->driver);
+	/*
+	 * A bus taking the parent lock would need @dev's parent pinned until
+	 * the work runs, which device_move() can invalidate.
+	 */
+	if (!drv || !dev->bus || dev->bus->need_parent_lock || !dev->p ||
+	    dev->p->dead || !device_is_registered(dev))
+		return -EINVAL;
+
+	rp = kzalloc_obj(*rp);
+	if (!rp)
+		return -ENOMEM;
+
+	rp->drv_name = kstrdup(drv->name, GFP_KERNEL);
+	if (!rp->drv_name) {
+		kfree(rp);
+		return -ENOMEM;
+	}
+
+	rp->dev = get_device(dev);
+	rp->drv = drv;
+
+	INIT_DELAYED_WORK(&rp->work, device_reprobe_work_fn);
+	queue_delayed_work(system_freezable_wq, &rp->work,
+			   msecs_to_jiffies(delay_ms));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_schedule_reprobe);
diff --git a/include/linux/device.h b/include/linux/device.h
index aee79fd6b32b..7a9916950577 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1314,6 +1314,8 @@ int  __must_check device_attach(struct device *dev);
 int __must_check driver_attach(const struct device_driver *drv);
 void device_initial_probe(struct device *dev);
 int __must_check device_reprobe(struct device *dev);
+int __must_check device_schedule_reprobe(struct device *dev,
+					 unsigned int delay_ms);
 
 bool device_is_bound(struct device *dev);
 
-- 
2.55.0

Re: [PATCH net-next v17 3/6] driver core: add device_schedule_reprobe()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 4 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] device_reprobe_work_fn() treats any `defer_all_probes == true`
  as a permanent shutdown.
- [Low] device_schedule_reprobe() (drivers/base/dd.c) reads the driver
  with `drv = READ_ONCE(dev->driver)` and holds no lock or reference on…
- [Low] device_reprobe_work_fn() identifies the binding only by
  `dev->driver == rp->drv && !strcmp(dev->driver->name, rp->drv_name)`.
- [Low] device_reprobe_work_fn() treats a re-probe as failed only when
  `ret < 0 && ret != -EPROBE_DEFER`, where ret comes from…
- [Low] The commit message says 'Nothing is locked or validated in the
  caller's context', but device_schedule_reprobe() does check the device…

Pre-existing issues:
- [Medium] device_reprobe_work_fn() checks defer_all_probes once, under
  device_lock(), and then calls __device_release_driver().
- [Low] The commit message's first paragraph gives, as motivation, the
  module-text epilogue race (put_device(); kfree();…

--

driver core: add device_schedule_reprobe()

This commit adds device_schedule_reprobe() to the driver core. It queues a
freezable delayed work item that detaches and re-probes a device, but only
if the device is still bound to the same driver, matched by pointer and
name. The work function is built-in text, so callers don't need a module
reference, and buses that take the parent lock to bind are refused.

> Drivers that need a deferred re-probe of their own device open-code a
> work item in module text. iwlwifi (iwl_trans_schedule_reprobe(), for a
> firmware crash a lighter restart cannot fix) and hci_h5
> (h5_btrtl_resume(), RTL devices lose their firmware state over suspend)
> both end that work function with put_device(); kfree();
> module_put(THIS_MODULE);, where a concurrent rmmod can free the module
> text the epilogue is still executing.

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch, but
neither of the two drivers given as motivation is converted anywhere in the
series. At the end of the series, iwl_trans_reprobe_wk() in iwl-trans.c and
h5_btrtl_reprobe_worker() in hci_h5.c still end with:

	put_device(reprobe->dev);
	kfree(reprobe);
	module_put(THIS_MODULE);

Could the commit message say these conversions are left for later? As
written, it reads as though the series deals with these drivers.

> Nothing is locked or validated in the caller's context, so the helper
> may be called with the device lock held, as the PM callbacks, ->remove()
> and ->shutdown() hold it.

[Severity: Low]
Is "validated" accurate here? device_schedule_reprobe() does check the
device in the caller's context:

	if (!drv || !dev->bus || dev->bus->need_parent_lock || !dev->p ||
	    dev->p->dead || !device_is_registered(dev))
		return -EINVAL;

The kernel-doc also lists these -EINVAL conditions.

What seems to be true is that nothing is locked. The checks are unlocked
snapshots, and the work repeats them under device_lock(). Could the
sentence say that instead?

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index f6525a7ee8c54..a26a6b0eaef5b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -1436,3 +1436,118 @@ void driver_detach(const struct device_driver *drv)

[ ... ]

> +static void device_reprobe_work_fn(struct work_struct *work)
> +{

[ ... ]

> +	device_lock(dev);
> +	/*
> +	 * rp->drv is only compared, never dereferenced: the driver it points
> +	 * to may have been unregistered and freed. The saved name rejects a
> +	 * freed address the allocator has since handed to another driver.
> +	 */
> +	if (!defer_all_probes && !dev->p->dead && dev->driver == rp->drv &&
> +	    !strcmp(dev->driver->name, rp->drv_name)) {

[Severity: Medium]
Can a pending request be lost for good if its timer fires between
dpm_prepare() and dpm_complete()?

dpm_prepare() sets defer_all_probes through device_block_probing(), and
dpm_complete() clears it again, so in that window the block is only
temporary. When this check fails, the work goes straight to kfree(rp) and
nothing re-queues the request.

The kernel-doc relies on system_freezable_wq being frozen during that
window. Freezable workqueues are only frozen by freeze_kernel_threads(),
through try_to_freeze_tasks(false)->freeze_workqueues_begin().
kernel_kexec() with preserve_context (CONFIG_KEXEC_JUMP) does:

	error = freeze_processes();
	...
	error = dpm_suspend_start(PMSG_FREEZE);

and never calls freeze_kernel_threads(). With CONFIG_SUSPEND_FREEZER=n,
suspend_freeze_processes() doesn't freeze anything at all.

In those cases, wouldn't the work run while defer_all_probes is set, skip
the detach and free the request? That contradicts the kernel-doc's "one
pending across system suspend runs once the system has resumed".

For the mxl862xx flash user later in the series, priv->skip_teardown would
stay set. Further flashes would be refused with "A previous flash is still
waiting for its reprobe" until someone unbinds and binds the device by
hand.

[Severity: Low]
Can this check tell the binding that made the request from a later binding
of the same driver? Driver names are unique per bus, so the (pointer, name)
pair also matches any later bind of the same driver. That includes:

  - a sysfs unbind followed by a bind within the delay (500 ms for
    mxl862xx)
  - a module reload that places the static struct device_driver at the
    same address
  - a second request queued while the first one is still pending

In each case, the stale work would detach and re-probe a fresh instance
that never asked for it. device_schedule_reprobe() doesn't coalesce
requests and returns no handle, so ->remove() cannot cancel a pending one.

With the later mxl862xx user, one sequence is:

  - the rescue-path reprobe fires during a flash
  - ->remove() waits in mxl862xx_flash_shutdown()
  - the flash queues another request
  - that second request re-probes the new instance again

The kernel-doc lists "is no longer bound, is bound to a different driver"
as skip conditions. It doesn't mention that an unbind followed by a rebind
to the same driver still triggers the re-probe. The v15 revision documented
this. Should the kernel-doc say so again?

> +		__device_release_driver(dev, NULL);
> +		detached = true;
> +	}

[Severity: Medium]
This isn't a bug introduced by this patch, since the window is in
__device_release_driver(). However, the helper makes it reachable from a
timer.

defer_all_probes is checked only once, above. __device_release_driver()
can then drop dev->mutex:

	while (device_links_busy(dev)) {
		__device_driver_unlock(dev, parent);
		device_links_unbind_consumers(dev);
		__device_driver_lock(dev, parent);
		...
		if (dev->driver != drv) {

In that window device_shutdown() can call device_block_probing(), take
device_lock(dev), and call ->shutdown() on the device, which is still
bound. When the work takes the lock back, dev->driver is unchanged, so it
goes on to device_remove(). device_block_probing() waits only for
probe_count, not for a release that is already running.

The commit message and kernel-doc describe this as a pre-existing
limitation. system_freezable_wq is not frozen during reboot, though. Is
->remove() after ->shutdown() safe for drivers whose ->shutdown() powers
the hardware off?

> +	device_unlock(dev);
> +
> +	if (detached) {
> +		ret = device_attach(dev);
> +		if (ret < 0 && ret != -EPROBE_DEFER)
> +			dev_err_probe(dev, ret,
> +				      "re-probe failed, device left unbound\n");
> +	}

[Severity: Low]
Can this message ever print when the re-probe fails? really_probe() turns
->probe() errors into positive values:

	ret = -ret;

Then __device_attach_driver() does:

	ret = driver_probe_device(drv, dev);
	if (ret < 0)
		return ret;
	return ret == 0;

So device_attach() returns 0 in both main failure cases: when ->probe()
fails, and when no driver matches because it was unregistered in between.

call_driver_probe() already logs a failed ->probe(). The no-match case,
though, leaves the device unbound without any message. Is the ret < 0
check reachable for the "A failed re-probe leaves @dev unbound" case that
the kernel-doc describes?

[ ... ]

> +int device_schedule_reprobe(struct device *dev, unsigned int delay_ms)
> +{
> +	const struct device_driver *drv;
> +	struct device_reprobe *rp;
> +
> +	drv = READ_ONCE(dev->driver);

[ ... ]

> +	rp = kzalloc_obj(*rp);
> +	if (!rp)
> +		return -ENOMEM;
> +
> +	rp->drv_name = kstrdup(drv->name, GFP_KERNEL);

[Severity: Low]
What keeps drv valid here? dev->driver is read without the device lock and
without a reference on the driver or its module. drv->name is then
dereferenced after the sleeping kzalloc_obj().

If a caller isn't serialized against the driver being unbound or its
module being unloaded, can drv->name point into freed module memory?

The callers in this series look fine:

  - mxl862xx_devlink_flash_update() runs under the devlink instance lock,
    which ->remove() waits for
  - mxl862xx_rescue_heal_work_fn() is cancelled with cancel_work_sync() in
    ->remove()

The Context line says "May be called from any process context", though.
Should it say the caller must be the bound driver, in a context that its
->remove() waits for?

Also, the commit message says the pointer is "only ever compared, never
dereferenced". That holds for the stored rp->drv, but not for this path.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790130482.git.daniel%40makrotopia.org