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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
