On la, 2015-07-11 at 21:54 +0100, Chris Wilson wrote:
> On Thu, Jul 09, 2015 at 12:59:05PM +0300, Imre Deak wrote:
> > +static int
> > +__i915_gem_userptr_set_pages(struct drm_i915_gem_object *obj,
> > + struct page **pvec, int num_pages)
> > +{
> > + int ret;
> > +
> > + ret = st_set_pages(&obj->pages, pvec, num_pages);
> > + if (ret)
> > + return ret;
> > +
> > + ret = i915_gem_gtt_prepare_object(obj);
> > + if (ret) {
> > + sg_free_table(obj->pages);
> > + kfree(obj->pages);
> > + obj->pages = NULL;
>
> Oh dear, we just leaked a ref one each page.
To summarize the IRC discussion on this: it would be logical that
sg_set_page() takes a ref - and in that case this would result in
leaking those refs - but this is not so. Instead we rely on the GUP refs
which we keep in case of success by setting pinned=0 (release_pages will
be a nop) and drop in case of failure by passing the original pinned
value to release_pages(). So after checking both the sync and async
userptr paths this looks ok to me.
--Imre
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html