linux-kernel - Re: [PATCH 4.19 11/57] drm/atomic_helper: Stop modesets on unregistered connectors harder

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKMK7uHw8gJEGQcCwR-MSEpndRKDZwO2BsveDThBSACxZfxLrw@mail.gmail.com>
Date:   Tue, 1 Dec 2020 17:47:12 +0100
From:   Daniel Vetter <daniel.vetter@...ll.ch>
To:     Pavel Machek <pavel@....cz>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        David Airlie <airlied@...ux.ie>, Lyude Paul <lyude@...hat.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Christoph Niedermaier <cniedermaier@...electronics.com>
Subject: Re: [PATCH 4.19 11/57] drm/atomic_helper: Stop modesets on
 unregistered connectors harder

On Tue, Dec 1, 2020 at 4:43 PM Pavel Machek <pavel@....cz> wrote:
>
> Hi!
>
> > From: Lyude Paul <lyude@...hat.com>
> >
> > commit de9f8eea5a44b0b756d3d6345af7f8e630a3c8c0 upstream.
>
> So this says protected by mutex:
>
> >       /**
> > -      * @registered: Is this connector exposed (registered) with userspace?
> > +      * @registration_state: Is this connector initializing, exposed
> > +      * (registered) with userspace, or unregistered?
> > +      *
> >        * Protected by @mutex.
> >        */
> > -     bool registered;
> > +     enum drm_connector_registration_state registration_state;
> >
> >       /**
> >        * @modes:
> > @@ -1165,6 +1214,24 @@ static inline void drm_connector_unrefer
> >       drm_connector_put(connector);
> >  }
> >
> > +/**
> > + * drm_connector_is_unregistered - has the connector been unregistered from
> > + * userspace?
> > + * @connector: DRM connector
> > + *
> > + * Checks whether or not @connector has been unregistered from userspace.
> > + *
> > + * Returns:
> > + * True if the connector was unregistered, false if the connector is
> > + * registered or has not yet been registered with userspace.
> > + */
> > +static inline bool
> > +drm_connector_is_unregistered(struct drm_connector *connector)
> > +{
> > +     return READ_ONCE(connector->registration_state) ==
> > +             DRM_CONNECTOR_UNREGISTERED;
> > +}
>
>
> But this uses READ_ONCE() for protection, and corresponding
> WRITE_ONCE() is nowhere to be seen. Should this take the mutex, too?

The read once here doesn't protect against any races, but just against
creative compilers doing funny stuff that might really break code
logic. I guess for symmetry we could throw the WRITE_ONCE on the write
side, but it really shouldn't matter, the entire thing is racy by
design. We0d also only ever need the write once on the unregister
call.
-Daniel

>
> Best regards,
>                                                                 Pavel
> --
> http://www.livejournal.com/~pavelmachek



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch