From mboxrd@z Thu Jan  1 00:00:00 1970
From: Imre Deak <imre.deak@intel.com>
Subject: Re: [PATCH v3 1/2] drm/i915: avoid leaking DMA mappings
Date: Mon, 13 Jul 2015 15:15:05 +0300
Message-ID: <1436789705.16551.6.camel@intel.com>
References: <1436372339-28242-1-git-send-email-imre.deak@intel.com>
	 <1436435945-13608-1-git-send-email-imre.deak@intel.com>
	 <20150711205435.GM21656@nuc-i3427.alporthouse.com>
Reply-To: imre.deak@intel.com
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Return-path: <stable-owner@vger.kernel.org>
In-Reply-To: <20150711205435.GM21656@nuc-i3427.alporthouse.com>
Sender: stable-owner@vger.kernel.org
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org, Daniel Vetter <daniel.vetter@ffwll.ch>, Jani Nikula <jani.nikula@intel.com>, stable@vger.kernel.org
List-Id: intel-gfx@lists.freedesktop.org

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