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

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

Critical: 0 · High: 0 · Medium: 3 · Low: 3

- [Medium] The new shutdown guard is per-device and set lazily:
  device_shutdown() (drivers/base/core.c:4913-4914) sets…
- [Medium] device_schedule_reprobe() validates the device (dev->bus,
  dev->p, device_is_registered(), dev->driver) at dd.c:1531-1534 and…
- [Medium] The shutdown_done test that is supposed to suppress the detach
  half is evaluated once (dd.c:1472) before __device_release_driver()…
- [Low] Commit message framing vs. what the patch does: the message
  devotes most of its length to two concrete bug classes in three named…
- [Low] device_reprobe_work_fn() reports every negative device_attach()
  return with dev_err_probe(dev, ret, "re-probe failed, device left…
- [Low] device_reprobe_work_fn() decides whether to forcibly detach a
  device solely by comparing dev->driver against the raw pointer value…

--

driver core: add device_schedule_reprobe()

This adds a driver-core helper that schedules a delayed detach and
re-probe of a device on system_freezable_wq, pinning the device and its
parent and recording the bound driver pointer so a stale re-probe does
nothing.  It also adds a shutdown_done flag to struct device_private,
set by device_shutdown() under the device lock, and makes
__device_release_driver() return whether it was the caller that released
the driver.

A few questions below.

    Both classes disappear when the driver core owns the deferred work.
    Add device_schedule_reprobe(), which schedules a detach and re-probe
    of a device after a caller-specified delay:

[Severity: Low]
This isn't a bug, but could the message say more plainly that the three
drivers it describes are not converted here or later in this series?

At the end of the series, iwl_trans_reprobe_wk() in
drivers/net/wireless/intel/iwlwifi/iwl-trans.c and
h5_btrtl_reprobe_worker() in drivers/bluetooth/hci_h5.c still end with:

	if (device_reprobe(reprobe->dev))
		dev_err(reprobe->dev, "reprobe failed!\n");
	put_device(reprobe->dev);
	kfree(reprobe);
	module_put(THIS_MODULE);

with no bound-state check, and btintel_pcie is unchanged, so both bug
classes are still present in-tree.  The helper does gain in-tree callers
in this series (two call sites in drivers/net/dsa/mxl862xx/mxl862xx-fw.c),
just not in the three motivating drivers.

[ ... ]

[Severity: Medium]
This flag is set only when the reverse-order walk reaches each device, so
it means "this device has been reached", not "shutdown is in progress".
What happens when a scheduled re-probe fires after device_shutdown() has
started but before the walk reaches @dev?

The guard in device_reprobe_work_fn() sees dead == false and
shutdown_done == false, so the detach half runs:

	__device_release_driver() -> device_remove() -> drv->remove()

at a point where every device registered after @dev (children, later
probing suppliers) already had bus->shutdown()/driver->shutdown() run.

The re-attach that follows cannot succeed either, because
device_shutdown() calls device_block_probing() before the walk and
really_probe() then returns -EPROBE_DEFER:

	if (defer_all_probes) {
		...
		return -EPROBE_DEFER;
	}

device_unblock_probing() is only reached from dpm_complete(), never on the
shutdown path, so the device stays unbound.

And when the walk finally reaches @dev, this test in device_shutdown():

	} else if (dev->driver && dev->driver->shutdown) {

is false, so the driver's ->shutdown() is skipped for that device.  Would
a global condition (probing blocked, or system_state) be a better guard
here than a per-device flag?

 		/* Don't allow any more runtime suspends */
 		pm_runtime_get_noresume(dev);
 		pm_runtime_barrier(dev);

[ ... ]

[Severity: Medium]
The commit message mentions this window ("One pre-existing window
remains: __device_release_driver() transiently drops the locks while
consumer device links are busy").  Since the new caller tests
shutdown_done only once, before the call, this loop revalidates only
dev->driver after retaking the locks:

	while (device_links_busy(dev)) {
		__device_driver_unlock(dev, parent);
		device_links_unbind_consumers(dev);
		__device_driver_lock(dev, parent);
		if (dev->driver != drv) {
			pm_runtime_put(dev);
			return false;
		}
	}

In that window device_shutdown() can take both locks, set shutdown_done
and run ->shutdown(), after which the work continues into device_remove()
and ->remove() runs after ->shutdown().  Would re-testing
dev->p->shutdown_done right after __device_driver_lock() close this for
the new helper?

[ ... ]

[Severity: Low]
For ret == -EPROBE_DEFER, dev_err_probe() does more than pick a log
level.  __dev_probe_failed() in drivers/base/core.c does:

	case -EPROBE_DEFER:
		device_set_deferred_probe_reason(dev, &vaf);
		dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);

so this string is stored in dev->p->deferred_probe_reason, which
drivers/base/base.h documents as the message retrieved via debugfs, and
any reason recorded earlier by the driver's own probe is overwritten.

-EPROBE_DEFER is reachable here: really_probe() returns it while
defer_all_probes is set, and __device_attach_driver()/__device_attach()
pass it back unchanged.  In that case /sys/kernel/debug/devices_deferred
reports "re-probe failed, device left unbound" for a device that is
queued for a retry.  Should the -EPROBE_DEFER case be handled separately
from a real failure?

+		if (rp->parent_lock)
+			device_unlock(parent);
+	}
+
+	put_device(dev);
+	put_device(parent);
+	kfree(rp);
+}
+

[ ... ]

+ * scheduling time. In particular an administrative unbind is never
+ * undone by a stale re-probe.

[ ... ]

+int device_schedule_reprobe(struct device *dev, unsigned int delay_ms)
+{
+	struct device_reprobe *rp;
+
+	if (!dev->bus || !dev->p || !device_is_registered(dev))
+		return -EINVAL;
+	if (!dev->driver)
+		return -EINVAL;
+
+	rp = kzalloc_obj(*rp);
+	if (!rp)
+		return -ENOMEM;
+
+	rp->dev = get_device(dev);
+	/*
+	 * Pin the parent too: the work locks it, and an unregister of @dev
+	 * would otherwise drop the last reference before the work runs.
+	 */
+	rp->parent = get_device(dev->parent);
+	rp->parent_lock = dev->parent && dev->bus->need_parent_lock;

[Severity: Medium]
These reads happen without the device lock, and after a kzalloc_obj()
that can sleep, so the checks above them may already be stale.

device_del() takes its own copy of dev->parent at entry, unbinds the
driver, calls kobject_del() (which clears the only thing
device_is_registered() tests) and ends with:

	kobject_del(&dev->kobj);
	cleanup_glue_dir(dev, glue_dir);
	memalloc_noio_restore(noio_flag);
	put_device(parent);

dev->parent itself is never cleared.  Can a caller that passed the checks
and then slept in the allocation reach get_device(dev->parent) after that
final put_device(parent) dropped the last reference, i.e. kobject_get() on
a freed struct device, followed later by device_lock(parent) and
put_device(parent) in the work?

Separately, rp->parent and rp->parent_lock come from two independent
reads of dev->parent, and device_move() assigns it without the device
lock:

	old_parent = dev->parent;
	dev->parent = new_parent;

so can rp->parent end up NULL while rp->parent_lock is true, leaving
device_reprobe_work_fn() calling device_lock(NULL)?

Would re-validating and snapshotting parent/parent_lock/driver under
device_lock(dev), checking dev->p->dead (which kill_device() sets under
that lock at the start of device_del(), before its put_device(parent)),
avoid both cases?

+	rp->drv = READ_ONCE(dev->driver);

[Severity: Low]
Is a raw pointer value enough to identify the binding that asked for the
re-probe?

Within delay_ms, an unbind followed by a bind of the same driver:

	echo dev > /sys/bus/.../unbind
	echo dev > /sys/bus/.../bind

restores dev->driver == rp->drv, so the check in
device_reprobe_work_fn() matches and the stale work runs a detach and
re-attach on a binding that never requested one.  That also narrows the
kernel-doc claim above, "an administrative unbind is never undone by a
stale re-probe": a plain unbind is honoured, an unbind plus rebind is
not.

And once the original module is unloaded, can another module's static
struct device_driver end up at the same address and make the comparison
succeed for an unrelated driver?  Would a generation counter, or a
recorded driver name, be a more stable token here?

+	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);

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