[PATCH 1/2] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[PATCH 1/2] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[Dmitry Torokhov]

This reverts commit c0a40097f0bc81deafc15f9195d1fb54595cd6d0.

Probing a device can take arbitrary long time. In the field we observed
that, for example, probing a bad micro-SD cards in an external USB card
reader (or maybe cards were good but cables were flaky) sometimes takes
longer than 2 minutes due to multiple retries at various levels of the
stack. We can not block uevent_show() method for that long because udev
is reading that attribute very often and that blocks udev and interferes
with booting of the system.

The change that introduced locking was concerned with dev_uevent()
racing with unbinding the driver. However we can handle it without
locking (which will be done in subsequent patch).

There was also claim that synchronization with probe() is needed to
properly load USB drivers, however this is a red herring: the change
adding the lock was introduced in May of last year and USB loading and
probing worked properly for many years before that.

Revert the harmful locking.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5a1f05198114..9f4d4868e3b4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2725,11 +2725,8 @@ static ssize_t uevent_show(struct device *dev, struct device_attribute *attr,
 	if (!env)
 		return -ENOMEM;
 
-	/* Synchronize with really_probe() */
-	device_lock(dev);
 	/* let the kset specific function add its keys */
 	retval = kset->uevent_ops->uevent(&dev->kobj, env);
-	device_unlock(dev);
 	if (retval)
 		goto out;
 
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH 2/2] driver core: fix potential NULL pointer dereference in dev_uevent()

[Dmitry Torokhov]

If userspace reads "uevent" device attribute at the same time as another
threads unbinds the device from its driver, change to dev->driver from a
valid pointer to NULL may result in crash. Fix this by using READ_ONCE()
when fetching the pointer, and take bus' drivers klist lock to make sure
driver instance will not disappear while we access it.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/base.h |  1 +
 drivers/base/bus.c  |  2 +-
 drivers/base/core.c | 32 ++++++++++++++++++++++++++++++--
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 8cf04a557bdb..91b786891209 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -73,6 +73,7 @@ static inline void subsys_put(struct subsys_private *sp)
 		kset_put(&sp->subsys);
 }
 
+struct subsys_private *bus_to_subsys(const struct bus_type *bus);
 struct subsys_private *class_to_subsys(const struct class *class);
 
 struct driver_private {
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 6b9e65a42cd2..c8c7e0804024 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -57,7 +57,7 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
  * NULL.  A call to subsys_put() must be done when finished with the pointer in
  * order for it to be properly freed.
  */
-static struct subsys_private *bus_to_subsys(const struct bus_type *bus)
+struct subsys_private *bus_to_subsys(const struct bus_type *bus)
 {
 	struct subsys_private *sp = NULL;
 	struct kobject *kobj;
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 9f4d4868e3b4..670f77b9b378 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2623,6 +2623,34 @@ static const char *dev_uevent_name(const struct kobject *kobj)
 	return NULL;
 }
 
+/*
+ * Try filling "DRIVER=<name>" uevent variable for a device. Because this
+ * function may race with binding and unbinding device from a driver we need to
+ * be careful. Binding is generally safe, at worst we miss the fact that device
+ * is already bound to a driver (but the driver information that is delivered
+ * through uevents is best-effort, it may become obsolete as soon as it is
+ * generated anyways). Unbinding is more risky as driver transitioning to NULL,
+ * so READ_ONCE() should be used to make sure we are dealing with the same
+ * pointer, and to ensure that driver structure is not going to disappear from
+ * under us we take bus' drivers klist lock. The assumption that only registered
+ * driver can be bound to a device, and to unregister a driver bus code will
+ * take the same lock.
+ */
+static void dev_driver_uevent(const struct device *dev, struct kobj_uevent_env *env)
+{
+	struct subsys_private *sp = bus_to_subsys(dev->bus);
+
+	if (sp) {
+		scoped_guard(spinlock, &sp->klist_drivers.k_lock) {
+			struct device_driver *drv = READ_ONCE(dev->driver);
+			if (drv)
+				add_uevent_var(env, "DRIVER=%s", drv->name);
+		}
+
+		subsys_put(sp);
+	}
+}
+
 static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
 {
 	const struct device *dev = kobj_to_dev(kobj);
@@ -2654,8 +2682,8 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
 	if (dev->type && dev->type->name)
 		add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
 
-	if (dev->driver)
-		add_uevent_var(env, "DRIVER=%s", dev->driver->name);
+	/* Add "DRIVER=%s" variable if the device is bound to a driver */
+	dev_driver_uevent(dev, env);
 
 	/* Add common DT information about the device */
 	of_device_uevent(dev, env);
-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH 2/2] driver core: fix potential NULL pointer dereference in dev_uevent()

[Rafael J. Wysocki]

On Thu, Feb 20, 2025 at 7:47 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> If userspace reads "uevent" device attribute at the same time as another
> threads unbinds the device from its driver, change to dev->driver from a
> valid pointer to NULL may result in crash. Fix this by using READ_ONCE()
> when fetching the pointer, and take bus' drivers klist lock to make sure
> driver instance will not disappear while we access it.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/base.h |  1 +
>  drivers/base/bus.c  |  2 +-
>  drivers/base/core.c | 32 ++++++++++++++++++++++++++++++--
>  3 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 8cf04a557bdb..91b786891209 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -73,6 +73,7 @@ static inline void subsys_put(struct subsys_private *sp)
>                 kset_put(&sp->subsys);
>  }
>
> +struct subsys_private *bus_to_subsys(const struct bus_type *bus);
>  struct subsys_private *class_to_subsys(const struct class *class);
>
>  struct driver_private {
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 6b9e65a42cd2..c8c7e0804024 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -57,7 +57,7 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
>   * NULL.  A call to subsys_put() must be done when finished with the pointer in
>   * order for it to be properly freed.
>   */
> -static struct subsys_private *bus_to_subsys(const struct bus_type *bus)
> +struct subsys_private *bus_to_subsys(const struct bus_type *bus)
>  {
>         struct subsys_private *sp = NULL;
>         struct kobject *kobj;
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 9f4d4868e3b4..670f77b9b378 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2623,6 +2623,34 @@ static const char *dev_uevent_name(const struct kobject *kobj)
>         return NULL;
>  }
>
> +/*
> + * Try filling "DRIVER=<name>" uevent variable for a device. Because this
> + * function may race with binding and unbinding device from a driver we need to
> + * be careful. Binding is generally safe, at worst we miss the fact that device
> + * is already bound to a driver (but the driver information that is delivered
> + * through uevents is best-effort, it may become obsolete as soon as it is
> + * generated anyways). Unbinding is more risky as driver transitioning to NULL,
> + * so READ_ONCE() should be used to make sure we are dealing with the same
> + * pointer, and to ensure that driver structure is not going to disappear from
> + * under us we take bus' drivers klist lock. The assumption that only registered
> + * driver can be bound to a device, and to unregister a driver bus code will
> + * take the same lock.
> + */
> +static void dev_driver_uevent(const struct device *dev, struct kobj_uevent_env *env)
> +{
> +       struct subsys_private *sp = bus_to_subsys(dev->bus);
> +
> +       if (sp) {
> +               scoped_guard(spinlock, &sp->klist_drivers.k_lock) {
> +                       struct device_driver *drv = READ_ONCE(dev->driver);

I think you need to use a matching WRITE_ONCE() on the update side
because you don't want to READ_ONCE() a partially updated memory
location.

> +                       if (drv)
> +                               add_uevent_var(env, "DRIVER=%s", drv->name);
> +               }
> +
> +               subsys_put(sp);
> +       }
> +}
> +
>  static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>  {
>         const struct device *dev = kobj_to_dev(kobj);
> @@ -2654,8 +2682,8 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>         if (dev->type && dev->type->name)
>                 add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
>
> -       if (dev->driver)
> -               add_uevent_var(env, "DRIVER=%s", dev->driver->name);
> +       /* Add "DRIVER=%s" variable if the device is bound to a driver */
> +       dev_driver_uevent(dev, env);
>
>         /* Add common DT information about the device */
>         of_device_uevent(dev, env);
> --

Re: [PATCH 2/2] driver core: fix potential NULL pointer dereference in dev_uevent()

[Dmitry Torokhov]

On Thu, Feb 20, 2025 at 11:59:02AM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 20, 2025 at 7:47 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > If userspace reads "uevent" device attribute at the same time as another
> > threads unbinds the device from its driver, change to dev->driver from a
> > valid pointer to NULL may result in crash. Fix this by using READ_ONCE()
> > when fetching the pointer, and take bus' drivers klist lock to make sure
> > driver instance will not disappear while we access it.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/base/base.h |  1 +
> >  drivers/base/bus.c  |  2 +-
> >  drivers/base/core.c | 32 ++++++++++++++++++++++++++++++--
> >  3 files changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/base.h b/drivers/base/base.h
> > index 8cf04a557bdb..91b786891209 100644
> > --- a/drivers/base/base.h
> > +++ b/drivers/base/base.h
> > @@ -73,6 +73,7 @@ static inline void subsys_put(struct subsys_private *sp)
> >                 kset_put(&sp->subsys);
> >  }
> >
> > +struct subsys_private *bus_to_subsys(const struct bus_type *bus);
> >  struct subsys_private *class_to_subsys(const struct class *class);
> >
> >  struct driver_private {
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 6b9e65a42cd2..c8c7e0804024 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -57,7 +57,7 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
> >   * NULL.  A call to subsys_put() must be done when finished with the pointer in
> >   * order for it to be properly freed.
> >   */
> > -static struct subsys_private *bus_to_subsys(const struct bus_type *bus)
> > +struct subsys_private *bus_to_subsys(const struct bus_type *bus)
> >  {
> >         struct subsys_private *sp = NULL;
> >         struct kobject *kobj;
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 9f4d4868e3b4..670f77b9b378 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2623,6 +2623,34 @@ static const char *dev_uevent_name(const struct kobject *kobj)
> >         return NULL;
> >  }
> >
> > +/*
> > + * Try filling "DRIVER=<name>" uevent variable for a device. Because this
> > + * function may race with binding and unbinding device from a driver we need to
> > + * be careful. Binding is generally safe, at worst we miss the fact that device
> > + * is already bound to a driver (but the driver information that is delivered
> > + * through uevents is best-effort, it may become obsolete as soon as it is
> > + * generated anyways). Unbinding is more risky as driver transitioning to NULL,
> > + * so READ_ONCE() should be used to make sure we are dealing with the same
> > + * pointer, and to ensure that driver structure is not going to disappear from
> > + * under us we take bus' drivers klist lock. The assumption that only registered
> > + * driver can be bound to a device, and to unregister a driver bus code will
> > + * take the same lock.
> > + */
> > +static void dev_driver_uevent(const struct device *dev, struct kobj_uevent_env *env)
> > +{
> > +       struct subsys_private *sp = bus_to_subsys(dev->bus);
> > +
> > +       if (sp) {
> > +               scoped_guard(spinlock, &sp->klist_drivers.k_lock) {
> > +                       struct device_driver *drv = READ_ONCE(dev->driver);
> 
> I think you need to use a matching WRITE_ONCE() on the update side
> because you don't want to READ_ONCE() a partially updated memory
> location.

Yes, indeed, thank you Rafael. I forgot that it is no longer guaranteed
that we won't have tearing when writing native word sizes.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[Greg Kroah-Hartman]

On Wed, Feb 19, 2025 at 10:46:44PM -0800, Dmitry Torokhov wrote:
> This reverts commit c0a40097f0bc81deafc15f9195d1fb54595cd6d0.
> 
> Probing a device can take arbitrary long time. In the field we observed
> that, for example, probing a bad micro-SD cards in an external USB card
> reader (or maybe cards were good but cables were flaky) sometimes takes
> longer than 2 minutes due to multiple retries at various levels of the
> stack. We can not block uevent_show() method for that long because udev
> is reading that attribute very often and that blocks udev and interferes
> with booting of the system.
> 
> The change that introduced locking was concerned with dev_uevent()
> racing with unbinding the driver. However we can handle it without
> locking (which will be done in subsequent patch).

So shouldn't we take the second patch first to prevent any issues here?

> There was also claim that synchronization with probe() is needed to
> properly load USB drivers, however this is a red herring: the change
> adding the lock was introduced in May of last year and USB loading and
> probing worked properly for many years before that.
> 
> Revert the harmful locking.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

No Fixes: or cc: stable for this?

thanks,

greg k-h

Re: [PATCH 1/2] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[Dmitry Torokhov]

On February 19, 2025 11:13:00 PM PST, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>On Wed, Feb 19, 2025 at 10:46:44PM -0800, Dmitry Torokhov wrote:
>> This reverts commit c0a40097f0bc81deafc15f9195d1fb54595cd6d0.
>> 
>> Probing a device can take arbitrary long time. In the field we observed
>> that, for example, probing a bad micro-SD cards in an external USB card
>> reader (or maybe cards were good but cables were flaky) sometimes takes
>> longer than 2 minutes due to multiple retries at various levels of the
>> stack. We can not block uevent_show() method for that long because udev
>> is reading that attribute very often and that blocks udev and interferes
>> with booting of the system.
>> 
>> The change that introduced locking was concerned with dev_uevent()
>> racing with unbinding the driver. However we can handle it without
>> locking (which will be done in subsequent patch).
>
>So shouldn't we take the second patch first to prevent any issues here?

I think the potential for the NULL dereference is extremely small, we lived with it for many years. But if you prefer the patches can be swapped.

>
>> There was also claim that synchronization with probe() is needed to
>> properly load USB drivers, however this is a red herring: the change
>> adding the lock was introduced in May of last year and USB loading and
>> probing worked properly for many years before that.
>> 
>> Revert the harmful locking.
>> 
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
>No Fixes: or cc: stable for this?

I did not think we need "fixes" for a revert... Do we?

As far as stable goes: you're the maintainer so IMO it's for you to decide, but yes, I'd like it to land in stable.

Thanks. 
Hi Greg, 
-- 
Dmitry

Re: [PATCH 1/2] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[Dmitry Torokhov]

On Wed, Feb 19, 2025 at 11:22:25PM -0800, Dmitry Torokhov wrote:
> On February 19, 2025 11:13:00 PM PST, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >On Wed, Feb 19, 2025 at 10:46:44PM -0800, Dmitry Torokhov wrote:
> >> This reverts commit c0a40097f0bc81deafc15f9195d1fb54595cd6d0.
> >> 
> >> Probing a device can take arbitrary long time. In the field we observed
> >> that, for example, probing a bad micro-SD cards in an external USB card
> >> reader (or maybe cards were good but cables were flaky) sometimes takes
> >> longer than 2 minutes due to multiple retries at various levels of the
> >> stack. We can not block uevent_show() method for that long because udev
> >> is reading that attribute very often and that blocks udev and interferes
> >> with booting of the system.
> >> 
> >> The change that introduced locking was concerned with dev_uevent()
> >> racing with unbinding the driver. However we can handle it without
> >> locking (which will be done in subsequent patch).
> >
> >So shouldn't we take the second patch first to prevent any issues here?
> 
> I think the potential for the NULL dereference is extremely small, we
> lived with it for many years. But if you prefer the patches can be
> swapped.

Greg, I was looking at this again and I do not think it makes sense to
swap the patches, as then explanation and justification makes no sense.
So we can either keep it as a straight revert and then address the
driver pointer handling, or combine the 2. What would be your
preference?

I will need to respin to address Rafael's comment anyways.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[Greg Kroah-Hartman]

On Mon, Feb 24, 2025 at 06:17:36PM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 19, 2025 at 11:22:25PM -0800, Dmitry Torokhov wrote:
> > On February 19, 2025 11:13:00 PM PST, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > >On Wed, Feb 19, 2025 at 10:46:44PM -0800, Dmitry Torokhov wrote:
> > >> This reverts commit c0a40097f0bc81deafc15f9195d1fb54595cd6d0.
> > >> 
> > >> Probing a device can take arbitrary long time. In the field we observed
> > >> that, for example, probing a bad micro-SD cards in an external USB card
> > >> reader (or maybe cards were good but cables were flaky) sometimes takes
> > >> longer than 2 minutes due to multiple retries at various levels of the
> > >> stack. We can not block uevent_show() method for that long because udev
> > >> is reading that attribute very often and that blocks udev and interferes
> > >> with booting of the system.
> > >> 
> > >> The change that introduced locking was concerned with dev_uevent()
> > >> racing with unbinding the driver. However we can handle it without
> > >> locking (which will be done in subsequent patch).
> > >
> > >So shouldn't we take the second patch first to prevent any issues here?
> > 
> > I think the potential for the NULL dereference is extremely small, we
> > lived with it for many years. But if you prefer the patches can be
> > swapped.
> 
> Greg, I was looking at this again and I do not think it makes sense to
> swap the patches, as then explanation and justification makes no sense.
> So we can either keep it as a straight revert and then address the
> driver pointer handling, or combine the 2. What would be your
> preference?

Your original sequence is fine, it makes more sense as you point out.

> I will need to respin to address Rafael's comment anyways.

That would be great, thanks!

greg k-h