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 17ACEC47DD9 for ; Wed, 27 Mar 2024 17:43:16 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 994EB6B0092; Wed, 27 Mar 2024 13:43:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 943D46B009C; Wed, 27 Mar 2024 13:43:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 80EC96B009F; Wed, 27 Mar 2024 13:43:15 -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 62FD36B0092 for ; Wed, 27 Mar 2024 13:43:15 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 26421A125E for ; Wed, 27 Mar 2024 17:43:15 +0000 (UTC) X-FDA: 81943540350.11.8415A23 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf08.hostedemail.com (Postfix) with ESMTP id 77754160004 for ; Wed, 27 Mar 2024 17:43:13 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=none; spf=pass (imf08.hostedemail.com: domain of cmarinas@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cmarinas@kernel.org; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none) ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1711561393; 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: in-reply-to:in-reply-to:references:references; bh=6xfaeiS71aJjwRgAAhEpMQ1quoGU9ig07MPMG7YyyoE=; b=2YkWK2iVHU3YUotVv3aTiH05hABC16GHa3C0T6B1QETxIbS6DqJEUUDd6gdJVIkkH2N1l7 44+VsXf3BmbwgX0UChHZaFiXhDMPNo8poEZmhjdB2FcRXevRDEWm57bNJvL9IZ+yQNqy1T Fsi2Yo3oktxpX0DlzZs2Ew0vaDDQwhg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711561393; a=rsa-sha256; cv=none; b=O0cH4v8+u/V3HmDoUENFp1pW4t/ZSRgIqGb7HTajJj2onxq6ts2r4bOpYKAqJdwNFfq+QA ekc+DDT66WgYu4RrxVfgGRWsnTaJEwxvX5ZAKP1uFboSmYlcJdHohFDjPjSZgppzs++YCC GvTZL+6DNGN4L9Nep4kddwW4dO5YLeY= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=none; spf=pass (imf08.hostedemail.com: domain of cmarinas@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cmarinas@kernel.org; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 7754261510; Wed, 27 Mar 2024 17:43:12 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E5B26C433C7; Wed, 27 Mar 2024 17:43:10 +0000 (UTC) Date: Wed, 27 Mar 2024 17:43:08 +0000 From: Catalin Marinas To: Andrew Morton Cc: Waiman Long , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Audra Mitchell Subject: Re: [PATCH v2] mm/kmemleak: Don't hold kmemleak_lock when calling printk() Message-ID: References: <20240307184707.961255-1-longman@redhat.com> <20240307114630.32702099ac24c182b91da517@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240307114630.32702099ac24c182b91da517@linux-foundation.org> X-Stat-Signature: ts95e9cwdns7dqm8km7xi9jfs7binuso X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 77754160004 X-Rspam-User: X-HE-Tag: 1711561393-475587 X-HE-Meta: U2FsdGVkX18YYm+9rwaBXH3oS41sMzXnBHdkgnYUM6HUWugPAJnLR2VHyjEguWG8erHdotKkP2pmK2laGM5Qafl0bXx6hJkOw3AV3NgsOhcHXjoWzBv1q/b4fD6W+hdZPU3Qvdr+4fsXGPAmZeVyhNKzc605TeVQMhv5rwpGjki5v+k66pbZmntS4lq2dhM4RDK5X8MoHRSafLQuvcDXaywzM9vkxkkHmh7DAhcuA2aL5a9STfy7XYi+HBjdhWWyzDrBDHmy97tstbLfWjl6Syqx0gtbFFqkNfqlrdDOHKJOVW1FLT0kv21HDpS8cknk5iEDXbXTapLMUdiVg6wHXMM7oNNWo8xattsihsuhdzFImLm5YPOtajpqBXGj5hGvM2Cx/hSgqjVXec9hWbJHsO4py4aNDVL7nzIRryNBVUjaW+yqqyis95wG8QkFpRg3Wbfn3AjZxiGWQ9hSAlJLpLw2Rc1KGDTSU15jXAXs7OphM/d5po22ua2nszGgEwFq7Zzm/ri9Y7w4O7q2fcnmKh5N/B4gjcMnLeIl7AKx0KcTcf8yR1omMRfe+0C3AYgbiiSW0jQzhGqCvzRhmD1It8MI1+nFp5GJcMlHKwVfxLzQoPv/TnLvcHfrQRUOxI/+Eu6Pu3D23Fq6m7bbBEo4zIAj4YenPYL96ff6U1GP6uxiqlnh3oW7WqGJpRGZEhIA3RMSy0csIqCs6+0TqMD7i4rlLd1yMKm9pOfFMpQoDKdYydjtz0vGZ1OuLXEe93/qTFp+FcTO7MEY0hSuV3jJAGEAqXXqBuZ1WiREHZKnGwYYBqTx3c9nuwY8cGqnBk2SXisZdkR58tHelJfwB0LqrQJ1DbUbwamKU0FIoxa3xxhGh4dAlPFif4pNJEHR0w+bD7cqUDPaeB+IToKRp+RjobLGK3KApwzdkRBoOya48F1XayJYp26jPAUesX0QfHKOCrvQfnxiBojUwMv1u44 OgSd33PB 04AWr8vCl7BxxIvh2NyeDucyEiTCuhZagvs3EZ3OW+qysTzKwZ0O+0ieOxD1UCu1P4l4F5skBLKn8P/5T8hmOF/exwyEIvs8ObuEUuT665DQTrNoxgSqZBlfxLLyOON+sGaV/f70X2KFAaU6knCMQm45sgr27ftQWs4M3nEQPyZeLZYeshnDUGraGaxosx7aij9F6wDkLqSMFQTj0pPBWGXn3XMGIBNbe9jsWDT/14UIO/6Wj8fcCQvMBXl6lNITyZ8MWYvTNZwVt7bbnrVRg9ppXwljzlzKmng9h57/LZohqjTONJ9cjouP5n3e6Qfajw5DHqlU5QApt2WAns8LIdQWRWh0MqVhIUgmyf1vTikcdPuHLyc3LgSkUHGNZSNJ4dn/e 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 Thu, Mar 07, 2024 at 11:46:30AM -0800, Andrew Morton wrote: > On Thu, 7 Mar 2024 13:47:07 -0500 Waiman Long wrote: > > When some error conditions happen (like OOM), some kmemleak functions > > call printk() to dump out some useful debugging information while holding > > the kmemleak_lock. This may cause deadlock as the printk() function > > may need to allocate additional memory leading to a create_object() > > call acquiring kmemleak_lock again. > > > > An abbreviated lockdep splat is as follows: > > > > ... > > > > Fix this deadlock issue by making sure that printk() is only called > > after releasing the kmemleak_lock. > > > > ... > > > > @@ -427,9 +442,19 @@ static struct kmemleak_object *__lookup_object(unsigned long ptr, int alias, > > else if (untagged_objp == untagged_ptr || alias) > > return object; > > else { > > + if (!get_object(object)) > > + break; > > + /* > > + * Release kmemleak_lock temporarily to avoid deadlock > > + * in printk(). dump_object_info() is called without > > + * holding object->lock (race unlikely). > > + */ > > + raw_spin_unlock(&kmemleak_lock); > > kmemleak_warn("Found object by alias at 0x%08lx\n", > > ptr); > > dump_object_info(object); > > + put_object(object); > > + raw_spin_lock(&kmemleak_lock); > > break; > > Please include a full description of why this is safe. Once we've > dropped that lock, the tree is in an unknown state and we shouldn't > touch it again. This consideration should be added to the relevant > functions' interface documentation and the code should be reviewed to > ensure that we're actually adhering to this. Or something like that. > > To simply drop and reacquire a lock without supporting analysis and > comments does not inspire confidence! I agree it looks fragile. I think it works, the code tends to bail out on those errors and doesn't expect the protected data to have remained intact. But we may change it in the future and forgot about this. I wonder whether we can actually make things slightly easier to reason about, defer the printing until unlock, store the details in some per-cpu variable. Another option would be to have a per-CPU array to store potential recursive kmemleak_*() callbacks during the critical regions. This should be bounded since the interrupts are disabled. On unlock, we'd replay the array and add those pointers. -- Catalin