[Intel-gfx] [PATCH 11/39] drm/i915: Allow userspace to clone contexts on creation

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-03-14 17:49:49)
> 
> On 14/03/2019 16:54, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-03-14 16:18:17)
> >>
> >> On 13/03/2019 14:43, Chris Wilson wrote:
> >>> +     if (local.flags & I915_CONTEXT_CLONE_VM) {
> >>> +             struct i915_hw_ppgtt *ppgtt;
> >>> +
> >>> +             do {
> >>> +                     ppgtt = READ_ONCE(src->ppgtt);
> >>> +                     if (!ppgtt)
> >>> +                             break;
> >>> +
> >>> +                     if (!kref_get_unless_zero(&ppgtt->ref))
> >>> +                             continue;
> >>> +
> >>> +                     if (ppgtt == READ_ONCE(src->ppgtt))
> >>> +                             break;
> >>
> >> This loop needs a comment. For instance I don't understand what is this
> >> line for. Firstly, we have a reference on ppgtt, so what do we care if
> >> the source context in the meantime changed it? Secondly, even though it
> >> matches now, why would it still match when we exit the loop?
> > 
> > Because it may have been reallocated between the read and the kref. Once
> > we have the kref, and passed through the strong memory barrier, can we
> > decide if this is the ppgtt that we wanted. It may indeed be released
> > from src->ppgtt after this point, but it can no longer be reused
> > elsewhere, so we can not inadvertently share the ppgtt from a third
> > party.
> 
> I don't get it. As soon as we have obtained a reference we could proceed 
> I think. It was a ppgtt which was set in the source context at one 
> point. It may not be by the time we decide to replace ours with it, but 
> apart from doing all under struct_mutex (to match set_ppgtt) I don't see 
> what another check after kref brings.

Without the check, it's a use-after-free.
-Chris