The driver_sysfs_remove() should always be called after successful driver_sysfs_add(). Otherwise, NULL pointers will be passed to the sysfs_remove_link(), where it is decoded as searching sysfs root.
Fixes: 1901fb2604fbc ("Driver core: fix "driver" symlink timing") Cc: stable@vger.kernel.org Signed-off-by: Lu Baolu baolu.lu@linux.intel.com --- drivers/base/dd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 68ea1f949daa..9eaaff2f556c 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -577,14 +577,14 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (dev->bus->dma_configure) { ret = dev->bus->dma_configure(dev); if (ret) - goto probe_failed; + goto pinctrl_bind_failed; }
ret = driver_sysfs_add(dev); if (ret) { pr_err("%s: driver_sysfs_add(%s) failed\n", __func__, dev_name(dev)); - goto probe_failed; + goto sysfs_failed; }
if (dev->pm_domain && dev->pm_domain->activate) { @@ -657,6 +657,8 @@ static int really_probe(struct device *dev, struct device_driver *drv) else if (drv->remove) drv->remove(dev); probe_failed: + driver_sysfs_remove(dev); +sysfs_failed: if (dev->bus) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_DRIVER_NOT_BOUND, dev); @@ -666,7 +668,6 @@ static int really_probe(struct device *dev, struct device_driver *drv) arch_teardown_dma_ops(dev); kfree(dev->dma_range_map); dev->dma_range_map = NULL; - driver_sysfs_remove(dev); dev->driver = NULL; dev_set_drvdata(dev, NULL); if (dev->pm_domain && dev->pm_domain->dismiss)
On Wed, Dec 29, 2021 at 12:51:59PM +0800, Lu Baolu wrote:
The driver_sysfs_remove() should always be called after successful driver_sysfs_add(). Otherwise, NULL pointers will be passed to the sysfs_remove_link(), where it is decoded as searching sysfs root.
What null pointer is being sent to sysfs_remove_link()? For which link?
How are you triggering this failure path and how was it tested?
Fixes: 1901fb2604fbc ("Driver core: fix "driver" symlink timing") Cc: stable@vger.kernel.org Signed-off-by: Lu Baolu baolu.lu@linux.intel.com
drivers/base/dd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 68ea1f949daa..9eaaff2f556c 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -577,14 +577,14 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (dev->bus->dma_configure) { ret = dev->bus->dma_configure(dev); if (ret)
goto probe_failed;
goto pinctrl_bind_failed;
Why not call the notifier chain here? Did you verify that this change still works properly?
thanks,
greg k-h
Hi Greg,
On 12/29/21 6:04 PM, Greg Kroah-Hartman wrote:
On Wed, Dec 29, 2021 at 12:51:59PM +0800, Lu Baolu wrote:
The driver_sysfs_remove() should always be called after successful driver_sysfs_add(). Otherwise, NULL pointers will be passed to the sysfs_remove_link(), where it is decoded as searching sysfs root.
What null pointer is being sent to sysfs_remove_link()? For which link?
Oh, my fault. Thank you for pointing this out.
The device and driver sysfs nodes have already been created, so there's no null pointers. The out-of-order call of driver_sysfs_remove() just tries to remove some nonexistent nodes under the device and driver sysfs nodes. It is allowed by the sysfs layer.
How are you triggering this failure path and how was it tested?
I hacked the a driver to return failure in dma_configure() callback. I didn't see any failure. But I mistakenly thought that driver_sysfs_remove() could possibly delete some sysfs entries by mistake. That's not true. Sorry for the noise.
Fixes: 1901fb2604fbc ("Driver core: fix "driver" symlink timing") Cc: stable@vger.kernel.org
This patch only improves the readability of really_probe() and it does not fix any bugs. I will remove above tags and resent a version if you think this improvement is valuable.
Signed-off-by: Lu Baolu baolu.lu@linux.intel.com
drivers/base/dd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 68ea1f949daa..9eaaff2f556c 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -577,14 +577,14 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (dev->bus->dma_configure) { ret = dev->bus->dma_configure(dev); if (ret)
goto probe_failed;
goto pinctrl_bind_failed;Why not call the notifier chain here? Did you verify that this change still works properly?
The BUS_NOTIFY_DRIVER_NOT_BOUND event is listened in two places in the tree.
$ git grep BUS_NOTIFY_DRIVER_NOT_BOUND -- :^drivers/base/dd.c :^include drivers/acpi/acpi_lpss.c: case BUS_NOTIFY_DRIVER_NOT_BOUND: drivers/base/power/clock_ops.c: case BUS_NOTIFY_DRIVER_NOT_BOUND:
The usage pattern is setting up something in BUS_NOTIFY_BIND_DRIVER and doing the cleanup in BUS_NOTIFY_DRIVER_NOT_BOUND or BUS_NOTIFY_UNBIND_DRIVER. The right order of these events should be
[failure case] - BUS_NOTIFY_BIND_DRIVER: driver is about to be bound - BUS_NOTIFY_DRIVER_NOT_BOUND: driver failed to be bound
or
[successful case] - BUS_NOTIFY_BIND_DRIVER: driver is about to be bound - BUS_NOTIFY_BOUND_DRIVER: driver bound to device - BUS_NOTIFY_UNBIND_DRIVER: driver is about to be unbound - BUS_NOTIFY_UNBOUND_DRIVER: driver is unbound from the device
Without above change, when dma_configure() returns failure, the listener could get a BUS_NOTIFY_DRIVER_NOT_BOUND without BUS_NOTIFY_BIND_DRIVER.
Please guide me if my understanding is wrong.
thanks,
greg k-h
Best regards, baolu
linux-stable-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Lu Baolu