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 X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DA4EEC433E1 for ; Mon, 8 Jun 2020 10:51:49 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8363D2076A for ; Mon, 8 Jun 2020 10:51:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8363D2076A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id D19446B0003; Mon, 8 Jun 2020 06:51:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CA34A6B0005; Mon, 8 Jun 2020 06:51:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B434C6B0006; Mon, 8 Jun 2020 06:51:48 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0230.hostedemail.com [216.40.44.230]) by kanga.kvack.org (Postfix) with ESMTP id 9725A6B0003 for ; Mon, 8 Jun 2020 06:51:48 -0400 (EDT) Received: from smtpin27.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 4DED05A0F8 for ; Mon, 8 Jun 2020 10:51:48 +0000 (UTC) X-FDA: 76905729096.27.loaf81_2516c8126db9 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin27.hostedemail.com (Postfix) with ESMTP id 135D95DC0F for ; Mon, 8 Jun 2020 10:51:48 +0000 (UTC) X-HE-Tag: loaf81_2516c8126db9 X-Filterd-Recvd-Size: 5830 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf08.hostedemail.com (Postfix) with ESMTP for ; Mon, 8 Jun 2020 10:51:47 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 76DDDAC37; Mon, 8 Jun 2020 10:51:48 +0000 (UTC) Subject: Re: slub freelist issue / BUG: unable to handle page fault for address: 000000003ffe0018 To: Kees Cook Cc: Vegard Nossum , "Rafael J. Wysocki" , Robert Moore , Erik Kaneda , "Rafael J. Wysocki" , Christoph Lameter , Andrew Morton , Marco Elver , Waiman Long , LKML , Linux MM , ACPI Devel Maling List , Len Brown , Steven Rostedt , Roman Gushchin References: <202006041054.874AA564@keescook> <34455dce-6675-1fc2-8d61-45bf56f3f554@suse.cz> <6b2b149e-c2bc-f87a-ea2c-3046c5e39bf9@oracle.com> <894e8cee-33df-1f63-fb12-72dceb024ea7@oracle.com> <202006050828.F85A75D13@keescook> <92d994be-e4f5-b186-4ad7-21828de44967@suse.cz> <202006051053.A61A42374C@keescook> From: Vlastimil Babka Message-ID: <228295be-2e40-3c91-68a0-2715e66486d1@suse.cz> Date: Mon, 8 Jun 2020 12:51:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: <202006051053.A61A42374C@keescook> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 135D95DC0F X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam01 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 6/5/20 8:46 PM, Kees Cook wrote: >> >> Hmm I have a different idea. The whole cache_from_obj() was added because of >> kmemcg (commit b9ce5ef49f00d) where per-memcg cache can be different from the >> root one. And I just realized this usecase can go away with Roman's series [1]. >> But cache_from_obj() also kept the original SLUB consistency check case, and you >> added the freelist hardening case. If kmemcg use case went away it would be nice >> to avoid the virt_to_cache() and check completely again, unless in debugging or >> hardened kernel. > > Is it that expensive? (I'm fine with it staying behind debug/hardening, > but if we can make it on by default, that'd be safer.) Well, it's fast path and e.g. networking guys did a lot of work to optimize SLUB. If we decide to stop trusting the supplied cache pointer completely, we can deprecate kmem_cache_free() and use kfree() everywhere (SLOB would need some adjustments to store size with each object like for kmalloc) but it would have to be a conscious decision. >> Furthermore, the original SLUB debugging case was an unconditional pr_err() plus >> WARN_ON_ONCE(1), which was kept by commit b9ce5ef49f00d. With freelist >> hardening this all changed to WARN_ONCE. So the second and later cases are not >> reported at all for hardening and also not for explicitly enabled debugging like >> in this case, which is IMHO not ideal. > > Oh, I have no problem with WARN vs WARN_ONCE -- there's no reason to > split this. And I'd love the hardening side to gain the tracking call > too, if it's available. > > I had just used WARN_ONCE() since sometimes it can be very noisy to keep > warning for some condition that might not be correctable. OK. >> So I propose the following - the freelist hardening case keeps the WARN_ONCE, >> but also a one-line pr_err() for each case so they are not silent. The SLUB >> debugging case is always a full warning, and printing the tracking info if >> enabled and available. Pure kmemcg case does virt_to_cache() for now (until >> hopefully removed by Roman's series) but no checking at all. Would that work for >> everyone? >> [...] >> @@ -520,9 +528,18 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) >> return s; >> >> cachep = virt_to_cache(x); >> - WARN_ONCE(cachep && !slab_equal_or_root(cachep, s), >> - "%s: Wrong slab cache. %s but object is from %s\n", >> - __func__, s->name, cachep->name); >> + if (unlikely(s->flags & SLAB_CONSISTENCY_CHECKS)) { >> + if (WARN(cachep && !slab_equal_or_root(cachep, s), >> + "%s: Wrong slab cache. %s but object is from %s\n", >> + __func__, s->name, cachep->name)) >> + slab_print_tracking(cachep, x); >> + } else if (IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED)) { >> + if (unlikely(cachep && !slab_equal_or_root(cachep, s))) { >> + pr_err("%s: Wrong slab cache. %s but object is from %s\n", >> + __func__, s->name, cachep->name); >> + WARN_ON_ONCE(1); >> + } >> + } > > How about just this (in addition to your slab_print_tracking() refactor): That could work, I will send a proper patch. > diff --git a/mm/slab.h b/mm/slab.h > index 207c83ef6e06..107b7f6db3c3 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -520,9 +520,10 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) > return s; > > cachep = virt_to_cache(x); > - WARN_ONCE(cachep && !slab_equal_or_root(cachep, s), > + if (WARN(cachep && !slab_equal_or_root(cachep, s), > "%s: Wrong slab cache. %s but object is from %s\n", > - __func__, s->name, cachep->name); > + __func__, s->name, cachep->name)) > + slab_print_tracking(cachep, x); > return cachep; > } > >