[Intel-gfx] [PATCH 4/4] drm/i915: Protect timeline->hwsp dereferencing

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-19 11:48:25)
> 
> On 18/09/2019 15:54, Chris Wilson wrote:
> > As not only is the signal->timeline volatile, so will be acquiring the
> > timeline's HWSP. We must first carefully acquire the timeline from the
> > signaling request and then lock the timeline. With the removal of the
> > struct_mutex serialisation of request construction, we can have multiple
> > timelines active at once, and so we must avoid using the nested mutex
> > lock as it is quite possible for both timelines to be establishing
> > semaphores on the other and so deadlock.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_timeline.c | 32 ++++++++++++++++++------
> >   1 file changed, 25 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c
> > index 2ce81bdf7f86..7b1d4098dd2e 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_timeline.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
> > @@ -500,17 +500,32 @@ int intel_timeline_read_hwsp(struct i915_request *from,
> >                            struct i915_request *to,
> >                            u32 *hwsp)
> >   {
> > -     struct intel_timeline_cacheline *cl = from->hwsp_cacheline;
> > -     struct intel_timeline *tl = from->timeline;
> 
> I think this should have been annotated in 2/4.

I left it un-annotated so that it would be left with a warning, as I felt
this change was substantial enough to merit its own patch.

> > +     struct intel_timeline *tl;
> >       int err;
> >   
> > +     rcu_read_lock();
> > +     tl = rcu_dereference(from->timeline);
> > +     if (i915_request_completed(from) || !kref_get_unless_zero(&tl->kref))
> > +             tl = NULL;
> > +     rcu_read_unlock();
> > +     if (!tl) /* already completed */
> > +             return 1;
> > +
> >       GEM_BUG_ON(rcu_access_pointer(to->timeline) == tl);
> >   
> > -     mutex_lock_nested(&tl->mutex, SINGLE_DEPTH_NESTING);
> > -     err = i915_request_completed(from);
> > -     if (!err)
> > +     err = -EBUSY;
> > +     if (mutex_trylock(&tl->mutex)) {
> > +             struct intel_timeline_cacheline *cl = from->hwsp_cacheline;
> > +
> > +             if (i915_request_completed(from)) {
> > +                     err = 1;
> > +                     goto unlock;
> > +             }
> > +
> >               err = cacheline_ref(cl, to);
> > -     if (!err) {
> > +             if (err)
> > +                     goto unlock;
> > +
> >               if (likely(cl == tl->hwsp_cacheline)) {
> >                       *hwsp = tl->hwsp_offset;
> >               } else { /* across a seqno wrap, recover the original offset */
> > @@ -518,8 +533,11 @@ int intel_timeline_read_hwsp(struct i915_request *from,
> >                               ptr_unmask_bits(cl->vaddr, CACHELINE_BITS) *
> >                               CACHELINE_BYTES;
> >               }
> > +
> > +unlock:
> > +             mutex_unlock(&tl->mutex);
> >       }
> > -     mutex_unlock(&tl->mutex);
> > +     intel_timeline_put(tl);
> >   
> >       return err;
> >   }
> > 
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Thanks,
-Chris