[PATCH v2] usb: phy: don't overwrite a device_type the bus already set

[PATCH v2] usb: phy: don't overwrite a device_type the bus already set

[Palla Raghunath]

usb_add_phy_dev() replaces the device_type of whatever device the PHY
driver passed in:

	x->dev->type = &usb_phy_dev_type;

That device isn't ours.  PHY drivers point x->dev at the device they are
bound to, and its bus has usually set a device_type up already.

i2c is where this hurts.  An i2c client keeps its release callback on
the device_type, and leaves dev->release NULL:

	const struct device_type i2c_client_type = {
		.groups  = i2c_dev_groups,
		.uevent  = i2c_device_uevent,
		.release = i2c_client_dev_release,
	};

usb_phy_dev_type has no ->release, so once it has replaced
i2c_client_type there is nothing left to free the client with, and
usb_remove_phy() doesn't put the old type back either.  Removing the
client then hits the warning in device_release():

  Device '0-002c' does not have a release() function, it is broken
  WARNING: drivers/base/core.c:2642 at device_release+0x1de/0x280
  Workqueue: usb_hub_wq hub_event
  Call Trace:
   kobject_put+0x162/0x260
   device_unregister+0x27/0x30
   i2c_deregister_clients+0x27d/0x410
   i2c_del_adapter+0xe9/0x230
   i2c_tiny_usb_disconnect+0x3f/0x90
   usb_unbind_interface+0x1e5/0x9c0
   device_remove+0x125/0x170
   device_release_driver_internal+0x4e2/0x6b0
   bus_remove_device+0x2f5/0x470

syzbot gets there with a fake i2c-tiny-usb adapter: instantiate an
isp1301 on the new bus through its new_device attribute, then unplug the
USB device.

Only i2c is affected.  phy-isp1301.c is the one i2c driver among the
twelve callers of usb_add_phy_dev(); the others pass a platform device
or a struct phy, and both leave ->type NULL and keep their release on
dev->release or dev->class->dev_release, so device_release() still finds
one for them.

So only take the device_type if nothing else has.  Callers that rely on
the uevent handler still get it, their ->type being NULL, and the i2c
client keeps the release it cannot do without.

Tested on x86_64 with the syzbot reproducer: before the change the first
isp1301 instantiation panics on unplug, after it 292 instantiate/unplug
cycles pass without a splat.

Reported-by: syzbot+3fb7629cfd12d04beeab@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3fb7629cfd12d04beeab
Fixes: a8534cb092d7 ("usb: phy: introduce usb_phy device type with its own uevent handler")
Cc: stable@vger.kernel.org
Reviewed-by: Peter Chen <peter.chen@kernel.org>
Signed-off-by: Palla Raghunath <raghunathpalla.0209@gmail.com>
---
v2:
- Add Cc: stable, as Peter asked, and his Reviewed-by
- v1: https://lore.kernel.org/all/20260924194236.168010-1-raghunathpalla.0209@gmail.com/

 drivers/usb/phy/phy.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index 5a9b9353f343..ded18c7fe32f 100644
--- a/drivers/usb/phy/phy.c
+++ b/drivers/usb/phy/phy.c
@@ -705,7 +705,13 @@ int usb_add_phy_dev(struct usb_phy *x)
 	if (ret)
 		return ret;
 
-	x->dev->type = &usb_phy_dev_type;
+	/*
+	 * Don't clobber a device_type the bus already set.  x->dev is the
+	 * PHY driver's own device, and for an i2c client the release
+	 * callback lives on the type.
+	 */
+	if (!x->dev->type)
+		x->dev->type = &usb_phy_dev_type;
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&x->notifier);
 
-- 
2.34.1

Re: [PATCH v2] usb: phy: don't overwrite a device_type the bus already set

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: phy: mutated device_type suppresses unbind/remove uevents
--

commit 3154a296e918fd840b2f1b31a53c7b6b47a5a85e
Author: Palla Raghunath <raghunathpalla.0209@gmail.com>

usb: phy: don't overwrite a device_type the bus already set

usb_add_phy_dev() replaces the device_type of whatever device the PHY driver
passed in. This hurts i2c clients as they keep their release callback on the
device_type. Only take the device_type if nothing else has been set.

> diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
> index 5a9b9353f343d..ded18c7fe32ff 100644
> --- a/drivers/usb/phy/phy.c
> +++ b/drivers/usb/phy/phy.c
> @@ -705,7 +705,13 @@ int usb_add_phy_dev(struct usb_phy *x)
>  	if (ret)
>  		return ret;
>  
> -	x->dev->type = &usb_phy_dev_type;
> +	/*
> +	 * Don't clobber a device_type the bus already set.  x->dev is the
> +	 * PHY driver's own device, and for an i2c client the release
> +	 * callback lives on the type.
> +	 */
> +	if (!x->dev->type)
> +		x->dev->type = &usb_phy_dev_type;

[Severity: Medium]
Will mutating the device type here suppress teardown uevents for platform
devices?

When a platform device driver unbinds, it first removes the PHY and then the
driver core emits uevents:

driver_remove()
  usb_remove_phy(x)
    list_del(&x->head);

device_unbind() / device_remove()
  kobject_uevent_env()
    dev_uevent()
      usb_phy_uevent()

Because usb_remove_phy() does not restore x->dev->type when it deletes the
PHY from phy_list, __device_to_usb_phy() will return NULL during the
teardown uevents.

usb_phy_uevent() then returns -ENODEV:

drivers/usb/phy/phy.c:usb_phy_uevent() {
	...
	spin_lock_irqsave(&phy_lock, flags);
	usb_phy = __device_to_usb_phy(dev);
	spin_unlock_irqrestore(&phy_lock, flags);

	if (!usb_phy)
		return -ENODEV;
	...
}

Does this -ENODEV return abort the kobject_uevent() emission entirely,
silently dropping the unbind and remove uevents?

Could this regression cause userspace tools like udev to miss device teardown
events?

>  
>  	ATOMIC_INIT_NOTIFIER_HEAD(&x->notifier);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011174853.59234-1-raghunathpalla.0209@gmail.com?part=1