linux-kernel - Re: [PATCH v3] drm/fence: allow fence waiting to be interrupted by userspace

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] [day] [month] [year] [list]

Message-ID: <20160912142332.GB12271@joana>
Date:   Mon, 12 Sep 2016 11:23:32 -0300
From:   Gustavo Padovan <gustavo@...ovan.org>
To:     Sean Paul <seanpaul@...gle.com>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        Stéphane Marchesin <marcheu@...gle.com>,
        Daniel Stone <daniels@...labora.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
        John Harrison <John.C.Harrison@...el.com>, m.chehab@...sung.com
Subject: Re: [PATCH v3] drm/fence: allow fence waiting to be interrupted by
 userspace

Hi Sean,

2016-09-12 Sean Paul <seanpaul@...gle.com>:

> On Thu, Aug 25, 2016 at 12:47 PM, Gustavo Padovan <gustavo@...ovan.org> wrote:
> > From: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
> >
> > If userspace is running an synchronously atomic commit and interrupts the
> > atomic operation during fence_wait() it will hang until the timer expires,
> > so here we change the wait to be interruptible so it stop immediately when
> > userspace wants to quit.
> >
> > Also adds the necessary error checking for fence_wait().
> >
> > v2: Comment by Daniel Vetter
> >         - Add error checking for fence_wait()
> >
> > v3: Rebase on top of new atomic noblocking support
> >
> > v4: Comment by Maarten Lankhorst
> >         - remove 'swapped' bitfield as it was duplicating information
> >
> > v5: Comments by Maarten Lankhorst
> >         - assign plane->state to plane_state if !intr
> >         - squash previous patch into this one
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++++++++++++++++--------
> >  drivers/gpu/drm/msm/msm_atomic.c    |  2 +-
> >  include/drm/drm_atomic_helper.h     |  5 +++--
> >  3 files changed, 37 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index e1f5de2..f752f9c 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1007,29 +1007,47 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
> >   * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
> >   * @dev: DRM device
> >   * @state: atomic state object with old state structures
> > + * @intr: if true, do an interruptible wait
> >   *
> >   * For implicit sync, driver should fish the exclusive fence out from the
> >   * incoming fb's and stash it in the drm_plane_state.  This is called after
> >   * drm_atomic_helper_swap_state() so it uses the current plane state (and
> >   * just uses the atomic state to find the changed planes)
> > + *
> > + * Returns zero if sucess or < 0 if fence_wait() fails.
> >   */
> > -void drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> > -                           struct drm_atomic_state *state)
> > +int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> > +                                      struct drm_atomic_state *state,
> > +                                      bool intr)
> >  {
> >         struct drm_plane *plane;
> >         struct drm_plane_state *plane_state;
> > -       int i;
> > +       int i, ret;
> >
> >         for_each_plane_in_state(state, plane, plane_state, i) {
> > -               if (!plane->state->fence)
> > +               /*
> > +                * If the caller asks for an interruptible wait it means
> > +                * that the state were not swapped yet and the operation
> > +                * can still be interrupted by userspace, so we need
> > +                * to look to plane_state instead.
> > +                */
> > +               if (!intr)
> > +                       plane_state = plane->state;
> 
> The "intr" name is tripping me up here, since it doesn't _really_ imply that the
> state hasn't been swapped yet (this is just an implementation detail
> of the caller(s).
> 
> Perhaps we could change the name to "pre_swap", and remove the comment
> above this conditonal. Below, above
> fence_wait, add a new comment to the effect of:
> 
> /*
>  * If waiting for fences pre-swap (ie: nonblock), userspace can still interrupt
>  * the operation. Instead of blocking until the timer expires, make the wait
>  * interruptible.
>  */

Indeed, that is much better naming. In this case specifically intr has
the same meaning as pre_swap, but as you said it is an implementation
detail and we need to avoid people to get it wrong.

I'll update it and send a v4.

Gustavo