Re: [PATCH] drm: damage_helper: Fix race checking plane->state->fb

[Sean Paul <sean@poorly.run>]

On Thu, Sep 05, 2019 at 12:41:27PM +0200, Daniel Vetter wrote:
> On Wed, Sep 4, 2019 at 10:29 PM Sean Paul <sean@poorly.run> wrote:
> >
> > From: Sean Paul <seanpaul@chromium.org>
> >
> > Since the dirtyfb ioctl doesn't give us any hints as to which plane is
> > scanning out the fb it's marking as damaged, we need to loop through
> > planes to find it.
> >
> > Currently we just reach into plane state and check, but that can race
> > with another commit changing the fb out from under us. This patch locks
> > the plane before checking the fb and will release the lock if the plane
> > is not displaying the dirty fb.
> >
> > Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb")
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Deepak Rawat <drawat@vmware.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v5.0+
> > Reported-by: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_damage_helper.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> > index 8230dac01a89..3a4126dc2520 100644
> > --- a/drivers/gpu/drm/drm_damage_helper.c
> > +++ b/drivers/gpu/drm/drm_damage_helper.c
> > @@ -212,8 +212,14 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> >         drm_for_each_plane(plane, fb->dev) {
> >                 struct drm_plane_state *plane_state;
> >
> > -               if (plane->state->fb != fb)
> > +               ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx);
> > +               if (ret)
> 
> I think for paranoid safety we should have a WARN_ON(ret == -EALREADY)
> here. It should be impossible, but if it's not for some oddball
> reason, we'll blow up.

drm_modeset_lock eats EALREADY and returns 0 for that case, so I guess it
depends _how_ paranoid you want to be here :-)

> 
> With that: Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
> 
> But please give this a spin with some workloads and the ww_mutex
> slowpath debugging enabled, just to makre sure.

Ok, had a chance to run through some tests this morning with
CONFIG_DEBUG_WW_MUTEX_SLOWPATH and things lgtm

Sean

> -Daniel
> 
> > +                       goto out;
> > +
> > +               if (plane->state->fb != fb) {
> > +                       drm_modeset_unlock(&plane->mutex);
> >                         continue;
> > +               }
> >
> >                 plane_state = drm_atomic_get_plane_state(state, plane);
> >                 if (IS_ERR(plane_state)) {
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Sean Paul, Software Engineer, Google / Chromium OS