Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp4732857pxu; Thu, 10 Dec 2020 04:16:27 -0800 (PST) X-Google-Smtp-Source: ABdhPJw6KmMl2AafaSn/2Rgeaazs9UFRBUXvElHaK7HJbH5R/d+BGtONo0XbhrU/5BMmXqhW8p+k X-Received: by 2002:a17:906:1151:: with SMTP id i17mr6315552eja.250.1607602587042; Thu, 10 Dec 2020 04:16:27 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1607602587; cv=none; d=google.com; s=arc-20160816; b=dtwS+dHQ3bEeswq6MGWEq6lNSXpco7TPl0uFEHkKGFTd04TU3+3TH3IV3VD8Xp+NcV nvILOxjsyX0VPII6WaAnEA2wBrJYCQ84W1g25XkZoRbdrkGM3U0j8Yyp5IFVZVeK7lkQ OyLdoKtP4k8vAQVawU2U6kWQ1Q70xTc8Nz30uCwdvFBn4yZxut/aXKeQBdDg7M2dLla9 7iIRjifPKqLrlmDo9WVCOA7Gbt6PNSorH3H899MHqyELOgrJW++DjSzuz1zDQzVBsVfc 5VQhariSPTypia1H0wK74UemUNegJT9l7jpdhoe7DvOUNwKEVvEivq04bKUoDAmmA5+3 2VOw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:subject:from :references:cc:to; bh=0oQ7+Yn6xyI/GCvhyVQVVkbr4r4tvXaHNxud97uI+Rw=; b=le8am/1hti7B2V2V3KR3wrA2oAJe9G39uiOxhCiIg/HgIMpWIW6DysZW2JBLTLadHT M63zelynl/6EQlzZ2wLjWXoE035kYyXoZ1HgUl+NxzZBLBnfcYbBI7FsMMXFvnn0FfxM 0vdiYGZqi/4sgkdKyFt792kmIQ80U4JjUbphfONRu5wW7GycApZcSEc6lcI9YqI33BA3 IpnNEepObNU45oT1uXTT/M4ub3nFV3bQJ9aWHSACcaNvWZJoHUIy5L1CflV58p1kzeWM +yYMELLEvzzVrKloN4ECiww+ylKPL1gKKJ9v3iBdBRCIpIKvKfxkbTqjVUJ4cFpjKQNB D9hA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id ce16si2616354edb.238.2020.12.10.04.16.04; Thu, 10 Dec 2020 04:16:27 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732601AbgLJLnD (ORCPT + 99 others); Thu, 10 Dec 2020 06:43:03 -0500 Received: from mx2.suse.de ([195.135.220.15]:34284 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727968AbgLJLnD (ORCPT ); Thu, 10 Dec 2020 06:43:03 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 177E4AE4A; Thu, 10 Dec 2020 11:42:19 +0000 (UTC) To: paulmck@kernel.org Cc: rcu@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@fb.com, mingo@kernel.org, jiangshanlai@gmail.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, josh@joshtriplett.org, tglx@linutronix.de, peterz@infradead.org, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, fweisbec@gmail.com, oleg@redhat.com, joel@joelfernandes.org, iamjoonsoo.kim@lge.com, andrii@kernel.org, Christoph Lameter , Pekka Enberg , David Rientjes , linux-mm@kvack.org References: <20201209011124.GA31164@paulmck-ThinkPad-P72> <20201209011303.32737-1-paulmck@kernel.org> <8ea31887-8cc3-24cc-82e8-779290c61c2c@suse.cz> <20201209230442.GH2657@paulmck-ThinkPad-P72> From: Vlastimil Babka Subject: Re: [PATCH v2 sl-b 1/5] mm: Add mem_dump_obj() to print source of memory block Message-ID: <211cc2a9-5d94-cb0f-91bf-3415510b7dae@suse.cz> Date: Thu, 10 Dec 2020 11:48:26 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.1 MIME-Version: 1.0 In-Reply-To: <20201209230442.GH2657@paulmck-ThinkPad-P72> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/10/20 12:04 AM, Paul E. McKenney wrote: >> > +/** >> > + * kmem_valid_obj - does the pointer reference a valid slab object? >> > + * @object: pointer to query. >> > + * >> > + * Return: %true if the pointer is to a not-yet-freed object from >> > + * kmalloc() or kmem_cache_alloc(), either %true or %false if the pointer >> > + * is to an already-freed object, and %false otherwise. >> > + */ >> >> It should be possible to find out more about object being free or not, than you >> currently do. At least to find out if it's definitely free. When it appears >> allocated, it can be actually still free in some kind of e.g. per-cpu or >> per-node cache that would be infeasible to check. But that improvement to the >> output can be also added later. Also SLUB stores the freeing stacktrace, which >> might be useful... > > I can see how this could help debugging a use-after-free situation, > at least as long as the poor sap that subsequently allocated it doesn't > free it. > > I can easily add more fields to the kmem_provenance structure. Maybe > it would make sense to have another exported API that you provide a > kmem_provenance structure to, and it fills it in. > > One caution though... I rely on the object being allocated. > If it officially might already be freed, complex and high-overhead > synchronization seems to be required to safely access the various data > structures. Good point! It's easy to forget that when being used to similar digging in a crash dump, where nothing changes. > So any use on an already-freed object is on a "you break it you get to > keep the pieces" basis. On the other hand, if you are dealing with a > use-after-free situation, life is hard anyway. Yeah, even now I think it's potentially dangerous, as you can get kmem_valid_obj() as true because PageSlab(page) is true. But the object might be already free, so as soon as another CPU frees another object from the same slab page, the page gets also freed... or it was already freed and then allocated by another slab so it's PageSlab() again. I guess at least some safety could be achieved by pinning the page with get_page_unless_zero. But maybe your current implementation is already safe, need to check in detail. > Or am I missing your point? > >> > +bool kmem_valid_obj(void *object) >> > +{ >> > + struct page *page; >> > + >> > + if (!virt_addr_valid(object)) >> > + return false; >> > + page = virt_to_head_page(object); >> > + return PageSlab(page); >> > +} >> > +EXPORT_SYMBOL_GPL(kmem_valid_obj); >> > + >> > +/** >> > + * kmem_dump_obj - Print available slab provenance information >> > + * @object: slab object for which to find provenance information. >> > + * >> > + * This function uses pr_cont(), so that the caller is expected to have >> > + * printed out whatever preamble is appropriate. The provenance information >> > + * depends on the type of object and on how much debugging is enabled. >> > + * For a slab-cache object, the fact that it is a slab object is printed, >> > + * and, if available, the slab name, return address, and stack trace from >> > + * the allocation of that object. >> > + * >> > + * This function will splat if passed a pointer to a non-slab object. >> > + * If you are not sure what type of object you have, you should instead >> > + * use mem_dump_obj(). >> > + */ >> > +void kmem_dump_obj(void *object) >> > +{ >> > + int i; >> > + struct page *page; >> > + struct kmem_provenance kp; >> > + >> > + if (WARN_ON_ONCE(!virt_addr_valid(object))) >> > + return; >> > + page = virt_to_head_page(object); >> > + if (WARN_ON_ONCE(!PageSlab(page))) { >> > + pr_cont(" non-slab memory.\n"); >> > + return; >> > + } >> > + kp.kp_ptr = object; >> > + kp.kp_page = page; >> > + kp.kp_nstack = KS_ADDRS_COUNT; >> > + kmem_provenance(&kp); >> >> You don't seem to be printing kp.kp_objp anywhere? (unless in later patch, but >> would make sense in this patch already). > > Good point! > > However, please note that the various debugging options that reserve > space at the beginning. This can make the meaning of kp.kp_objp a bit > different than one might expect. Yeah, I think the best would be to match the address that kmalloc/kmem_cache_alloc() would return, thus the beginning of the object itself, so you can calculate the offset within it, etc. >> > --- a/mm/util.c >> > +++ b/mm/util.c >> >> I think mm/debug.c is a better fit as it already has dump_page() of a similar >> nature. Also you can call that from from mem_dump_obj() at least in case when >> the more specific handlers fail. It will even include page_owner info if enabled! :) > > I will count this as one vote for mm/debug.c. > > Two things to consider, though... First, Joonsoo suggests that the fact > that this produces useful information without any debugging information > enabled makes it not be debugging as such. Well there's already dump_page() which also produces information without special configs. We're not the best subsystem in this kind of consistency... > Second, mm/debug.c does > not include either slab.h or vmalloc.h. The second might not be a > showstopper, but I was interpreting this to mean that its role was > less central. I think it can include whatever becomes needed there :) > Thanx, Paul > >> Thanks, >> Vlastimil >> >> > @@ -970,3 +970,28 @@ int __weak memcmp_pages(struct page *page1, struct page *page2) >> > kunmap_atomic(addr1); >> > return ret; >> > } >> > + >> > +/** >> > + * mem_dump_obj - Print available provenance information >> > + * @object: object for which to find provenance information. >> > + * >> > + * This function uses pr_cont(), so that the caller is expected to have >> > + * printed out whatever preamble is appropriate. The provenance information >> > + * depends on the type of object and on how much debugging is enabled. >> > + * For example, for a slab-cache object, the slab name is printed, and, >> > + * if available, the return address and stack trace from the allocation >> > + * of that object. >> > + */ >> > +void mem_dump_obj(void *object) >> > +{ >> > + if (!virt_addr_valid(object)) { >> > + pr_cont(" non-paged (local) memory.\n"); >> > + return; >> > + } >> > + if (kmem_valid_obj(object)) { >> > + kmem_dump_obj(object); >> > + return; >> > + } >> > + pr_cont(" non-slab memory.\n"); >> > +} >> > +EXPORT_SYMBOL_GPL(mem_dump_obj); >> > >> >