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 BEFEAC47074 for ; Wed, 15 Nov 2023 12:33:27 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2B51C80017; Wed, 15 Nov 2023 07:33:27 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 264E980010; Wed, 15 Nov 2023 07:33:27 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 105F480017; Wed, 15 Nov 2023 07:33:27 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id F0DB480010 for ; Wed, 15 Nov 2023 07:33:26 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id C83781CB2E2 for ; Wed, 15 Nov 2023 12:33:26 +0000 (UTC) X-FDA: 81460129212.05.29F4320 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.115]) by imf10.hostedemail.com (Postfix) with ESMTP id 2CEBCC001C for ; Wed, 15 Nov 2023 12:33:22 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=HRcuh62U; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf10.hostedemail.com: domain of andrzej.hajda@intel.com designates 192.55.52.115 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=1700051604; 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:in-reply-to:references:references:dkim-signature; bh=jg86TQC5j5MwROkPP+VeUx461eWUpuzb50Eofggt46I=; b=rXxid5WIovW//hIHKf2cvUK+fOXWDwC3SW8d5EJkxF9iR6kyCjPdPSDFpwagANY2G4eaNs RNmqisKSJOLzCFtXFN+FRi+6MBcdo7tpZh4PgvkqXyW1uOx+iXZwI5W0o2/Enb9YVMpldD K+gv3Pi4wM0M5TyZ/CAQ2R+320dxtc8= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=HRcuh62U; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf10.hostedemail.com: domain of andrzej.hajda@intel.com designates 192.55.52.115 as permitted sender) smtp.mailfrom=andrzej.hajda@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1700051604; a=rsa-sha256; cv=none; b=jHJUS0QvgWZg8Tcexcniqvr1Vq+BP+H8k5GYRbKguLdHeGIYEFYTAQjX4hxWWVSq68OZ/H f0lr4e3H036cTHsCuBeHDoRyPe/YIrFjHgZzrWNBoRep9izxt5A1OtoGSrITO4FDm4/7XM NOVQQWkiP2mOuHf5ynPvh0OilVZACNo= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1700051603; x=1731587603; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=t1dW6o2nTHz2o6AFCdNOcaRMy7d6+pwh7NWq+AsApkY=; b=HRcuh62UOZRohq2IQzr+GqOakEITChxMg5Qm6AZdHcM5gMuQDtQDaphp ZKQ8CBm+iDNubCLbmCjwmdJpoDR7mfWMVHCn+n7A5OrkWx4xzJJbZ7JUa 3KQGTr7teeCkwQgRRMsWfyYYE03fA0hrbfQE7PZ4YZ0R8M44g9WTnxGAl cCWwl5BQTmP6AsttneH6U7W9z+GpZ/7xoDeYODT/Sfirz+LBXPmhBAnXI A2AcXaDMCpvju1RFR/Di7RvNsv95QBlJxkSLjjryUhbMW+Dwf1Nvw2qnW qePRaHgJyxHKne4G2ApvqGOBLmp1L/PucZd01jc2X7r+N4P6xRK9iWLRe w==; X-IronPort-AV: E=McAfee;i="6600,9927,10894"; a="390664927" X-IronPort-AV: E=Sophos;i="6.03,304,1694761200"; d="scan'208";a="390664927" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Nov 2023 04:33:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10894"; a="1096429926" X-IronPort-AV: E=Sophos;i="6.03,304,1694761200"; d="scan'208";a="1096429926" Received: from ahajda-mobl.ger.corp.intel.com (HELO [10.213.26.106]) ([10.213.26.106]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Nov 2023 04:33:19 -0800 Message-ID: <2f72313b-2fb4-4f62-a9d7-3fe05f1051c4@intel.com> Date: Wed, 15 Nov 2023 13:33:17 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [Intel-gfx] [PATCH v3] debugobjects: stop accessing objects after releasing spinlock Content-Language: en-US To: linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org, linux-mm@kvack.org, Thomas Gleixner Cc: Nirmoy Das References: <20231025-debugobjects_fix-v3-1-2bc3bf7084c2@intel.com> From: Andrzej Hajda Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173, 80-298 Gdansk - KRS 101882 - NIP 957-07-52-316 In-Reply-To: <20231025-debugobjects_fix-v3-1-2bc3bf7084c2@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspam-User: X-Stat-Signature: zxn9urgb9op6s1bba8znsuzq11fqetkh X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 2CEBCC001C X-HE-Tag: 1700051602-14519 X-HE-Meta: U2FsdGVkX1+ciwQiZihYTDPSgxZJL1zmESZT19RWqqlsg8QcE1zOvSkcD6DwYH+P5iTm8FWGC/GFL9fnFrlf8Y3GSMSTLQ9uetIttI4yVq6DRoow1556GQh4JDwKTPVv+BidQh4RDLgsyWxAB8xm+zUmTt+DLTsZGeE3XURJDVVZ7yV4FDh6ZU2wRDM/nN/vVR4DOKrPT2Th+qs6Ef0e3mMTWegRl5CF5YJ6rnPwTZFqXWcDSGshqjYOmESb8750t7P+C1CsBhhv8DlYSgUMMM4/4eWIHHghWimPDvbwQFGlY/6pgcT3bkVRd/WVQvPPa54I8YOEqOxRyw7d5YACfw6PARiDstPsVYusg0XEjxduhbnu8m0GAew5mo8+e6HDQSJpBH9gDhpLHLO1aGxoNUzHwu3b+5lySxRpACP5lxU21pCKRW6GqnanhaNRFK28SDlyK9KmVrEwHVGXSEKaYxCvnGcVL5uI8ZI4yizWvwT7+vu47twAVwKvRi7R7cNNPqgXKG3PzdJjmz6F6qXrN+LmlGske4v1ibCU4TPyYtgTJdcdLZUxwnGfjxybhWbgS4QQynQgroxHB9/ZItfIV5KHiuUYMdoiNUL2q8ypjzvY+5TX+Z4CVIeiObUvH3iVC7J9w2orhwOW59U3spnd07TFQms5Vkmz0JCfkQmT7rWRxfi9alVOlC28y36OaEElfWyJVRJafwbQdhp4S6SpY4eyLSMS1ouZz7XizFarRx1BD/tuJsB2wgQmeYHUH5PZGDNSAvPGAHeh2OkPUyWhBmtHsqxxIxq2lT10Bblp1g40wE1ztSG3YZhaHnPtpX08A36DwwjxUb1/9iMldcNigFcJJ3uogTpXVgZtEV0nEzyrFGa5Vrob9f0xo0CcIZyfhDQ7yavU28myL1dUYLUO5AtwQHy4XtnLA0fAEkIuQeHA5S8fQjGw9xqwPfquKoKGF6rf8HJ17b1SOlOZ+/K d+oP2VOt S95FyixSurRE3YpPFrAu5NZ1GJbiVgEW1i/WLvK5cQDd2+whYySaU99YtKOBMbnSGFebCLKGu3sYxF0sUx+HM1XnBiA7QTAw6Agf3UkdrdMCIafCOKEjAqR4X+yM2uEAhxEDIGRuMnjweO+56wi1ehJBNgfLlF9FVLVou5FtmwybSBW/qqj/RUzEVNvjH1d7jP6i/IDAWwCUJnXKa1nqRgZzlNJK7PQ4AB6p0U1pVwgWONUI= 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: On 25.10.2023 23:39, Andrzej Hajda wrote: > 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 Gently ping. Regards Andrzej > --- > 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,