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 2AD86C76196 for ; Tue, 28 Mar 2023 14:13:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 868066B0071; Tue, 28 Mar 2023 10:13:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 818746B0074; Tue, 28 Mar 2023 10:13:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6DFBD6B0075; Tue, 28 Mar 2023 10:13:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 5B7E96B0071 for ; Tue, 28 Mar 2023 10:13:40 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 2343FA02CE for ; Tue, 28 Mar 2023 14:13:40 +0000 (UTC) X-FDA: 80618500200.17.61A6F93 Received: from mail-yb1-f170.google.com (mail-yb1-f170.google.com [209.85.219.170]) by imf11.hostedemail.com (Postfix) with ESMTP id 5A08040017 for ; Tue, 28 Mar 2023 14:13:36 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=ZxRNUzUx; spf=pass (imf11.hostedemail.com: domain of elver@google.com designates 209.85.219.170 as permitted sender) smtp.mailfrom=elver@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1680012816; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=jzWLbAObXvnp3yLVMyDVLp9e3kaBgXUsN1mJJacdlX8=; b=xvpb5U7jv8PElrxTZd7KLecr7msW5OAUs0MaZUqNELG1705C0JCvBKKiYc/2GLesnM3UZy eqOmTfEjjBPJrWx3DeGAgIHYj5IoAOZwCVIWrYO0dHXqB5d6eVlpPNGFMk3HjQ9LTxiOz0 aZmHPGO+Wjdez3XjC5FOp1fkePtb7vg= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=ZxRNUzUx; spf=pass (imf11.hostedemail.com: domain of elver@google.com designates 209.85.219.170 as permitted sender) smtp.mailfrom=elver@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680012816; a=rsa-sha256; cv=none; b=tNxYH9bnjJBeq+UyDBzZdiQcYTu8vlsMeLDsJevI6plWSfzxzYkuF3Nh0E6bRHyvAuvC8/ fy/rK0Le3tgaaYSJHJrXeBqYiTAxqDi8Xd2jEPyi4a+vmRUeV+xPHgyqW8OzoGqyxC+bAu /FDfP62RtXp8JYJH06A0KvslQPayQfw= Received: by mail-yb1-f170.google.com with SMTP id p204so15185561ybc.12 for ; Tue, 28 Mar 2023 07:13:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680012815; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=jzWLbAObXvnp3yLVMyDVLp9e3kaBgXUsN1mJJacdlX8=; b=ZxRNUzUxLB9REoDREOixbz4eFEWuyEqwmhgyBF6WmbsF00RDZFKU5uzC2pdcoOfRPK QBanYwehL+UDx9Pvcs5Nei1iZHw7D1OwTezMUMs4SAk8OKgL4d26BIa9q30tXs1nymei 7qtkgJ19TKcEHmuQ5LgIxxcOvUrf364zTTlDDt35fFK8HnN3WIJaqu3YkoFpdASeB5x+ zW/6L7mkZ/nQcroYwbW3JZc9x78AtLtB6+h/7OCWHr/F5NfBQx7W5DytnmcAYgfoZuyZ wam0ugMe4jX6adpjoJdzkCrbVoSl1YsUzVqNCDAM9K44qNAdJ6x8pQtsV21EopaDBvPz IEog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680012815; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=jzWLbAObXvnp3yLVMyDVLp9e3kaBgXUsN1mJJacdlX8=; b=J4K50yGCi1epjGAjm2Z+hF4EwR70b7KuSFIlCMZgXDMU9tt7PEOXkeqwlDbMj09xnZ tqFzRO1/lpnQ8Ggd+XveAQqwdBQjKG66ZC4ZS3wDMU+tGoa7YcxZqvahcG1O9cUY0Yk0 zGuVzfAaFUBBqZLH46uTjJ6vuOnORDb22hcanEgtX6gXr8ntJFeJXCVj631K7bzerC2t PXSt4aCe/2IRMdEyDDJFfVTolmMCST2TvNTkG/fN0RXBusNFZKlsxrcvkGCoXwL2ef48 VdcSnUY6V4+vs3VwWTcyblpvCKS+c5zEGbass3eKqKxeg9srP8Nts3L21+W0Fh2OQR6U 8e8A== X-Gm-Message-State: AAQBX9cb8TBZCzGxiCEkCC4Tvui5TUHtVtbq1zFtLesoSUm1qKccLTaP ZV7ThquKh/ZMk5+AIVd0i2GhQ2bZSoKhhkdDfDwulg== X-Google-Smtp-Source: AKy350Yel3s2ZrfmqC5v9KmHoPxUi+CMV8z/smBb5noBYFwid1ZitraBvLeD9d7uUR0i6sboB64GES0/JGWIFMA/1DQ= X-Received: by 2002:a25:1fc2:0:b0:b59:172a:eafa with SMTP id f185-20020a251fc2000000b00b59172aeafamr12664636ybf.17.1680012815308; Tue, 28 Mar 2023 07:13:35 -0700 (PDT) MIME-Version: 1.0 References: <20230328095807.7014-1-songmuchun@bytedance.com> <20230328095807.7014-6-songmuchun@bytedance.com> <291FB0BF-F824-4ED9-B836-DA7773BFDA48@linux.dev> In-Reply-To: <291FB0BF-F824-4ED9-B836-DA7773BFDA48@linux.dev> From: Marco Elver Date: Tue, 28 Mar 2023 16:12:58 +0200 Message-ID: Subject: Re: [PATCH 5/6] mm: kfence: change kfence pool page layout To: Muchun Song Cc: Muchun Song , glider@google.com, dvyukov@google.com, akpm@linux-foundation.org, jannh@google.com, sjpark@amazon.de, kasan-dev@googlegroups.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 5A08040017 X-Stat-Signature: d1gedcbbzfzpibkafwq8qhnnd3b91w9y X-Rspam-User: X-HE-Tag: 1680012816-106291 X-HE-Meta: U2FsdGVkX1/aQiOtBW6FS0Z2aBeRopUfh0RdMsLOdlQ2OYKoKbg8XJ4qqGbsV9qAV5HNevyBF9cqGo3wHp1H0jbU/OmUW9N7/4kwKvXWwW99M9D5ZLhjSmAjhtkvji1UQwzIS59fPDzbiakwZqYQM8weS0Oe7kyrX22I9VeH+QjFSXRxvbQTpK2G8BbuzdF1BDsjAdADM60f3Pk4v7ZFGBLkNrSXf/NCFsrmnBBf72EK5qonIjT7a4MLpSGH5EjvAVK9kjauu7qzBm0lZzEUWBXbvLcc/2eRoAY3sUuKQ1GSPNXtqt7cSVnIMgRA2TVQeupMihoknjjTfgqJOKylMQlqHk2RmZgbLoMpr7ckBF/6hC0NyE7FkjgDt9u1OOempZtyAEXTEkb/50FQABVANcvS7mH0GRc2L/KMePpKPmUFZ/mLMTjtjn8/vD8/R2nOXVi1bmx/Vh4q/OLqGHZHFACwKsDNB0/gLjbqox1sr1Q+5Q7Fik9bzvqSt6lhhCE2MBvXjkEV/wYZCz95nCRWmPN8iGBZDGLOw5PiKXKDfOlcL1dFtdgizYZkYy0c7fnT5fJ782VQwv1fL8UbKO9d1auaP5vAKWyA8NxE2YAUb5Nmbc6/J0PPkQcHnIMMGPY856jgTyZ/3c8AYUz5vUPx/G84dVXFF5okXVA5LzvnIu4U3IlL1coaoro7ESLxlOsGc+9nvm98qXk/FpqYqMD5sSNh+AQ+i8BtGUfCuHzZhjkgmoJJZeNXFgE2jnVdMmiXRevCluWEe+VIDa/wFTN/PX9XkieJWUv5vCDIcYRXvp107+XixN1zHj1oMCHwF+nkh5EnenY6N6Q/chfGjPzP7EnfVk1e/QAAKKDkKQJWZOY3zvKMsPeO5eGnjo7STscG1+QUEZXYyZuVdhMHxbGmI6RKWAl2izI2zXODm30YBlow6+H4bSJ256TVukONoMfen7ugdv7BfJ6hh7YFuQj 63kzXuOv ak8cMqiejNjrE9aBtCUkZ+w5AMlLvqMVk1ldYLTHng/6FylBQ/hvuG6v2KxoH2ByV39JXc/tCe0732chrFM8Y/I6XSvgCR1zaLil1xlwDaQt9UrOONuceFQBJnwA2ZCNdJKZGJ8cbucVkudrcHeJIMYDHzu4WQc6qs41GZ9LkIctYDfTsdpUdO+KJtaraH/KPFo/GqxYIK5cFEWtD75hSseQPTcUXAqRVRjqZw4Pazr5M1SD8oFEk/crBYlzRVbtNPFUdHlfRChMpjCfmiZfu0LNTj4XlVuImW2WkubbxvRZjLfP9eoFMILqAm/6Q1qw0TQS+OKSbc/RD7kt9vxbpfsth9UksWWtGqiThJzmLl8p2ZGPfFXBKnBBuPEF8kkecr+CX 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 Tue, 28 Mar 2023 at 15:33, Muchun Song wrote: > > > > > On Mar 28, 2023, at 20:59, Marco Elver wrote: > > > > On Tue, 28 Mar 2023 at 11:58, 'Muchun Song' via kasan-dev > > wrote: > >> > >> The original kfence pool layout (Given a layout with 2 objects): > >> > >> +------------+------------+------------+------------+------------+------------+ > >> | guard page | guard page | object | guard page | object | guard page | > >> +------------+------------+------------+------------+------------+------------+ > >> | | | > >> +----kfence_metadata[0]---+----kfence_metadata[1]---+ > >> > >> The comment says "the additional page in the beginning gives us an even > >> number of pages, which simplifies the mapping of address to metadata index". > >> > >> However, removing the additional page does not complicate any mapping > >> calculations. So changing it to the new layout to save a page. And remmove > >> the KFENCE_ERROR_INVALID test since we cannot test this case easily. > >> > >> The new kfence pool layout (Given a layout with 2 objects): > >> > >> +------------+------------+------------+------------+------------+ > >> | guard page | object | guard page | object | guard page | > >> +------------+------------+------------+------------+------------+ > >> | | | > >> +----kfence_metadata[0]---+----kfence_metadata[1]---+ > >> > >> Signed-off-by: Muchun Song > >> --- > >> include/linux/kfence.h | 8 ++------ > >> mm/kfence/core.c | 40 ++++++++-------------------------------- > >> mm/kfence/kfence.h | 2 +- > >> mm/kfence/kfence_test.c | 14 -------------- > >> 4 files changed, 11 insertions(+), 53 deletions(-) > >> > >> diff --git a/include/linux/kfence.h b/include/linux/kfence.h > >> index 726857a4b680..25b13a892717 100644 > >> --- a/include/linux/kfence.h > >> +++ b/include/linux/kfence.h > >> @@ -19,12 +19,8 @@ > >> > >> extern unsigned long kfence_sample_interval; > >> > >> -/* > >> - * We allocate an even number of pages, as it simplifies calculations to map > >> - * address to metadata indices; effectively, the very first page serves as an > >> - * extended guard page, but otherwise has no special purpose. > >> - */ > >> -#define KFENCE_POOL_SIZE ((CONFIG_KFENCE_NUM_OBJECTS + 1) * 2 * PAGE_SIZE) > >> +/* The last page serves as an extended guard page. */ > > > > The last page is just a normal guard page? I.e. the last 2 pages are: > > | > > Right. > > The new kfence pool layout (Given a layout with 2 objects): > > +------------+------------+------------+------------+------------+ > | guard page | object | guard page | object | guard page | > +------------+------------+------------+------------+------------+ > | | | ^ > +----kfence_metadata[0]---+----kfence_metadata[1]---+ | > | > | > the last page > > > > > Or did I misunderstand? > > > >> +#define KFENCE_POOL_SIZE ((CONFIG_KFENCE_NUM_OBJECTS * 2 + 1) * PAGE_SIZE) > >> extern char *__kfence_pool; > >> > >> DECLARE_STATIC_KEY_FALSE(kfence_allocation_key); > >> diff --git a/mm/kfence/core.c b/mm/kfence/core.c > >> index 41befcb3b069..f205b860f460 100644 > >> --- a/mm/kfence/core.c > >> +++ b/mm/kfence/core.c > >> @@ -240,24 +240,7 @@ static inline void kfence_unprotect(unsigned long addr) > >> > >> static inline unsigned long metadata_to_pageaddr(const struct kfence_metadata *meta) > >> { > >> - unsigned long offset = (meta - kfence_metadata + 1) * PAGE_SIZE * 2; > >> - unsigned long pageaddr = (unsigned long)&__kfence_pool[offset]; > >> - > >> - /* The checks do not affect performance; only called from slow-paths. */ > >> - > >> - /* Only call with a pointer into kfence_metadata. */ > >> - if (KFENCE_WARN_ON(meta < kfence_metadata || > >> - meta >= kfence_metadata + CONFIG_KFENCE_NUM_OBJECTS)) > >> - return 0; > > > > Could we retain this WARN_ON? Or just get rid of > > metadata_to_pageaddr() altogether, because there's only 1 use left and > > the function would now just be a simple ALIGN_DOWN() anyway. > > I'll inline this function to its caller since the warning is unlikely. > > > > >> - /* > >> - * This metadata object only ever maps to 1 page; verify that the stored > >> - * address is in the expected range. > >> - */ > >> - if (KFENCE_WARN_ON(ALIGN_DOWN(meta->addr, PAGE_SIZE) != pageaddr)) > >> - return 0; > >> - > >> - return pageaddr; > >> + return ALIGN_DOWN(meta->addr, PAGE_SIZE); > >> } > >> > >> /* > >> @@ -535,34 +518,27 @@ static void kfence_init_pool(void) > >> unsigned long addr = (unsigned long)__kfence_pool; > >> int i; > >> > >> - /* > >> - * Protect the first 2 pages. The first page is mostly unnecessary, and > >> - * merely serves as an extended guard page. However, adding one > >> - * additional page in the beginning gives us an even number of pages, > >> - * which simplifies the mapping of address to metadata index. > >> - */ > >> - for (i = 0; i < 2; i++, addr += PAGE_SIZE) > >> - kfence_protect(addr); > >> - > >> for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++, addr += 2 * PAGE_SIZE) { > >> struct kfence_metadata *meta = &kfence_metadata[i]; > >> - struct slab *slab = page_slab(virt_to_page(addr)); > >> + struct slab *slab = page_slab(virt_to_page(addr + PAGE_SIZE)); > >> > >> /* Initialize metadata. */ > >> INIT_LIST_HEAD(&meta->list); > >> raw_spin_lock_init(&meta->lock); > >> meta->state = KFENCE_OBJECT_UNUSED; > >> - meta->addr = addr; /* Initialize for validation in metadata_to_pageaddr(). */ > >> + meta->addr = addr + PAGE_SIZE; > >> list_add_tail(&meta->list, &kfence_freelist); > >> > >> - /* Protect the right redzone. */ > >> - kfence_protect(addr + PAGE_SIZE); > >> + /* Protect the left redzone. */ > >> + kfence_protect(addr); > >> > >> __folio_set_slab(slab_folio(slab)); > >> #ifdef CONFIG_MEMCG > >> slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS; > >> #endif > >> } > >> + > >> + kfence_protect(addr); > >> } > >> > >> static bool __init kfence_init_pool_early(void) > >> @@ -1043,7 +1019,7 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs > >> > >> atomic_long_inc(&counters[KFENCE_COUNTER_BUGS]); > >> > >> - if (page_index % 2) { > >> + if (page_index % 2 == 0) { > >> /* This is a redzone, report a buffer overflow. */ > >> struct kfence_metadata *meta; > >> int distance = 0; > >> diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h > >> index 600f2e2431d6..249d420100a7 100644 > >> --- a/mm/kfence/kfence.h > >> +++ b/mm/kfence/kfence.h > >> @@ -110,7 +110,7 @@ static inline struct kfence_metadata *addr_to_metadata(unsigned long addr) > >> * __kfence_pool, in which case we would report an "invalid access" > >> * error. > >> */ > >> - index = (addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2) - 1; > >> + index = (addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2); > >> if (index < 0 || index >= CONFIG_KFENCE_NUM_OBJECTS) > >> return NULL; > > > > Assume there is a right OOB that hit the last guard page. In this case > > > > addr >= __kfence_pool + (NUM_OBJECTS * 2 * PAGE_SIZE) && addr < > > __kfence_pool + POOL_SIZE > > > > therefore > > > > index >= (NUM_OBJECTS * 2 * PAGE_SIZE) / (PAGE_SIZE * 2) && index < > > POOL_SIZE / (PAGE_SIZE * 2) > > index == NUM_OBJECTS > > > > And according to the above comparison, this will return NULL and > > report KFENCE_ERROR_INVALID, which is wrong. > > Look at kfence_handle_page_fault(), which first look up "addr - PAGE_SIZE" (passed > to addr_to_metadata()) and then look up "addr + PAGE_SIZE", the former will not > return NULL, the latter will return NULL. So kfence will report KFENCE_ERROR_OOB > in this case, right? Or what I missed here? Yes, you're right. Thanks, -- Marco