From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 591CBC433EF for ; Wed, 22 Sep 2021 11:35:23 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EA9DD60E09 for ; Wed, 22 Sep 2021 11:35:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org EA9DD60E09 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B449B6EB82; Wed, 22 Sep 2021 11:35:20 +0000 (UTC) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id AA4416EB80; Wed, 22 Sep 2021 11:35:19 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10114"; a="220376078" X-IronPort-AV: E=Sophos;i="5.85,313,1624345200"; d="scan'208";a="220376078" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Sep 2021 04:35:03 -0700 X-IronPort-AV: E=Sophos;i="5.85,313,1624345200"; d="scan'208";a="474525554" Received: from mmazarel-mobl1.ger.corp.intel.com (HELO [10.249.254.175]) ([10.249.254.175]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Sep 2021 04:35:01 -0700 Subject: Re: [PATCH 2/3] drm/i915/ttm: Fix lockdep warning in __i915_gem_free_object() To: Matthew Auld Cc: Intel Graphics Development , ML dri-devel , Maarten Lankhorst , Matthew Auld References: <20210922083807.888206-1-thomas.hellstrom@linux.intel.com> <20210922083807.888206-3-thomas.hellstrom@linux.intel.com> From: =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= Message-ID: Date: Wed, 22 Sep 2021 13:34:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 9/22/21 12:55 PM, Matthew Auld wrote: > On Wed, 22 Sept 2021 at 09:38, Thomas Hellström > wrote: >> In the mman selftest, some tests make the ttm_bo_init_reserved() fail, >> which may trigger a call to the i915_ttm_bo_destroy() function. >> However, at this point the gem object refcount is set to 1, which >> triggers a lockdep warning in __i915_gem_free_object() and a >> corresponding failure in DG1 BAT, i915_selftest@live@mman. >> >> Fix this by clearing the gem object refcount if called from that >> failure path. >> >> Fixes: f9b23c157a78 ("drm/i915: Move __i915_gem_free_object to ttm_bo_destroy") >> Cc: Maarten Lankhorst >> Signed-off-by: Thomas Hellström >> --- >> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> index b94497989995..b1f561543ff3 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> @@ -900,6 +900,10 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) >> >> i915_ttm_backup_free(obj); >> >> + /* Failure during ttm_bo_init_reserved leaves the refcount set to 1. */ >> + if (IS_ENABLED(CONFIG_LOCKDEP) && !obj->ttm.created) >> + refcount_set(&obj->base.refcount.refcount, 0); >> + >> /* This releases all gem object bindings to the backend. */ >> __i915_gem_free_object(obj); > The __i915_gem_free_object is also nuking stuff like mm.placements, > which is still owned by the caller AFAIK, or at least it is until we > have successfully initialised the object, so smells like potential > double free? Can we easily move that under the ttm.created check? > Otherwise maybe we are meant to move the mm.placements handling into > the RCU callback? Yes, it indeed sounds like a closer look is needed for the error handling here. Perhaps it makes sense to initialize the TTM part and then the GEM part while still having the lock. Meanwhile I'll put it under the ttm.created check. Thanks, Thomas > >> -- >> 2.31.1 >>