From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.13]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4D3EB40B103 for ; Wed, 5 Aug 2026 09:45:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.13 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785923140; cv=none; b=riNn8fwm2ZLfpdxEHVJgMS37dQkMNUt8+4AH3JtuaE3Gdh75wIH0x/PCSFLi0a1Q0LjKHjK3eMPk9Gg0Q8/ohjR49ZMXP1UMfn6SnF3RozMd0/hrTk6VN64tx7C//LJsqa+6QVqncKTedLliyXnaEE9NZYASAR5kz9tUNLKkISU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785923140; c=relaxed/simple; bh=VbXQHh8evBzgMPcOjnNk5oHCvpn+mjwupKHiUdef7CY=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=FOj8BRJ/bavQzSnHrCaPoG4GIsD48tHhpgcvfmnj4Dq5/dXi/h4hcUsBvt+uI/vOHw+QaAC5ohdwSkg7rgSFMWTLci4KTn5D77cwz7fRH6GYJi/mbx587sBT1IAN9YWQgTfI3sf9/xOywntEQpBWZZmXdxN854iT5m8jU6zFQP4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=eV/132bA; arc=none smtp.client-ip=192.198.163.13 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="eV/132bA" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1785923138; x=1817459138; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=VbXQHh8evBzgMPcOjnNk5oHCvpn+mjwupKHiUdef7CY=; b=eV/132bAToN/Ei1FAI4dY+GB/oSJ0ryQRPClX+p+O18/dLNefJgmDqC2 PlpUdheEikOQlh45UB+IF6/Wt9sU8Vi2mLRb1lMAUyukVaNrJxTfN7wnE F904YjX+tCICRzGIB6JRbBuHoqb2bYNvDLxMAvN6bPaInJyHjB3L7674K K3JZEMRIk9eKFwcDHBaBzMydRjGXuB3XiX6Fjy/wCswT38cNFMuv9gmfv GCxndZLxAA3Lzf5lD3u5HnV6Kinr6T0UXH5NzJg6Xub23Y2J6ywd/64bi BdN7M8J5HatbIxtQy511OSx70AAnXCAWFXiPTzqZ2igpiMNFrBgllVbWc A==; X-CSE-ConnectionGUID: WYOMcg32RLahUJKazQsdUQ== X-CSE-MsgGUID: RSSlwJ4DRb6eRw86mhjUJw== X-IronPort-AV: E=McAfee;i="6800,10657,11865"; a="89015017" X-IronPort-AV: E=Sophos;i="6.25,206,1779174000"; d="scan'208";a="89015017" Received: from fmviesa004.fm.intel.com ([10.60.135.144]) by fmvoesa107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Aug 2026 02:45:37 -0700 X-CSE-ConnectionGUID: zw19Zo1eR1KWkXZ1vP8qNg== X-CSE-MsgGUID: +m8ykC3QREiergrF+UVGGQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.25,206,1779174000"; d="scan'208";a="263750057" Received: from amilburn-desk.amilburn-desk (HELO [10.245.244.54]) ([10.245.244.54]) by fmviesa004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Aug 2026 02:27:11 -0700 Message-ID: <7cc285c5-ecd2-4f65-8d2c-4c597b060841@linux.intel.com> Date: Wed, 5 Aug 2026 11:28:24 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] drm/i915/userptr: Fix user pages leak on get_pages failure To: Joonas Lahtinen , Jani Nikula , Rodrigo Vivi , Tvrtko Ursulin , ZhaoJinming , dri-devel@lists.freedesktop.org Cc: David Airlie , Simona Vetter , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, Kees Cook , Thomas Zimmermann , Matthew Auld References: <20260728054152.140524-1-zhaojinming@uniontech.com> <178591982239.30698.15708833006428973734@jlahtine-mobl> Content-Language: en-US From: Maarten Lankhorst In-Reply-To: <178591982239.30698.15708833006428973734@jlahtine-mobl> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hey, On 8/5/26 10:50, Joonas Lahtinen wrote: > Adding Maarten and Matt for below discussion points. > > Quoting ZhaoJinming (2026-07-28 08:41:52) >> When ____i915_gem_object_get_pages() fails inside >> i915_gem_object_userptr_submit_init(), the pvec containing pages >> pinned by pin_user_pages_fast() was never freed: >> >> 1. obj->userptr.pvec was set to the pinned pages >> 2. The local pvec variable was NULLed >> 3. ____i915_gem_object_get_pages() failed, but its internal >> i915_gem_object_userptr_drop_ref() only decremented page_ref >> from 2 to 1, not triggering the pvec cleanup >> 4. The unconditional obj->userptr.page_ref-- brought page_ref >> to 0, but the pinned pages remained referenced only by >> obj->userptr.pvec with no path to reclaim them >> >> Additionally, the cache hit path could return success on a >> subsequent call despite page_ref being 0, leading to a >> GEM_BUG_ON(obj->userptr.page_ref < 0) crash in drop_ref when >> the pages were eventually invalidated. > > That's a very verbose way of saying that error handling is missing > for ____i915_gem_object_get_pages failures here. > >> Fix by calling i915_gem_object_userptr_drop_ref() on the >> get_pages failure path, which properly decrements page_ref >> from 1 to 0, triggering the pvec cleanup. Make the page_ref-- >> after the if block conditional on success, since drop_ref >> already handles the refcount on failure. > > Not sure that is the right fix, seems that i915_gem_userptr_get_pages > is rigged in a way that in the intro page_ref >= 1 is required, and is > unconditionally increased (becomes >= 2), then on error path drop_ref is > called which will unconditionally decrease but never free because page_ref > always remains at least 1. I think that is the real bug which is being papered > over here. Maarten, am I missing something? > > This seems to be introduced by commit: > > ed29c2691188 ("drm/i915: Fix userptr so we do not have to worry about obj->mm.lock, v7.") > > As for the original report, lack of error handling seems to be introduced > by commit: > > b4b9731b02c3 ("drm/i915: Simplify userptr locking") > > The commit message goes on to explain it a little bit, but this chunk from > original patch still seems quite odd? > > -------------------------- 8< -------------------------- > @@ -341,12 +317,14 @@ int i915_gem_object_userptr_submit_init(struct drm_i915_gem_object *obj) > if (!obj->userptr.page_ref++) { > obj->userptr.pvec = pvec; > obj->userptr.notifier_seq = notifier_seq; > - > pvec = NULL; > + ret = ____i915_gem_object_get_pages(obj); > } > > + obj->userptr.page_ref--; > + > out_unlock: > - spin_unlock(&i915->mm.notifier_lock); > + i915_gem_object_unlock(obj); > > out: > if (pvec) { > -------------------------- 8< -------------------------- > > Regards, Joonas > >> >> Signed-off-by: ZhaoJinming >> --- >> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> index 043095f93ac6..7d2750528485 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >> @@ -292,9 +292,12 @@ int i915_gem_object_userptr_submit_init(struct drm_i915_gem_object *obj) >> obj->userptr.notifier_seq = notifier_seq; >> pvec = NULL; >> ret = ____i915_gem_object_get_pages(obj); >> + if (ret) >> + i915_gem_object_userptr_drop_ref(obj); >> } >> >> - obj->userptr.page_ref--; >> + if (!ret) >> + obj->userptr.page_ref--; >> >> out_unlock: >> i915_gem_object_unlock(obj); >> -- >> 2.20.1 >> I completely and entirely forgot everything related to i915 userpointer code, so I'm not much of help here. It seems that i915_gem_userptr_get_pages() checks if page-ref is 0, then increases the page ref on success, which we then use to decrease our original page ref. The patch by ZhaoJinming removes correct handling of the case where page_ref is already >= 0 Might be better to do change it to if (!obj->userptr.page_ref) { obj->userptr.pvec = pvec; obj->userptr.notifier_seq = notifier_seq; pvec = NULL; ret = ____i915_gem_object_get_pages(obj); } and in i915_gem_userptr_get_pages, look for !userptr.pvec instead of !obj->userptr.page_ref Kind regards, ~Maarten Lankhorst