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 C9B85C433EF for ; Wed, 1 Jun 2022 16:13:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 437388D0025; Wed, 1 Jun 2022 12:13:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3BD508D001C; Wed, 1 Jun 2022 12:13:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 25E4D8D0025; Wed, 1 Jun 2022 12:13:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 1802C8D001C for ; Wed, 1 Jun 2022 12:13:50 -0400 (EDT) Received: from smtpin31.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id D3B1120D74 for ; Wed, 1 Jun 2022 16:13:49 +0000 (UTC) X-FDA: 79530162978.31.F3496CE Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf21.hostedemail.com (Postfix) with ESMTP id 73D9A1C0054 for ; Wed, 1 Jun 2022 16:13:34 +0000 (UTC) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id D8FD76158B; Wed, 1 Jun 2022 16:13:47 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1B9F0C385A5; Wed, 1 Jun 2022 16:13:45 +0000 (UTC) Date: Wed, 1 Jun 2022 17:13:42 +0100 From: Catalin Marinas To: Patrick Wang Cc: akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, yee.lee@mediatek.com Subject: Re: [PATCH] mm: kmemleak: check boundary of objects allocated with physical address when scan Message-ID: References: <20220531150823.1004101-1-patrick.wang.shcn@gmail.com> <99faf6b0-30bf-f87c-2620-1eafb4eac1ac@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <99faf6b0-30bf-f87c-2620-1eafb4eac1ac@gmail.com> X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 73D9A1C0054 X-Stat-Signature: aca77iar1iitqybxsnbnrneki3t8p8fc X-Rspam-User: Authentication-Results: imf21.hostedemail.com; dkim=none; spf=pass (imf21.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) X-HE-Tag: 1654100014-329443 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: On Wed, Jun 01, 2022 at 06:24:34PM +0800, Patrick Wang wrote: > On 2022/6/1 00:29, Catalin Marinas wrote: > > On Tue, May 31, 2022 at 11:08:23PM +0800, Patrick Wang wrote: > > > + if (kmemleak_enabled && (unsigned long)__va(phys) >= PAGE_OFFSET && > > > + !IS_ERR(__va(phys))) > > > + /* create object with OBJECT_PHYS flag */ > > > + create_object((unsigned long)__va(phys), size, min_count, > > > + gfp, true); > > > > Do we still need to check for __va(phys) >= PAGE_OFFSET? Also I don't > > think IS_ERR(__va(phys)) makes sense, we can't store an error in a > > physical address. The kmemleak_alloc_phys() function is only called on > > successful allocation, so shouldn't bother with error codes. > > In this commit: > 972fa3a7c17c(mm: kmemleak: alloc gray object for reserved > region with direct map) > > The kmemleak_alloc_phys() function is called directly by passing > physical address from devicetree. So I'm concerned that could > __va() => __pa() convert always get the phys back? I thought > check for __va(phys) might help, but it probably dosen't work > and using IS_ERR is indeed inappropriate. > > We might have to store phys in object and convert it via __va() > for normal use like: > > #define object_pointer(obj) \ > (obj->flags & OBJECT_PHYS ? (unsigned long)__va((void *)obj->pointer) \ > : obj->pointer) In the commit you mentioned, the kmemleak callback is skipped if the memory is marked no-map. But you have a point with the va->pa conversion. On 32-bit architectures, the __va() is no longer valid if the pfn is above max_low_pfn. So whatever we add to the rbtree may be entirely bogus, and we can't guarantee that the va->pa conversion back is correct. Storing the phys address in object->pointer only solves the conversion but it doesn't solve the rbtree problem (VA and PA values may overlap, we can't just store the physical address either). And we use the rbtree for searching objects on freeing as well. Given that all the kmemleak_alloc_phys() calls always pass min_count=0 (we should probably get rid of the extra arguments), we don't expect them to leak, so there's no point in adding them to the rbtree. We can instead add a new object_phys_tree_root to store these objects by the physical address for when we need to search (kmemleak_free_part_phys()). This would probably look simpler than recording the callbacks and replaying them. Wherever we use object_tree_root we should check for OBJECT_PHYS and use object_phys_tree_root instead. There aren't many places. -- Catalin