On 2019.04.04 09:11:55 +0100, Chris Wilson wrote:
> Quoting Zhenyu Wang (2019-04-04 08:48:34)
> > On 2019.04.04 08:30:56 +0100, Chris Wilson wrote:
> > > ppgtt_free_all_spt() iterates the radixtree as it is deleting it,
> > > forgoing all protection against the leaves being freed in the process
> > > (leaving the iter pointing into the void).
> > > 
> > > A minimal fix seems to be to use the available post_shadow_list to
> > > decompose the tree into a list prior to destroying the radixtree.
> > > 
> > > Alerted by the sparse warnings:
> > > 
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void **slot
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void [noderef] <asn:4> **
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void **slot
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void [noderef] <asn:4> **
> > > drivers/gpu/drm/i915/gvt/gtt.c:758:45: warning: incorrect type in argument 1 (different address spaces)
> > > drivers/gpu/drm/i915/gvt/gtt.c:758:45:    expected void [noderef] <asn:4> **slot
> > > drivers/gpu/drm/i915/gvt/gtt.c:758:45:    got void **slot
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in argument 1 (different address spaces)
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void [noderef] <asn:4> **slot
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void **slot
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9: warning: incorrect type in assignment (different address spaces)
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    expected void **slot
> > > drivers/gpu/drm/i915/gvt/gtt.c:757:9:    got void [noderef] <asn:4> **
> > > 
> > > This would also have been loudly warning if run through CI for the
> > > invalid RCU dereferences.
> > > 
> > > Fixes: b6c126a39345 ("drm/i915/gvt: Manage shadow pages with radix tree")
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Changbin Du <changbin.du@intel.com>
> > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > Cc: Zhi Wang <zhi.a.wang@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gvt/gtt.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> > > index cf133ef03873..9814773882ec 100644
> > > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > > @@ -750,14 +750,20 @@ static void ppgtt_free_spt(struct intel_vgpu_ppgtt_spt *spt)
> > >  
> > >  static void ppgtt_free_all_spt(struct intel_vgpu *vgpu)
> > >  {
> > > -     struct intel_vgpu_ppgtt_spt *spt;
> > > +     struct intel_vgpu_ppgtt_spt *spt, *spn;
> > >       struct radix_tree_iter iter;
> > > -     void **slot;
> > > +     LIST_HEAD(all_spt);
> > > +     void __rcu **slot;
> > >  
> > > +     rcu_read_lock();
> > >       radix_tree_for_each_slot(slot, &vgpu->gtt.spt_tree, &iter, 0) {
> > >               spt = radix_tree_deref_slot(slot);
> > > -             ppgtt_free_spt(spt);
> > > +             list_move(&spt->post_shadow_list, &all_spt);
> > >       }
> > > +     rcu_read_unlock();
> > > +
> > > +     list_for_each_entry_safe(spt, spn, &all_spt, post_shadow_list)
> > > +             ppgtt_free_spt(spt);
> > >  }
> > >
> > 
> > As we ensure to flush post shadow list, so this is safe to reuse.
> 
> Phew! I looked, couldn't see that it would be used at this point, so
> hoped for the best.
>  
> > Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> 
> Will you take both of these patches through your tree?

Yes.

Thanks!

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827