driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

[PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Posted by Danilo Krummrich 1 month, 4 weeks ago

dev_has_sync_state() reads dev->driver twice without holding
device_lock() -- once for the NULL check and once to dereference
->sync_state. Some callers only hold device_links_write_lock, which
doesn't prevent a concurrent unbind from clearing dev->driver via
device_unbind_cleanup().

Fix it by reading dev->driver exactly once with READ_ONCE(), pairing
with the WRITE_ONCE() in device_set_driver().

Link: https://lore.kernel.org/driver-core/DHW8QPU1VU1F.3P6PH69HLFBYC@kernel.org/
Fixes: ac338acf514e ("driver core: Add dev_has_sync_state()")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 include/linux/device.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index ac972e7bead4..4c1c9cb8570a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1018,9 +1018,12 @@ static inline void device_lock_assert(struct device *dev)
 
 static inline bool dev_has_sync_state(struct device *dev)
 {
+	struct device_driver *drv;
+
 	if (!dev)
 		return false;
-	if (dev->driver && dev->driver->sync_state)
+	drv = READ_ONCE(dev->driver);
+	if (drv && drv->sync_state)
 		return true;
 	if (dev->bus && dev->bus->sync_state)
 		return true;

base-commit: 5b484311507b5d403c1f7a45f6aa3778549e268b
-- 
2.53.0

Re: [PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Posted by Danilo Krummrich 1 month, 2 weeks ago

On Sat, 18 Apr 2026 18:22:18 +0200, Danilo Krummrich wrote:
> [PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Applied, thanks!

  Branch: driver-core-testing
  Tree:   git://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git

[1/1] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()
      commit: e9506871a8ea

The patch will appear in the next linux-next integration (typically within 24
hours on weekdays).

The patch is in the driver-core-testing branch and will be promoted to
driver-core-next after validation.

Re: [PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Posted by Rafael J. Wysocki 1 month, 3 weeks ago

On Sat, Apr 18, 2026 at 6:22 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> dev_has_sync_state() reads dev->driver twice without holding
> device_lock() -- once for the NULL check and once to dereference
> ->sync_state. Some callers only hold device_links_write_lock, which
> doesn't prevent a concurrent unbind from clearing dev->driver via
> device_unbind_cleanup().
>
> Fix it by reading dev->driver exactly once with READ_ONCE(), pairing
> with the WRITE_ONCE() in device_set_driver().
>
> Link: https://lore.kernel.org/driver-core/DHW8QPU1VU1F.3P6PH69HLFBYC@kernel.org/
> Fixes: ac338acf514e ("driver core: Add dev_has_sync_state()")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>

> ---
>  include/linux/device.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ac972e7bead4..4c1c9cb8570a 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1018,9 +1018,12 @@ static inline void device_lock_assert(struct device *dev)
>
>  static inline bool dev_has_sync_state(struct device *dev)
>  {
> +       struct device_driver *drv;
> +
>         if (!dev)
>                 return false;
> -       if (dev->driver && dev->driver->sync_state)
> +       drv = READ_ONCE(dev->driver);
> +       if (drv && drv->sync_state)
>                 return true;
>         if (dev->bus && dev->bus->sync_state)
>                 return true;
>

On a somewhat related note, I'm wondering if this function can be
moved to drivers/base/base.h?

It doesn't look like having it defined in device.h is particularly useful.

Re: [PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Posted by Ulf Hansson 1 month, 1 week ago

On Mon, 20 Apr 2026 at 12:40, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sat, Apr 18, 2026 at 6:22 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > dev_has_sync_state() reads dev->driver twice without holding
> > device_lock() -- once for the NULL check and once to dereference
> > ->sync_state. Some callers only hold device_links_write_lock, which
> > doesn't prevent a concurrent unbind from clearing dev->driver via
> > device_unbind_cleanup().
> >
> > Fix it by reading dev->driver exactly once with READ_ONCE(), pairing
> > with the WRITE_ONCE() in device_set_driver().
> >
> > Link: https://lore.kernel.org/driver-core/DHW8QPU1VU1F.3P6PH69HLFBYC@kernel.org/
> > Fixes: ac338acf514e ("driver core: Add dev_has_sync_state()")
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>
> Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
>
> > ---
> >  include/linux/device.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index ac972e7bead4..4c1c9cb8570a 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -1018,9 +1018,12 @@ static inline void device_lock_assert(struct device *dev)
> >
> >  static inline bool dev_has_sync_state(struct device *dev)
> >  {
> > +       struct device_driver *drv;
> > +
> >         if (!dev)
> >                 return false;
> > -       if (dev->driver && dev->driver->sync_state)
> > +       drv = READ_ONCE(dev->driver);
> > +       if (drv && drv->sync_state)
> >                 return true;
> >         if (dev->bus && dev->bus->sync_state)
> >                 return true;
> >
>
> On a somewhat related note, I'm wondering if this function can be
> moved to drivers/base/base.h?
>
> It doesn't look like having it defined in device.h is particularly useful.

Apologize for the delay. Genpd needs the function [1] when it tries to
assign a common ->sync_state() callback for its providers.

Kind regards
Uffe

[1]
https://lore.kernel.org/all/20260410104058.83748-6-ulf.hansson@linaro.org/

Re: [PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Posted by Danilo Krummrich 1 month, 1 week ago

On Mon May 4, 2026 at 11:49 AM CEST, Ulf Hansson wrote:
> Apologize for the delay. Genpd needs the function [1] when it tries to
> assign a common ->sync_state() callback for its providers.

Can you add a revert of [1] to your patch series, so it gets (re-)exported with
a user?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/commit/?h=driver-core-next&id=9db268212e0d7c7e3c4aef3494e55afbc1695b1f

Re: [PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Posted by Ulf Hansson 1 month, 1 week ago

On Mon, 4 May 2026 at 12:30, Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon May 4, 2026 at 11:49 AM CEST, Ulf Hansson wrote:
> > Apologize for the delay. Genpd needs the function [1] when it tries to
> > assign a common ->sync_state() callback for its providers.
>
> Can you add a revert of [1] to your patch series, so it gets (re-)exported with
> a user?

Yes!

I was trying to avoid dependent changes to the driver core to funnel
my series entirely through my pmdomain tree. Anyway, perhaps you can
host an immutable branch for me to pull in, in this case.

Let's see, I will suggest something for the merge path in the cover
letter when I post my new version.

Kind regards
Uffe

>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/commit/?h=driver-core-next&id=9db268212e0d7c7e3c4aef3494e55afbc1695b1f

Re: [PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Posted by Danilo Krummrich 1 month, 1 week ago

On Mon May 4, 2026 at 8:43 PM CEST, Ulf Hansson wrote:
> On Mon, 4 May 2026 at 12:30, Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Mon May 4, 2026 at 11:49 AM CEST, Ulf Hansson wrote:
>> > Apologize for the delay. Genpd needs the function [1] when it tries to
>> > assign a common ->sync_state() callback for its providers.
>>
>> Can you add a revert of [1] to your patch series, so it gets (re-)exported with
>> a user?
>
> Yes!
>
> I was trying to avoid dependent changes to the driver core to funnel
> my series entirely through my pmdomain tree. Anyway, perhaps you can
> host an immutable branch for me to pull in, in this case.

I think you can just make the revert the first patch in your series and when it
is ready I can pick up only the revert in the driver-core tree; everything else
can go through your pmdomain tree.

I.e. you don't need the revert in your pmdomain tree as it should still have
dev_has_sync_state() exported in device.h.

Re: [PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Posted by Greg KH 1 month, 3 weeks ago

On Sat, Apr 18, 2026 at 06:22:18PM +0200, Danilo Krummrich wrote:
> dev_has_sync_state() reads dev->driver twice without holding
> device_lock() -- once for the NULL check and once to dereference
> ->sync_state. Some callers only hold device_links_write_lock, which
> doesn't prevent a concurrent unbind from clearing dev->driver via
> device_unbind_cleanup().
> 
> Fix it by reading dev->driver exactly once with READ_ONCE(), pairing
> with the WRITE_ONCE() in device_set_driver().
> 
> Link: https://lore.kernel.org/driver-core/DHW8QPU1VU1F.3P6PH69HLFBYC@kernel.org/
> Fixes: ac338acf514e ("driver core: Add dev_has_sync_state()")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  include/linux/device.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ac972e7bead4..4c1c9cb8570a 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1018,9 +1018,12 @@ static inline void device_lock_assert(struct device *dev)
>  
>  static inline bool dev_has_sync_state(struct device *dev)
>  {
> +	struct device_driver *drv;
> +
>  	if (!dev)
>  		return false;
> -	if (dev->driver && dev->driver->sync_state)
> +	drv = READ_ONCE(dev->driver);
> +	if (drv && drv->sync_state)
>  		return true;
>  	if (dev->bus && dev->bus->sync_state)
>  		return true;
> 
> base-commit: 5b484311507b5d403c1f7a45f6aa3778549e268b
> -- 
> 2.53.0
> 
> 

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] driver core: use READ_ONCE() for dev->driver in dev_has_sync_state()

Posted by Saravana Kannan 1 month, 4 weeks ago

On Sat, Apr 18, 2026 at 9:22 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> dev_has_sync_state() reads dev->driver twice without holding
> device_lock() -- once for the NULL check and once to dereference
> ->sync_state. Some callers only hold device_links_write_lock, which
> doesn't prevent a concurrent unbind from clearing dev->driver via
> device_unbind_cleanup().
>
> Fix it by reading dev->driver exactly once with READ_ONCE(), pairing
> with the WRITE_ONCE() in device_set_driver().
>
> Link: https://lore.kernel.org/driver-core/DHW8QPU1VU1F.3P6PH69HLFBYC@kernel.org/
> Fixes: ac338acf514e ("driver core: Add dev_has_sync_state()")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Saravana Kannan <saravanak@kernel.org>

-Saravana

> ---
>  include/linux/device.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ac972e7bead4..4c1c9cb8570a 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1018,9 +1018,12 @@ static inline void device_lock_assert(struct device *dev)
>
>  static inline bool dev_has_sync_state(struct device *dev)
>  {
> +       struct device_driver *drv;
> +
>         if (!dev)
>                 return false;
> -       if (dev->driver && dev->driver->sync_state)
> +       drv = READ_ONCE(dev->driver);
> +       if (drv && drv->sync_state)
>                 return true;
>         if (dev->bus && dev->bus->sync_state)
>                 return true;
>
> base-commit: 5b484311507b5d403c1f7a45f6aa3778549e268b
> --
> 2.53.0
>