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 634F3C433EF for ; Mon, 6 Jun 2022 15:01:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id ACCAA6B0071; Mon, 6 Jun 2022 11:01:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A7C376B0073; Mon, 6 Jun 2022 11:01:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 96C9B8D0001; Mon, 6 Jun 2022 11:01:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 882A96B0071 for ; Mon, 6 Jun 2022 11:01:31 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 644336083A for ; Mon, 6 Jun 2022 15:01:31 +0000 (UTC) X-FDA: 79548124782.04.F5D9960 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by imf13.hostedemail.com (Postfix) with ESMTP id 37E2920027 for ; Mon, 6 Jun 2022 15:00:53 +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 ams.source.kernel.org (Postfix) with ESMTPS id D04C0B819F9; Mon, 6 Jun 2022 15:01:28 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 65CBDC34115; Mon, 6 Jun 2022 15:01:26 +0000 (UTC) Date: Mon, 6 Jun 2022 16:01:22 +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 v2 3/4] mm: kmemleak: handle address stored in object based on its type Message-ID: References: <20220603035415.1243913-1-patrick.wang.shcn@gmail.com> <20220603035415.1243913-4-patrick.wang.shcn@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220603035415.1243913-4-patrick.wang.shcn@gmail.com> X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 37E2920027 Authentication-Results: imf13.hostedemail.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none); spf=pass (imf13.hostedemail.com: domain of cmarinas@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=cmarinas@kernel.org X-Stat-Signature: hy14tp8637q7mrqs5nmskgep6d8p89ii X-Rspam-User: X-HE-Tag: 1654527653-68562 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 Fri, Jun 03, 2022 at 11:54:14AM +0800, Patrick Wang wrote: > Treat the address stored in object in different way according > to its type: > > - Only use kasan_reset_tag for virtual address > - Only update min_addr and max_addr for virtual address > - Convert physical address to virtual address in scan_object > > Suggested-by: Catalin Marinas > Signed-off-by: Patrick Wang > --- > mm/kmemleak.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/mm/kmemleak.c b/mm/kmemleak.c > index 218144392446..246a70b7218f 100644 > --- a/mm/kmemleak.c > +++ b/mm/kmemleak.c > @@ -297,7 +297,9 @@ static void hex_dump_object(struct seq_file *seq, > warn_or_seq_printf(seq, " hex dump (first %zu bytes):\n", len); > kasan_disable_current(); > warn_or_seq_hex_dump(seq, DUMP_PREFIX_NONE, HEX_ROW_SIZE, > - HEX_GROUP_SIZE, kasan_reset_tag((void *)ptr), len, HEX_ASCII); > + HEX_GROUP_SIZE, object->flags & OBJECT_PHYS ? ptr : > + kasan_reset_tag((void *)ptr), > + len, HEX_ASCII); > kasan_enable_current(); > } This will go wrong since ptr is the actual physical address, it cannot be dereferenced. This should only be used on virtual pointers and this is the case already as we never print unreferenced objects from the phys tree. What we could do though is something like an early exit from this function (together with a comment that it doesn't support dumping such objects): if (WARN_ON_ONCE(object->flags & OBJECT_PHYS)) return; > > @@ -389,14 +391,15 @@ static struct kmemleak_object *lookup_object(unsigned long ptr, int alias, > { > struct rb_node *rb = is_phys ? object_phys_tree_root.rb_node : > object_tree_root.rb_node; > - unsigned long untagged_ptr = (unsigned long)kasan_reset_tag((void *)ptr); > + unsigned long untagged_ptr = is_phys ? ptr : (unsigned long)kasan_reset_tag((void *)ptr); > > while (rb) { > struct kmemleak_object *object; > unsigned long untagged_objp; > > object = rb_entry(rb, struct kmemleak_object, rb_node); > - untagged_objp = (unsigned long)kasan_reset_tag((void *)object->pointer); > + untagged_objp = is_phys ? object->pointer : > + (unsigned long)kasan_reset_tag((void *)object->pointer); > > if (untagged_ptr < untagged_objp) > rb = object->rb_node.rb_left; You could leave this unchanged. A phys pointer is already untagged, so it wouldn't make any difference. > @@ -643,16 +646,19 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size, > > raw_spin_lock_irqsave(&kmemleak_lock, flags); > > - untagged_ptr = (unsigned long)kasan_reset_tag((void *)ptr); > - min_addr = min(min_addr, untagged_ptr); > - max_addr = max(max_addr, untagged_ptr + size); > + untagged_ptr = is_phys ? ptr : (unsigned long)kasan_reset_tag((void *)ptr); Same here. > + if (!is_phys) { > + min_addr = min(min_addr, untagged_ptr); > + max_addr = max(max_addr, untagged_ptr + size); > + } > link = is_phys ? &object_phys_tree_root.rb_node : > &object_tree_root.rb_node; > rb_parent = NULL; > while (*link) { > rb_parent = *link; > parent = rb_entry(rb_parent, struct kmemleak_object, rb_node); > - untagged_objp = (unsigned long)kasan_reset_tag((void *)parent->pointer); > + untagged_objp = is_phys ? parent->pointer : > + (unsigned long)kasan_reset_tag((void *)parent->pointer); And here. > if (untagged_ptr + size <= untagged_objp) > link = &parent->rb_node.rb_left; > else if (untagged_objp + parent->size <= untagged_ptr) > @@ -1202,7 +1208,9 @@ static bool update_checksum(struct kmemleak_object *object) > > kasan_disable_current(); > kcsan_disable_current(); > - object->checksum = crc32(0, kasan_reset_tag((void *)object->pointer), object->size); > + object->checksum = crc32(0, object->flags & OBJECT_PHYS ? (void *)object->pointer : > + kasan_reset_tag((void *)object->pointer), > + object->size); Luckily that's never called on a phys object, otherwise *object->pointer would segfault. As for hex_dump, just return early with a warning if that's the case. > kasan_enable_current(); > kcsan_enable_current(); > > @@ -1353,6 +1361,7 @@ static void scan_object(struct kmemleak_object *object) > { > struct kmemleak_scan_area *area; > unsigned long flags; > + void *obj_ptr; > > /* > * Once the object->lock is acquired, the corresponding memory block > @@ -1364,10 +1373,15 @@ static void scan_object(struct kmemleak_object *object) > if (!(object->flags & OBJECT_ALLOCATED)) > /* already freed object */ > goto out; > + > + obj_ptr = object->flags & OBJECT_PHYS ? > + __va((void *)object->pointer) : > + (void *)object->pointer; > + > if (hlist_empty(&object->area_list) || > object->flags & OBJECT_FULL_SCAN) { > - void *start = (void *)object->pointer; > - void *end = (void *)(object->pointer + object->size); > + void *start = obj_ptr; > + void *end = obj_ptr + object->size; > void *next; > > do { This looks fine, assuming that the following patch adds the checks for objects above max_low_pfn (I haven't got there yet). -- Catalin