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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 7E933C25B6B for ; Wed, 25 Oct 2023 21:39:24 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E40D510E5E2; Wed, 25 Oct 2023 21:39:23 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id DB66D10E5E2 for ; Wed, 25 Oct 2023 21:39:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1698269961; x=1729805961; h=from:date:subject:mime-version:content-transfer-encoding: message-id:to:cc; bh=M9YR0XWgWOmEjasdaF5dvVIdmizBqpKx0yyPpSgRQEE=; b=Ht77Mgblu311F+jcWZzC0kO8EdTLQDtqF45MKDUbanFi91w8oda2KYru QTdmRQXWLcuF6YlRhc0zaYMOIPE+81cqAOTr2FhK5mQrvmOnYvd/Yj3xg 24v6oFnlSlKmMWmAmCaiAAYpRZoOq4IsoQ3+h9SPDxB82jkhA91pXLwKA 4L1eJ4MYUP0PRraWdYGREzf28lcEcz+fWesBNxEO4NV5kEjWLO1nxsDqm h9tsVktIRAbeibVwCc3ZMK5AIL0Dmn568Jhd7iCw8RkL3wsEDew/ZHaGO c9RLkDzGeFKAZgXS0WG3lcVOgzilcxr35S6a073ep7/nuC/Quix2b0/0h w==; X-IronPort-AV: E=McAfee;i="6600,9927,10874"; a="390263665" X-IronPort-AV: E=Sophos;i="6.03,250,1694761200"; d="scan'208";a="390263665" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Oct 2023 14:39:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10874"; a="849677457" X-IronPort-AV: E=Sophos;i="6.03,250,1694761200"; d="scan'208";a="849677457" Received: from lab-ah.igk.intel.com ([10.102.138.202]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Oct 2023 14:39:16 -0700 From: Andrzej Hajda Date: Wed, 25 Oct 2023 23:39:07 +0200 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20231025-debugobjects_fix-v3-1-2bc3bf7084c2@intel.com> X-B4-Tracking: v=1; b=H4sIAPqKOWUC/z3MTQ7CIBCG4as0rKUWKK115T2MMQNMC40FA/UnN r271IXLdzLfs5CE0WEix2IhEZ8uueBziF1BtAU/IHUmN+EVF6zikhpUjyGoEfWcrr1706ZByTs uZatrkmf3iPn8I8+X3H0ME51tRPhDVX5nggnZlbyrDwfeUkbBm/jBsbQwGjg5P+Ot1GHaSAUJq Yrgtd0IVe9foBRZ1y/zpBUyvwAAAA== To: linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org, linux-mm@kvack.org X-Mailer: b4 0.12.3 X-Developer-Signature: v=1; a=openpgp-sha256; l=11404; i=andrzej.hajda@intel.com; h=from:subject:message-id; bh=M9YR0XWgWOmEjasdaF5dvVIdmizBqpKx0yyPpSgRQEE=; b=owEB7QES/pANAwAKASNispPeEP3XAcsmYgBlOYsDrBMPiLwxsaT9p7frP5Jp6GZUcLUmVQuloKaH 0KwwZ0OJAbMEAAEKAB0WIQT8qEQxNN2/XeF/A00jYrKT3hD91wUCZTmLAwAKCRAjYrKT3hD910unC/ 9guaFzM2bfO+yD/g91t70H8/PquAzFoHsNO9DiLcYCUy0lVikzyP0xydf1mfrpC1M4zua8pdhsmkUJ DJ4o1CJ2ofv4Z8kBXJ2P2J/BCmzIfcl6dDYWFJ3D5TOwdG09AC1ftUET1OtRXh1xPV8YjH9+EMGdcK L3s+dap7EOqC5DCXYAp8mk6VYiZOCf9dJzmq9TpW619cEnqhfKT4XA2fXXMwyYSmLtxok3kAqvS+8B F3AqbqvoljTNxSnGZ/IOCk7jaBlA8gjbbI3IQLSSpmxHFlhuWMIRn6aCePS2LhSlKt4pe+uyM+uQZC MZKBMP/FnZp6xFB1GaCpid0yfnrkYdFw+N0iHDJ4e6W/dMHFZ94VsZDcFW0bsH9c3x4/I2gEUhgoWB EQYr09yu9uASGfdmEQxZWs5wXdRZnu62jQQ+kQbr97HgfSHv28i8GJAU+Mn1y1KdZEE5pVegvlwLqI kIFmWqUEu5IhaIRv7gpQFpOnqgX+5V7Hvwnl75KH25b1Y= X-Developer-Key: i=andrzej.hajda@intel.com; a=openpgp; fpr=FCA8443134DDBF5DE17F034D2362B293DE10FDD7 Subject: [Intel-gfx] [PATCH v3] debugobjects: stop accessing objects after releasing spinlock X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Thomas Gleixner , Andrzej Hajda , Nirmoy Das Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" After spinlock release object can be modified/freed by concurrent thread. Using it in such case is error prone, even for printing object state. To avoid such situation local copy of the object is created if necessary. Sample buggy scenario: 1. Thread tries to deactivate destroyed object, debugobjects detects it, spin lock is released, thread is preempted. 2. Other thread frees debugobject, then allocates new one on the same memory location, ie 'obj' variable from 1st thread point to it - it is possible because there is no locking. 3. Then preemption occurs, and 1st thread reports error for wrong object. Signed-off-by: Andrzej Hajda --- v2: add missing switch breaks v3: abandon single-point-of-unlock approach --- lib/debugobjects.c | 196 +++++++++++++++++++++-------------------------------- 1 file changed, 77 insertions(+), 119 deletions(-) diff --git a/lib/debugobjects.c b/lib/debugobjects.c index a517256a270b71..c074dbbec084a6 100644 --- a/lib/debugobjects.c +++ b/lib/debugobjects.c @@ -620,9 +620,8 @@ static void debug_objects_fill_pool(void) static void __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack) { - enum debug_obj_state state; struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; debug_objects_fill_pool(); @@ -643,24 +642,18 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: obj->state = ODEBUG_STATE_INIT; - break; - - case ODEBUG_STATE_ACTIVE: - state = obj->state; raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "init"); - debug_object_fixup(descr->fixup_init, addr, state); - return; - - case ODEBUG_STATE_DESTROYED: - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "init"); return; default: break; } + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); + debug_print_object(&o, "init"); + + if (o.state == ODEBUG_STATE_ACTIVE) + debug_object_fixup(descr->fixup_init, addr, o.state); } /** @@ -701,11 +694,9 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_stack); int debug_object_activate(void *addr, const struct debug_obj_descr *descr) { struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; - enum debug_obj_state state; struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; - int ret; if (!debug_objects_enabled) return 0; @@ -717,49 +708,38 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr) raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object_or_alloc(addr, db, descr, false, true); - if (likely(!IS_ERR_OR_NULL(obj))) { - bool print_object = false; - + if (unlikely(!obj)) { + raw_spin_unlock_irqrestore(&db->lock, flags); + debug_objects_oom(); + return 0; + } else if (likely(!IS_ERR(obj))) { switch (obj->state) { - case ODEBUG_STATE_INIT: - case ODEBUG_STATE_INACTIVE: - obj->state = ODEBUG_STATE_ACTIVE; - ret = 0; - break; - case ODEBUG_STATE_ACTIVE: - state = obj->state; - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "activate"); - ret = debug_object_fixup(descr->fixup_activate, addr, state); - return ret ? 0 : -EINVAL; - case ODEBUG_STATE_DESTROYED: - print_object = true; - ret = -EINVAL; break; + case ODEBUG_STATE_INIT: + case ODEBUG_STATE_INACTIVE: + obj->state = ODEBUG_STATE_ACTIVE; + fallthrough; default: - ret = 0; - break; + raw_spin_unlock_irqrestore(&db->lock, flags); + return 0; } - raw_spin_unlock_irqrestore(&db->lock, flags); - if (print_object) - debug_print_object(obj, "activate"); - return ret; } + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); + debug_print_object(&o, "activate"); - /* If NULL the allocation has hit OOM */ - if (!obj) { - debug_objects_oom(); - return 0; + switch (o.state) { + case ODEBUG_STATE_ACTIVE: + case ODEBUG_STATE_NOTAVAILABLE: + if (debug_object_fixup(descr->fixup_activate, addr, o.state)) + return 0; + fallthrough; + default: + return -EINVAL; } - - /* Object is neither static nor tracked. It's not initialized */ - debug_print_object(&o, "activate"); - ret = debug_object_fixup(descr->fixup_activate, addr, ODEBUG_STATE_NOTAVAILABLE); - return ret ? 0 : -EINVAL; } EXPORT_SYMBOL_GPL(debug_object_activate); @@ -770,10 +750,10 @@ EXPORT_SYMBOL_GPL(debug_object_activate); */ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) { + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; - bool print_object = false; if (!debug_objects_enabled) return; @@ -785,33 +765,24 @@ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) obj = lookup_object(addr, db); if (obj) { switch (obj->state) { + case ODEBUG_STATE_DESTROYED: + break; case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: case ODEBUG_STATE_ACTIVE: - if (!obj->astate) - obj->state = ODEBUG_STATE_INACTIVE; - else - print_object = true; - break; - - case ODEBUG_STATE_DESTROYED: - print_object = true; - break; + if (obj->astate) + break; + obj->state = ODEBUG_STATE_INACTIVE; + fallthrough; default: - break; + raw_spin_unlock_irqrestore(&db->lock, flags); + return; } + o = *obj; } raw_spin_unlock_irqrestore(&db->lock, flags); - if (!obj) { - struct debug_obj o = { .object = addr, - .state = ODEBUG_STATE_NOTAVAILABLE, - .descr = descr }; - - debug_print_object(&o, "deactivate"); - } else if (print_object) { - debug_print_object(obj, "deactivate"); - } + debug_print_object(&o, "deactivate"); } EXPORT_SYMBOL_GPL(debug_object_deactivate); @@ -822,11 +793,9 @@ EXPORT_SYMBOL_GPL(debug_object_deactivate); */ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) { - enum debug_obj_state state; struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; - bool print_object = false; if (!debug_objects_enabled) return; @@ -836,32 +805,31 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object(addr, db); - if (!obj) - goto out_unlock; + if (!obj) { + raw_spin_unlock_irqrestore(&db->lock, flags); + return; + } switch (obj->state) { + case ODEBUG_STATE_ACTIVE: + case ODEBUG_STATE_DESTROYED: + break; case ODEBUG_STATE_NONE: case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: obj->state = ODEBUG_STATE_DESTROYED; - break; - case ODEBUG_STATE_ACTIVE: - state = obj->state; + fallthrough; + default: raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "destroy"); - debug_object_fixup(descr->fixup_destroy, addr, state); return; - - case ODEBUG_STATE_DESTROYED: - print_object = true; - break; - default: - break; } -out_unlock: + + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); - if (print_object) - debug_print_object(obj, "destroy"); + debug_print_object(&o, "destroy"); + + if (o.state == ODEBUG_STATE_ACTIVE) + debug_object_fixup(descr->fixup_destroy, addr, o.state); } EXPORT_SYMBOL_GPL(debug_object_destroy); @@ -872,9 +840,8 @@ EXPORT_SYMBOL_GPL(debug_object_destroy); */ void debug_object_free(void *addr, const struct debug_obj_descr *descr) { - enum debug_obj_state state; struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; if (!debug_objects_enabled) @@ -885,24 +852,26 @@ void debug_object_free(void *addr, const struct debug_obj_descr *descr) raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object(addr, db); - if (!obj) - goto out_unlock; + if (!obj) { + raw_spin_unlock_irqrestore(&db->lock, flags); + return; + } switch (obj->state) { case ODEBUG_STATE_ACTIVE: - state = obj->state; - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "free"); - debug_object_fixup(descr->fixup_free, addr, state); - return; + break; default: hlist_del(&obj->node); raw_spin_unlock_irqrestore(&db->lock, flags); free_object(obj); return; } -out_unlock: + + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); + debug_print_object(&o, "free"); + + debug_object_fixup(descr->fixup_free, addr, o.state); } EXPORT_SYMBOL_GPL(debug_object_free); @@ -954,10 +923,10 @@ void debug_object_active_state(void *addr, const struct debug_obj_descr *descr, unsigned int expect, unsigned int next) { + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; - bool print_object = false; if (!debug_objects_enabled) return; @@ -970,28 +939,20 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr, if (obj) { switch (obj->state) { case ODEBUG_STATE_ACTIVE: - if (obj->astate == expect) + if (obj->astate == expect) { obj->astate = next; - else - print_object = true; + raw_spin_unlock_irqrestore(&db->lock, flags); + return; + } break; - default: - print_object = true; break; } + o = *obj; } raw_spin_unlock_irqrestore(&db->lock, flags); - if (!obj) { - struct debug_obj o = { .object = addr, - .state = ODEBUG_STATE_NOTAVAILABLE, - .descr = descr }; - - debug_print_object(&o, "active_state"); - } else if (print_object) { - debug_print_object(obj, "active_state"); - } + debug_print_object(&o, "active_state"); } EXPORT_SYMBOL_GPL(debug_object_active_state); @@ -999,11 +960,9 @@ EXPORT_SYMBOL_GPL(debug_object_active_state); static void __debug_check_no_obj_freed(const void *address, unsigned long size) { unsigned long flags, oaddr, saddr, eaddr, paddr, chunks; - const struct debug_obj_descr *descr; - enum debug_obj_state state; struct debug_bucket *db; struct hlist_node *tmp; - struct debug_obj *obj; + struct debug_obj *obj, o; int cnt, objs_checked = 0; saddr = (unsigned long) address; @@ -1026,12 +985,11 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size) switch (obj->state) { case ODEBUG_STATE_ACTIVE: - descr = obj->descr; - state = obj->state; + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "free"); - debug_object_fixup(descr->fixup_free, - (void *) oaddr, state); + debug_print_object(&o, "free"); + debug_object_fixup(o.descr->fixup_free, + (void *) oaddr, o.state); goto repeat; default: hlist_del(&obj->node); --- base-commit: 201c8a7bd1f3f415920a2df4b8a8817e973f42fe change-id: 20231025-debugobjects_fix-66e5292557c4 Best regards, -- Andrzej Hajda 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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 788E2C0032E for ; Wed, 25 Oct 2023 21:39:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 123158D0016; Wed, 25 Oct 2023 17:39:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0D39F8D0001; Wed, 25 Oct 2023 17:39:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EDD1F8D0016; Wed, 25 Oct 2023 17:39:24 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id DFB408D0001 for ; Wed, 25 Oct 2023 17:39:24 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id B560D160501 for ; Wed, 25 Oct 2023 21:39:24 +0000 (UTC) X-FDA: 81385300248.20.E5275B5 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) by imf20.hostedemail.com (Postfix) with ESMTP id 783F91C0003 for ; Wed, 25 Oct 2023 21:39:22 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=n2Ys9WGS; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf20.hostedemail.com: domain of andrzej.hajda@intel.com designates 134.134.136.24 as permitted sender) smtp.mailfrom=andrzej.hajda@intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1698269962; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding:in-reply-to: references:dkim-signature; bh=zXP1WKDtEBgcaJWBtUcGxE1VhtguZ3Lb29YKMsV+dJI=; b=30EC10v8N4HGkE3cFdVIVedmDXDT09BgbeIitDinQf2NAiU3nMgJx8N9303lN+zvBDgjhC kXh2VFH3KqVBOI31uIpKUojxpqzHDl4HidXIPT9teV1e9SMDM2vH2K9dKsHPq2Ll/360dS OpcrfH3++pqsoXusZwSjoG5iZPkNMdA= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=n2Ys9WGS; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf20.hostedemail.com: domain of andrzej.hajda@intel.com designates 134.134.136.24 as permitted sender) smtp.mailfrom=andrzej.hajda@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1698269962; a=rsa-sha256; cv=none; b=TLsRpHxS3x1dkYF+0yfWbO3+XMTlzjymCIvIyiZx+ed5wSr5WKFc64rLCxrZjDXDBPNihr 1QQPgI+XWMIdutBoWnd6MXaTjnjnY+GZBzq2BEYp23TndLvposyQf+AojfXcxk1a1QOjd3 D968ayZVxQzAnVwOWuCeLGTbjZ3ZqZY= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1698269962; x=1729805962; h=from:date:subject:mime-version:content-transfer-encoding: message-id:to:cc; bh=M9YR0XWgWOmEjasdaF5dvVIdmizBqpKx0yyPpSgRQEE=; b=n2Ys9WGS7rSgo+o+z7bpkFy20bH3LYTN7qk49Z6mzTKN1aXWUk/19PBU gWs/AwYLFVc/4Eiht2XSBmybVi2JOV5u6l2ARHarMCo31ELFkjlnuycrM 2JCIiVbX4N1JarDl5OYHYyGGuXIOAqvyEecgSeFiu6a8DkHtkS1RNCOIi IA7/bJ3vzs8CWcc1UiG6GXzQLXW4qIS2qkS+5BzzXZl8Jgv01VJU8OxQs LqUkgdupHD58dUhZEjmC11y9t7nYSBvudscR6f1sSHPsfhcyOnK8daQmn hed+v50fvMPBTt7LGdeV7xLo5nbv4eBAYdQczdhZD+5HK0TGu06p+BwG7 A==; X-IronPort-AV: E=McAfee;i="6600,9927,10874"; a="390263663" X-IronPort-AV: E=Sophos;i="6.03,250,1694761200"; d="scan'208";a="390263663" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Oct 2023 14:39:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10874"; a="849677457" X-IronPort-AV: E=Sophos;i="6.03,250,1694761200"; d="scan'208";a="849677457" Received: from lab-ah.igk.intel.com ([10.102.138.202]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Oct 2023 14:39:16 -0700 From: Andrzej Hajda Date: Wed, 25 Oct 2023 23:39:07 +0200 Subject: [PATCH v3] debugobjects: stop accessing objects after releasing spinlock MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20231025-debugobjects_fix-v3-1-2bc3bf7084c2@intel.com> X-B4-Tracking: v=1; b=H4sIAPqKOWUC/z3MTQ7CIBCG4as0rKUWKK115T2MMQNMC40FA/UnN r271IXLdzLfs5CE0WEix2IhEZ8uueBziF1BtAU/IHUmN+EVF6zikhpUjyGoEfWcrr1706ZByTs uZatrkmf3iPn8I8+X3H0ME51tRPhDVX5nggnZlbyrDwfeUkbBm/jBsbQwGjg5P+Ot1GHaSAUJq Yrgtd0IVe9foBRZ1y/zpBUyvwAAAA== To: linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org, linux-mm@kvack.org Cc: Thomas Gleixner , Nirmoy Das , Andi Shyti , Janusz Krzysztofik , Andrzej Hajda X-Mailer: b4 0.12.3 X-Developer-Signature: v=1; a=openpgp-sha256; l=11404; i=andrzej.hajda@intel.com; h=from:subject:message-id; bh=M9YR0XWgWOmEjasdaF5dvVIdmizBqpKx0yyPpSgRQEE=; b=owEB7QES/pANAwAKASNispPeEP3XAcsmYgBlOYsDrBMPiLwxsaT9p7frP5Jp6GZUcLUmVQuloKaH 0KwwZ0OJAbMEAAEKAB0WIQT8qEQxNN2/XeF/A00jYrKT3hD91wUCZTmLAwAKCRAjYrKT3hD910unC/ 9guaFzM2bfO+yD/g91t70H8/PquAzFoHsNO9DiLcYCUy0lVikzyP0xydf1mfrpC1M4zua8pdhsmkUJ DJ4o1CJ2ofv4Z8kBXJ2P2J/BCmzIfcl6dDYWFJ3D5TOwdG09AC1ftUET1OtRXh1xPV8YjH9+EMGdcK L3s+dap7EOqC5DCXYAp8mk6VYiZOCf9dJzmq9TpW619cEnqhfKT4XA2fXXMwyYSmLtxok3kAqvS+8B F3AqbqvoljTNxSnGZ/IOCk7jaBlA8gjbbI3IQLSSpmxHFlhuWMIRn6aCePS2LhSlKt4pe+uyM+uQZC MZKBMP/FnZp6xFB1GaCpid0yfnrkYdFw+N0iHDJ4e6W/dMHFZ94VsZDcFW0bsH9c3x4/I2gEUhgoWB EQYr09yu9uASGfdmEQxZWs5wXdRZnu62jQQ+kQbr97HgfSHv28i8GJAU+Mn1y1KdZEE5pVegvlwLqI kIFmWqUEu5IhaIRv7gpQFpOnqgX+5V7Hvwnl75KH25b1Y= X-Developer-Key: i=andrzej.hajda@intel.com; a=openpgp; fpr=FCA8443134DDBF5DE17F034D2362B293DE10FDD7 X-Rspamd-Queue-Id: 783F91C0003 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: uab8g4wdsqtouf7oqmuk7tffxdkwfaeg X-HE-Tag: 1698269962-373926 X-HE-Meta: U2FsdGVkX19Fqcoan2gDs2JGAMnngTkZH2FVR+OfqJrOJYQ8cW09PeU5pQSzKcAMJUhkqnzD56mJpACAnS5bqPN2xFluhbtWPRnV6Q9nQrAQS4I7t6nhJTkwg9Ar9EO+h4ROXsSMByC0QB/PKGOkGdyjCer8a0E1fpIdrhEb5N+YXWTUPgZ2+TrF2jVxSZz7z1RnAurX/60rUgYFL6U/6ulzJOyUWX5/UjMv4XgX9wlXQqsxS1D79Cv8X1Tg37iS+GffHL7SYrvkkDDhOBSj18siAzgvYO9z9ArobxJZz72FJ8uFHEIMEIaAQypWUYzcEuZFvPgbF6jQCECSjzxWsx5yZvUMIISDCgVdpuet2WnAs+vHhmHnR/WSm59p0hfFp52VMYCjBrIFmqLt0O/JNIJTi0cob/B6ibefzI4x4x8BwXP8ddP0M8XlmPyxmuedkVuYsj7yjXotrC38ZPI+xDOaJZfq0vQrqETdWOMowBbTm6PG9qxTDOylhMRORKeEZr+WO4M2poCdDb5HRp9A1XHzOLoJchO+G/4cwEMen1AzSjFzHy1b+D9aqfx1VBajzWQy3IrKBq24tpJnkEyyi2op9SXNME3lKzsu/cdLCtDv62Z3NNoeNrlqOP6ZHcRFrtSWLRXUa6VcmOCODscEKeTn5CwBQOGqT3PFAGB2IeroNPwJ+7Q0+1wfI4av0AcKBTYIgzTe1RdFFa94uiWlY2Z0zQRkts26C20sgBQa8t+6AL6MwyCUyq3iRkmPToMER1cdLzc4HvX2dRoogeq6Szmg+ha9P5l22O8Rx6uJd1K+zIOcV2UxGQIQULG7Hb3OK8X2pkmmBaEdEmMPYcfuExdWaPDzZXCAKCpvroJR1UrVj16s/Sqg8B5LCeefM+XuUDOzE7mriScU5rWZaJe0x+xsRlOsQ505RftS3ZQqMxF8QKL545FHy/OpfcjtB/csFAi/FbSXkOy9UtfZTAj AcacJ3FT 433klScko5hJP65/mMHXY4JKRuN0tUyMkLV5qgb4hgdc0Dg0RlSXHNmFP4tvm6300Dmb/wki5pJwU/md0PVjF1OOl8jmQfQf2EjR0Xplk4waSYgCcqgJd7l9+DKE/Bkua/mTjUyWPy/L0hkMQ2/52ckY2rB5qduNGA3Tb6jbc6ywZzxo= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: After spinlock release object can be modified/freed by concurrent thread. Using it in such case is error prone, even for printing object state. To avoid such situation local copy of the object is created if necessary. Sample buggy scenario: 1. Thread tries to deactivate destroyed object, debugobjects detects it, spin lock is released, thread is preempted. 2. Other thread frees debugobject, then allocates new one on the same memory location, ie 'obj' variable from 1st thread point to it - it is possible because there is no locking. 3. Then preemption occurs, and 1st thread reports error for wrong object. Signed-off-by: Andrzej Hajda --- v2: add missing switch breaks v3: abandon single-point-of-unlock approach --- lib/debugobjects.c | 196 +++++++++++++++++++++-------------------------------- 1 file changed, 77 insertions(+), 119 deletions(-) diff --git a/lib/debugobjects.c b/lib/debugobjects.c index a517256a270b71..c074dbbec084a6 100644 --- a/lib/debugobjects.c +++ b/lib/debugobjects.c @@ -620,9 +620,8 @@ static void debug_objects_fill_pool(void) static void __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack) { - enum debug_obj_state state; struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; debug_objects_fill_pool(); @@ -643,24 +642,18 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: obj->state = ODEBUG_STATE_INIT; - break; - - case ODEBUG_STATE_ACTIVE: - state = obj->state; raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "init"); - debug_object_fixup(descr->fixup_init, addr, state); - return; - - case ODEBUG_STATE_DESTROYED: - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "init"); return; default: break; } + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); + debug_print_object(&o, "init"); + + if (o.state == ODEBUG_STATE_ACTIVE) + debug_object_fixup(descr->fixup_init, addr, o.state); } /** @@ -701,11 +694,9 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_stack); int debug_object_activate(void *addr, const struct debug_obj_descr *descr) { struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; - enum debug_obj_state state; struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; - int ret; if (!debug_objects_enabled) return 0; @@ -717,49 +708,38 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr) raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object_or_alloc(addr, db, descr, false, true); - if (likely(!IS_ERR_OR_NULL(obj))) { - bool print_object = false; - + if (unlikely(!obj)) { + raw_spin_unlock_irqrestore(&db->lock, flags); + debug_objects_oom(); + return 0; + } else if (likely(!IS_ERR(obj))) { switch (obj->state) { - case ODEBUG_STATE_INIT: - case ODEBUG_STATE_INACTIVE: - obj->state = ODEBUG_STATE_ACTIVE; - ret = 0; - break; - case ODEBUG_STATE_ACTIVE: - state = obj->state; - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "activate"); - ret = debug_object_fixup(descr->fixup_activate, addr, state); - return ret ? 0 : -EINVAL; - case ODEBUG_STATE_DESTROYED: - print_object = true; - ret = -EINVAL; break; + case ODEBUG_STATE_INIT: + case ODEBUG_STATE_INACTIVE: + obj->state = ODEBUG_STATE_ACTIVE; + fallthrough; default: - ret = 0; - break; + raw_spin_unlock_irqrestore(&db->lock, flags); + return 0; } - raw_spin_unlock_irqrestore(&db->lock, flags); - if (print_object) - debug_print_object(obj, "activate"); - return ret; } + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); + debug_print_object(&o, "activate"); - /* If NULL the allocation has hit OOM */ - if (!obj) { - debug_objects_oom(); - return 0; + switch (o.state) { + case ODEBUG_STATE_ACTIVE: + case ODEBUG_STATE_NOTAVAILABLE: + if (debug_object_fixup(descr->fixup_activate, addr, o.state)) + return 0; + fallthrough; + default: + return -EINVAL; } - - /* Object is neither static nor tracked. It's not initialized */ - debug_print_object(&o, "activate"); - ret = debug_object_fixup(descr->fixup_activate, addr, ODEBUG_STATE_NOTAVAILABLE); - return ret ? 0 : -EINVAL; } EXPORT_SYMBOL_GPL(debug_object_activate); @@ -770,10 +750,10 @@ EXPORT_SYMBOL_GPL(debug_object_activate); */ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) { + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; - bool print_object = false; if (!debug_objects_enabled) return; @@ -785,33 +765,24 @@ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) obj = lookup_object(addr, db); if (obj) { switch (obj->state) { + case ODEBUG_STATE_DESTROYED: + break; case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: case ODEBUG_STATE_ACTIVE: - if (!obj->astate) - obj->state = ODEBUG_STATE_INACTIVE; - else - print_object = true; - break; - - case ODEBUG_STATE_DESTROYED: - print_object = true; - break; + if (obj->astate) + break; + obj->state = ODEBUG_STATE_INACTIVE; + fallthrough; default: - break; + raw_spin_unlock_irqrestore(&db->lock, flags); + return; } + o = *obj; } raw_spin_unlock_irqrestore(&db->lock, flags); - if (!obj) { - struct debug_obj o = { .object = addr, - .state = ODEBUG_STATE_NOTAVAILABLE, - .descr = descr }; - - debug_print_object(&o, "deactivate"); - } else if (print_object) { - debug_print_object(obj, "deactivate"); - } + debug_print_object(&o, "deactivate"); } EXPORT_SYMBOL_GPL(debug_object_deactivate); @@ -822,11 +793,9 @@ EXPORT_SYMBOL_GPL(debug_object_deactivate); */ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) { - enum debug_obj_state state; struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; - bool print_object = false; if (!debug_objects_enabled) return; @@ -836,32 +805,31 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object(addr, db); - if (!obj) - goto out_unlock; + if (!obj) { + raw_spin_unlock_irqrestore(&db->lock, flags); + return; + } switch (obj->state) { + case ODEBUG_STATE_ACTIVE: + case ODEBUG_STATE_DESTROYED: + break; case ODEBUG_STATE_NONE: case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: obj->state = ODEBUG_STATE_DESTROYED; - break; - case ODEBUG_STATE_ACTIVE: - state = obj->state; + fallthrough; + default: raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "destroy"); - debug_object_fixup(descr->fixup_destroy, addr, state); return; - - case ODEBUG_STATE_DESTROYED: - print_object = true; - break; - default: - break; } -out_unlock: + + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); - if (print_object) - debug_print_object(obj, "destroy"); + debug_print_object(&o, "destroy"); + + if (o.state == ODEBUG_STATE_ACTIVE) + debug_object_fixup(descr->fixup_destroy, addr, o.state); } EXPORT_SYMBOL_GPL(debug_object_destroy); @@ -872,9 +840,8 @@ EXPORT_SYMBOL_GPL(debug_object_destroy); */ void debug_object_free(void *addr, const struct debug_obj_descr *descr) { - enum debug_obj_state state; struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; if (!debug_objects_enabled) @@ -885,24 +852,26 @@ void debug_object_free(void *addr, const struct debug_obj_descr *descr) raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object(addr, db); - if (!obj) - goto out_unlock; + if (!obj) { + raw_spin_unlock_irqrestore(&db->lock, flags); + return; + } switch (obj->state) { case ODEBUG_STATE_ACTIVE: - state = obj->state; - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "free"); - debug_object_fixup(descr->fixup_free, addr, state); - return; + break; default: hlist_del(&obj->node); raw_spin_unlock_irqrestore(&db->lock, flags); free_object(obj); return; } -out_unlock: + + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); + debug_print_object(&o, "free"); + + debug_object_fixup(descr->fixup_free, addr, o.state); } EXPORT_SYMBOL_GPL(debug_object_free); @@ -954,10 +923,10 @@ void debug_object_active_state(void *addr, const struct debug_obj_descr *descr, unsigned int expect, unsigned int next) { + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; - bool print_object = false; if (!debug_objects_enabled) return; @@ -970,28 +939,20 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr, if (obj) { switch (obj->state) { case ODEBUG_STATE_ACTIVE: - if (obj->astate == expect) + if (obj->astate == expect) { obj->astate = next; - else - print_object = true; + raw_spin_unlock_irqrestore(&db->lock, flags); + return; + } break; - default: - print_object = true; break; } + o = *obj; } raw_spin_unlock_irqrestore(&db->lock, flags); - if (!obj) { - struct debug_obj o = { .object = addr, - .state = ODEBUG_STATE_NOTAVAILABLE, - .descr = descr }; - - debug_print_object(&o, "active_state"); - } else if (print_object) { - debug_print_object(obj, "active_state"); - } + debug_print_object(&o, "active_state"); } EXPORT_SYMBOL_GPL(debug_object_active_state); @@ -999,11 +960,9 @@ EXPORT_SYMBOL_GPL(debug_object_active_state); static void __debug_check_no_obj_freed(const void *address, unsigned long size) { unsigned long flags, oaddr, saddr, eaddr, paddr, chunks; - const struct debug_obj_descr *descr; - enum debug_obj_state state; struct debug_bucket *db; struct hlist_node *tmp; - struct debug_obj *obj; + struct debug_obj *obj, o; int cnt, objs_checked = 0; saddr = (unsigned long) address; @@ -1026,12 +985,11 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size) switch (obj->state) { case ODEBUG_STATE_ACTIVE: - descr = obj->descr; - state = obj->state; + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "free"); - debug_object_fixup(descr->fixup_free, - (void *) oaddr, state); + debug_print_object(&o, "free"); + debug_object_fixup(o.descr->fixup_free, + (void *) oaddr, o.state); goto repeat; default: hlist_del(&obj->node); --- base-commit: 201c8a7bd1f3f415920a2df4b8a8817e973f42fe change-id: 20231025-debugobjects_fix-66e5292557c4 Best regards, -- Andrzej Hajda