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=-18.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham 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 05E2BC47088 for ; Wed, 26 May 2021 19:28:11 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 93631613D3 for ; Wed, 26 May 2021 19:28:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 93631613D3 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 303596B0070; Wed, 26 May 2021 15:28:10 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2BF976B0071; Wed, 26 May 2021 15:28:10 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 12DBC6B0072; Wed, 26 May 2021 15:28:10 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0144.hostedemail.com [216.40.44.144]) by kanga.kvack.org (Postfix) with ESMTP id D1D2F6B0070 for ; Wed, 26 May 2021 15:28:09 -0400 (EDT) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 5BC6982499B9 for ; Wed, 26 May 2021 19:28:09 +0000 (UTC) X-FDA: 78184367898.01.6AB3E0D Received: from mail-io1-f41.google.com (mail-io1-f41.google.com [209.85.166.41]) by imf15.hostedemail.com (Postfix) with ESMTP id BCFEDA0001C7 for ; Wed, 26 May 2021 19:28:04 +0000 (UTC) Received: by mail-io1-f41.google.com with SMTP id z24so2248388ioi.3 for ; Wed, 26 May 2021 12:28:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=gCq8NcQxXA6r8RU5X2XGNe+9K0cAkn6vm+7mYoaBAcA=; b=tPd2PAnBZaXVKlZEktTqh+KinK9P+C4EueZC9IRX/SFjEw2NwN3siD7OaDbjIUR76K w/0nH8JKg16CKNI+cvWtNxIqeWwrJiMDe983tEw/tBxK6MacRWyJxxsUzxIiv9yJpeLm KomT8xnAnCkrFOM0u7REG/qU2SctEAydCEmfam7IfumGAdtF5LjJom50jEjnU3/TRX2F yH6Seuzdu1XqtaeYTJp5iu9n+NNEaUDYRGrVRXZgv5ekaGSvAnWu6416oxx2kjZ/qZJl jHjBjlTYToWytawpzc25cToGCUz+FCP3FeNohnLus+f7hDDAQ20tXR/wOD3Nb7K7lknA TYug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=gCq8NcQxXA6r8RU5X2XGNe+9K0cAkn6vm+7mYoaBAcA=; b=DHag5VpIcp8qwBEsTIjI7y3XZsOKmNpeNI9mbpWh2Ye6T0ySmjzJgSMipFnEk+cdfb zvsxkEUt64/osaipiiPObb3WEdZYJ5SCyz9sJ/h0EnKOsdZZBuPokVOwuRrZltiOox7J jS6QfiDYczXR4lik5JAYxdwHeeu6z6sVUjj8IlMiRxb+vTlCn/stlrhP+D1oe1N70KZ/ 9igAEwEuOPHnXhBM0phQ9a5RTHzOSy2QpnyqRfNuH/KKBNoNeDlBInLyDdrj7JUlZ7Ib 8dAUvSuuGtQyO7qmxgbAafYVATVLiCqapq2m6GHa2dsimxHxtBYR3oOBIsh/IGEPRBvQ 3R0Q== X-Gm-Message-State: AOAM533AUD3LwHihWPjL5/fy/rzjHwUvhZehi3caH8NpevSScqKedg8J rdpajEcrD5tLH2G4YcIOoAVZnMlzZ+2uA71g4fuehQ== X-Google-Smtp-Source: ABdhPJz+Zvjq8TZ+zUItDmUeLh9Tg2irfu83UGwCHSk0/zzH7JO/t8xuE5qsh04sbZpeNzaoZ5k9q8Nu3UU6GltlhuU= X-Received: by 2002:a02:93a4:: with SMTP id z33mr4660280jah.107.1622057288166; Wed, 26 May 2021 12:28:08 -0700 (PDT) MIME-Version: 1.0 References: <78af73393175c648b4eb10312825612f6e6889f6.1620849613.git.pcc@google.com> In-Reply-To: From: Peter Collingbourne Date: Wed, 26 May 2021 12:27:56 -0700 Message-ID: Subject: Re: [PATCH v3 1/3] kasan: use separate (un)poison implementation for integrated init To: Marco Elver Cc: Andrey Konovalov , Alexander Potapenko , Catalin Marinas , Vincenzo Frascino , Andrew Morton , Evgenii Stepanov , Linux Memory Management List , Linux ARM , kasan-dev Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20161025 header.b=tPd2PAnB; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of pcc@google.com designates 209.85.166.41 as permitted sender) smtp.mailfrom=pcc@google.com X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: BCFEDA0001C7 X-Stat-Signature: w79js83moy713mty8g8dg3a6831cc6a5 X-HE-Tag: 1622057284-684572 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, May 26, 2021 at 3:12 AM Marco Elver wrote: > > On Wed, May 12, 2021 at 01:09PM -0700, Peter Collingbourne wrote: > [...] > > +void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags); > > +void kasan_free_pages(struct page *page, unsigned int order); > > + > > #else /* CONFIG_KASAN_HW_TAGS */ > > > > static inline bool kasan_enabled(void) > > { > > +#ifdef CONFIG_KASAN > > return true; > > +#else > > + return false; > > +#endif > > } > > Just > > return IS_ENABLED(CONFIG_KASAN); Will do. > > static inline bool kasan_has_integrated_init(void) > > @@ -113,8 +113,30 @@ static inline bool kasan_has_integrated_init(void) > > return false; > > } > > > > +static __always_inline void kasan_alloc_pages(struct page *page, > > + unsigned int order, gfp_t flags) > > +{ > > + /* Only available for integrated init. */ > > + BUILD_BUG(); > > +} > > + > > +static __always_inline void kasan_free_pages(struct page *page, > > + unsigned int order) > > +{ > > + /* Only available for integrated init. */ > > + BUILD_BUG(); > > +} > > This *should* always work, as long as the compiler optimizes everything > like we expect. Yeah, as I mentioned to Catalin on an earlier revision I'm not a fan of relying on the compiler optimizing this away, but it looks like we're already relying on this elsewhere in the kernel. > But: In this case, I think this is sign that the interface design can be > improved. Can we just make kasan_{alloc,free}_pages() return a 'bool > __must_check' to indicate if kasan takes care of init? I considered a number of different approaches including something like that before settling on the one in this patch. One consideration was that we should avoid involving KASAN in normal execution as much as possible, in order to make the normal code path as comprehensible as possible. With an approach where alloc/free return a bool the reader needs to understand what the KASAN alloc/free functions do in the normal case. Whereas with an approach where an "accessor" function on the KASAN side returns a bool, it's more obvious that the code has a "normal path" and a "KASAN path", and readers who only care about the normal path can ignore the KASAN path. Does that make sense? I don't feel too strongly so I can change alloc/free to return a bool if you don't agree. > The variants here would simply return kasan_has_integrated_init(). > > That way, there'd be no need for the BUILD_BUG()s and the interface > becomes harder to misuse by design. > > Also, given that kasan_{alloc,free}_pages() initializes memory, this is > an opportunity to just give them a better name. Perhaps > > /* Returns true if KASAN took care of initialization, false otherwise. */ > bool __must_check kasan_alloc_pages_try_init(struct page *page, unsigned int order, gfp_t flags); > bool __must_check kasan_free_pages_try_init(struct page *page, unsigned int order); I considered changing the name but concluded that we probably shouldn't try to pack too much information into the name. With a code flow like: if (kasan_has_integrated_init()) { kasan_alloc_pages(); } else { kernel_init_free_pages(); } I think it's probably clear enough that kasan_alloc_pages() is doing the stuff in kernel_init_free_pages() as well. > [...] > > - init = want_init_on_free(); > > - if (init && !kasan_has_integrated_init()) > > - kernel_init_free_pages(page, 1 << order); > > - kasan_free_nondeferred_pages(page, order, init, fpi_flags); > > + if (kasan_has_integrated_init()) { > > + if (!skip_kasan_poison) > > + kasan_free_pages(page, order); > > I think kasan_free_pages() could return a bool, and this would become > > if (skip_kasan_poison || !kasan_free_pages(...)) { > ... > > > + } else { > > + bool init = want_init_on_free(); > > + > > + if (init) > > + kernel_init_free_pages(page, 1 << order); > > + if (!skip_kasan_poison) > > + kasan_poison_pages(page, order, init); > > + } > > > > /* > > * arch_free_page() can make the page's contents inaccessible. s390 > > @@ -2324,8 +2324,6 @@ static bool check_new_pages(struct page *page, unsigned int order) > > inline void post_alloc_hook(struct page *page, unsigned int order, > > gfp_t gfp_flags) > > { > > - bool init; > > - > > set_page_private(page, 0); > > set_page_refcounted(page); > > > > @@ -2344,10 +2342,16 @@ inline void post_alloc_hook(struct page *page, unsigned int order, > > * kasan_alloc_pages and kernel_init_free_pages must be > > * kept together to avoid discrepancies in behavior. > > */ > > - init = !want_init_on_free() && want_init_on_alloc(gfp_flags); > > - kasan_alloc_pages(page, order, init); > > - if (init && !kasan_has_integrated_init()) > > - kernel_init_free_pages(page, 1 << order); > > + if (kasan_has_integrated_init()) { > > + kasan_alloc_pages(page, order, gfp_flags); > > It looks to me that kasan_alloc_pages() could return a bool, and this > would become > > if (!kasan_alloc_pages(...)) { > ... > > > + } else { > > + bool init = > > + !want_init_on_free() && want_init_on_alloc(gfp_flags); > > + > > [ No need for line-break (for cases like this the kernel is fine with up > to 100 cols if it improves readability). ] Okay, I'll make that change. Peter