[PATCH 0/6] Simplify kfence code

[Muchun Song]

This series aims to simplify kfence code, please review each patch separately.

Thanks.

Muchun Song (6):
mm: kfence: simplify kfence pool initialization
mm: kfence: check kfence pool size at building time
mm: kfence: make kfence_protect_page() void
mm: kfence: remove useless check for CONFIG_KFENCE_NUM_OBJECTS
mm: kfence: change kfence pool page layout
mm: kfence: replace ALIGN_DOWN(x, PAGE_SIZE) with PAGE_ALIGN_DOWN(x)

arch/arm/include/asm/kfence.h | 4 +-
arch/arm64/include/asm/kfence.h | 4 +-
arch/parisc/include/asm/kfence.h | 7 +-
arch/powerpc/include/asm/kfence.h | 8 +-
arch/riscv/include/asm/kfence.h | 4 +-
arch/s390/include/asm/kfence.h | 3 +-
arch/x86/include/asm/kfence.h | 9 +-
include/linux/kfence.h | 8 +-
mm/kfence/core.c | 229 +++++++++++++-------------------------
mm/kfence/kfence.h | 2 +-
mm/kfence/kfence_test.c | 14 ---
11 files changed, 89 insertions(+), 203 deletions(-)

--
2.11.0

[Muchun Song]

There are three similar loops to initialize kfence pool, we could merge
all of them into one loop to simplify the code and make code more
efficient.

Signed-off-by: Muchun Song <songm...@bytedance.com>
---
mm/kfence/core.c | 47 ++++++-----------------------------------------
1 file changed, 6 insertions(+), 41 deletions(-)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 7d01a2c76e80..de62a84d4830 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -539,35 +539,10 @@ static void rcu_guarded_free(struct rcu_head *h)
static unsigned long kfence_init_pool(void)
{
unsigned long addr = (unsigned long)__kfence_pool;
- struct page *pages;
int i;

if (!arch_kfence_init_pool())
return addr;
-
- pages = virt_to_page(__kfence_pool);
-
- /*
- * Set up object pages: they must have PG_slab set, to avoid freeing
- * these as real pages.
- *
- * We also want to avoid inserting kfence_free() in the kfree()
- * fast-path in SLUB, and therefore need to ensure kfree() correctly
- * enters __slab_free() slow-path.
- */
- for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) {
- struct slab *slab = page_slab(nth_page(pages, i));
-
- if (!i || (i % 2))
- continue;
-
- __folio_set_slab(slab_folio(slab));
-#ifdef CONFIG_MEMCG
- slab->memcg_data = (unsigned long)&kfence_metadata[i / 2 - 1].objcg |
- MEMCG_DATA_OBJCGS;
-#endif
- }
-
/*
* Protect the first 2 pages. The first page is mostly unnecessary, and
* merely serves as an extended guard page. However, adding one
@@ -581,8 +556,9 @@ static unsigned long kfence_init_pool(void)
addr += PAGE_SIZE;
}

- for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
+ 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));

/* Initialize metadata. */
INIT_LIST_HEAD(&meta->list);
@@ -593,26 +569,15 @@ static unsigned long kfence_init_pool(void)

/* Protect the right redzone. */
if (unlikely(!kfence_protect(addr + PAGE_SIZE)))
- goto reset_slab;
-
- addr += 2 * PAGE_SIZE;
- }
-
- return 0;
-
-reset_slab:
- for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) {
- struct slab *slab = page_slab(nth_page(pages, i));
+ return addr;

- if (!i || (i % 2))
- continue;
+ __folio_set_slab(slab_folio(slab));
#ifdef CONFIG_MEMCG
- slab->memcg_data = 0;
+ slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
#endif
- __folio_clear_slab(slab_folio(slab));
}

- return addr;
+ return 0;
}

static bool __init kfence_init_pool_early(void)
--
2.11.0

[Muchun Song]

Check kfence pool size at building time to expose problem ASAP.

Signed-off-by: Muchun Song <songm...@bytedance.com>
---

mm/kfence/core.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index de62a84d4830..6781af1dfa66 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -841,10 +841,9 @@ static int kfence_init_late(void)
return -ENOMEM;
__kfence_pool = page_to_virt(pages);
#else
- if (nr_pages > MAX_ORDER_NR_PAGES) {
- pr_warn("KFENCE_NUM_OBJECTS too large for buddy allocator\n");
- return -EINVAL;
- }
+ BUILD_BUG_ON_MSG(get_order(KFENCE_POOL_SIZE) > MAX_ORDER,
+ "CONFIG_KFENCE_NUM_OBJECTS is too large for buddy allocator");
+
__kfence_pool = alloc_pages_exact(KFENCE_POOL_SIZE, GFP_KERNEL);
if (!__kfence_pool)
return -ENOMEM;
--
2.11.0

[Muchun Song]

The arch_kfence_init_pool() make sure kfence pool is mapped with base page
size (e.g. 4KB), so the following PTE lookup in kfence_protect_page() will
always succeed. Then there is no way to stop kfence_protect_page() always
returning true, so make it void to simplify the code.

Signed-off-by: Muchun Song <songm...@bytedance.com>
---

arch/arm/include/asm/kfence.h | 4 +-
arch/arm64/include/asm/kfence.h | 4 +-
arch/parisc/include/asm/kfence.h | 7 +-

arch/powerpc/include/asm/kfence.h | 8 +--

arch/riscv/include/asm/kfence.h | 4 +-
arch/s390/include/asm/kfence.h | 3 +-

arch/x86/include/asm/kfence.h | 9 +--
mm/kfence/core.c | 142 +++++++++++++++++---------------------
8 files changed, 73 insertions(+), 108 deletions(-)

diff --git a/arch/arm/include/asm/kfence.h b/arch/arm/include/asm/kfence.h
index 7980d0f2271f..c30a5f8125e8 100644
--- a/arch/arm/include/asm/kfence.h
+++ b/arch/arm/include/asm/kfence.h
@@ -43,11 +43,9 @@ static inline bool arch_kfence_init_pool(void)
return true;
}

-static inline bool kfence_protect_page(unsigned long addr, bool protect)
+static inline void kfence_protect_page(unsigned long addr, bool protect)
{
set_memory_valid(addr, 1, !protect);
-
- return true;
}

#endif /* __ASM_ARM_KFENCE_H */
diff --git a/arch/arm64/include/asm/kfence.h b/arch/arm64/include/asm/kfence.h
index a81937fae9f6..7717c6d98b6f 100644
--- a/arch/arm64/include/asm/kfence.h
+++ b/arch/arm64/include/asm/kfence.h
@@ -12,11 +12,9 @@

static inline bool arch_kfence_init_pool(void) { return true; }

-static inline bool kfence_protect_page(unsigned long addr, bool protect)
+static inline void kfence_protect_page(unsigned long addr, bool protect)
{
set_memory_valid(addr, 1, !protect);
-
- return true;
}

#ifdef CONFIG_KFENCE
diff --git a/arch/parisc/include/asm/kfence.h b/arch/parisc/include/asm/kfence.h
index 6259e5ac1fea..290792009315 100644
--- a/arch/parisc/include/asm/kfence.h
+++ b/arch/parisc/include/asm/kfence.h
@@ -19,13 +19,10 @@ static inline bool arch_kfence_init_pool(void)
}

/* Protect the given page and flush TLB. */
-static inline bool kfence_protect_page(unsigned long addr, bool protect)
+static inline void kfence_protect_page(unsigned long addr, bool protect)
{
pte_t *pte = virt_to_kpte(addr);

- if (WARN_ON(!pte))
- return false;
-
/*
* We need to avoid IPIs, as we may get KFENCE allocations or faults
* with interrupts disabled.
@@ -37,8 +34,6 @@ static inline bool kfence_protect_page(unsigned long addr, bool protect)
set_pte(pte, __pte(pte_val(*pte) | _PAGE_PRESENT));

flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
-
- return true;
}

#endif /* _ASM_PARISC_KFENCE_H */
diff --git a/arch/powerpc/include/asm/kfence.h b/arch/powerpc/include/asm/kfence.h
index 6fd2b4d486c5..9d8502a7d0a4 100644
--- a/arch/powerpc/include/asm/kfence.h
+++ b/arch/powerpc/include/asm/kfence.h
@@ -21,16 +21,14 @@ static inline bool arch_kfence_init_pool(void)
}

#ifdef CONFIG_PPC64
-static inline bool kfence_protect_page(unsigned long addr, bool protect)
+static inline void kfence_protect_page(unsigned long addr, bool protect)
{
struct page *page = virt_to_page(addr);

__kernel_map_pages(page, 1, !protect);
-
- return true;
}
#else
-static inline bool kfence_protect_page(unsigned long addr, bool protect)
+static inline void kfence_protect_page(unsigned long addr, bool protect)
{
pte_t *kpte = virt_to_kpte(addr);

@@ -40,8 +38,6 @@ static inline bool kfence_protect_page(unsigned long addr, bool protect)
} else {
pte_update(&init_mm, addr, kpte, 0, _PAGE_PRESENT, 0);
}
-
- return true;
}
#endif

diff --git a/arch/riscv/include/asm/kfence.h b/arch/riscv/include/asm/kfence.h
index d887a54042aa..1299f47170b5 100644
--- a/arch/riscv/include/asm/kfence.h
+++ b/arch/riscv/include/asm/kfence.h
@@ -46,7 +46,7 @@ static inline bool arch_kfence_init_pool(void)
return true;
}

-static inline bool kfence_protect_page(unsigned long addr, bool protect)
+static inline void kfence_protect_page(unsigned long addr, bool protect)
{
pte_t *pte = virt_to_kpte(addr);

@@ -56,8 +56,6 @@ static inline bool kfence_protect_page(unsigned long addr, bool protect)
set_pte(pte, __pte(pte_val(*pte) | _PAGE_PRESENT));

flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
-
- return true;
}

#endif /* _ASM_RISCV_KFENCE_H */
diff --git a/arch/s390/include/asm/kfence.h b/arch/s390/include/asm/kfence.h
index d55ba878378b..6d7b3632d79c 100644
--- a/arch/s390/include/asm/kfence.h
+++ b/arch/s390/include/asm/kfence.h
@@ -33,10 +33,9 @@ static __always_inline void kfence_split_mapping(void)
#endif
}

-static inline bool kfence_protect_page(unsigned long addr, bool protect)
+static inline void kfence_protect_page(unsigned long addr, bool protect)
{
__kernel_map_pages(virt_to_page(addr), 1, !protect);
- return true;
}

#endif /* _ASM_S390_KFENCE_H */
diff --git a/arch/x86/include/asm/kfence.h b/arch/x86/include/asm/kfence.h
index ff5c7134a37a..6ffd4a078a71 100644
--- a/arch/x86/include/asm/kfence.h
+++ b/arch/x86/include/asm/kfence.h
@@ -38,13 +38,9 @@ static inline bool arch_kfence_init_pool(void)
}

/* Protect the given page and flush TLB. */
-static inline bool kfence_protect_page(unsigned long addr, bool protect)
+static inline void kfence_protect_page(unsigned long addr, bool protect)
{
- unsigned int level;
- pte_t *pte = lookup_address(addr, &level);
-
- if (WARN_ON(!pte || level != PG_LEVEL_4K))
- return false;
+ pte_t *pte = virt_to_kpte(addr);

/*
* We need to avoid IPIs, as we may get KFENCE allocations or faults
@@ -65,7 +61,6 @@ static inline bool kfence_protect_page(unsigned long addr, bool protect)
preempt_disable();
flush_tlb_one_kernel(addr);
preempt_enable();
- return true;
}

#endif /* !MODULE */
diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 6781af1dfa66..5726bf2ae13c 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -229,14 +229,14 @@ static bool alloc_covered_contains(u32 alloc_stack_hash)
return true;
}

-static bool kfence_protect(unsigned long addr)
+static inline void kfence_protect(unsigned long addr)
{
- return !KFENCE_WARN_ON(!kfence_protect_page(ALIGN_DOWN(addr, PAGE_SIZE), true));
+ kfence_protect_page(ALIGN_DOWN(addr, PAGE_SIZE), true);
}

-static bool kfence_unprotect(unsigned long addr)
+static inline void kfence_unprotect(unsigned long addr)
{
- return !KFENCE_WARN_ON(!kfence_protect_page(ALIGN_DOWN(addr, PAGE_SIZE), false));
+ kfence_protect_page(ALIGN_DOWN(addr, PAGE_SIZE), false);
}

static inline unsigned long metadata_to_pageaddr(const struct kfence_metadata *meta)
@@ -531,30 +531,19 @@ static void rcu_guarded_free(struct rcu_head *h)
kfence_guarded_free((void *)meta->addr, meta, false);
}

-/*
- * Initialization of the KFENCE pool after its allocation.
- * Returns 0 on success; otherwise returns the address up to
- * which partial initialization succeeded.
- */
-static unsigned long kfence_init_pool(void)
+static void kfence_init_pool(void)

{
unsigned long addr = (unsigned long)__kfence_pool;

int i;

- if (!arch_kfence_init_pool())
- return addr;

/*
* 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++) {
- if (unlikely(!kfence_protect(addr)))
- return addr;
-
- addr += PAGE_SIZE;
- }
+ 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];

@@ -568,38 +557,33 @@ static unsigned long kfence_init_pool(void)
list_add_tail(&meta->list, &kfence_freelist);

/* Protect the right redzone. */

- if (unlikely(!kfence_protect(addr + PAGE_SIZE)))
- return addr;
+ kfence_protect(addr + PAGE_SIZE);

__folio_set_slab(slab_folio(slab));
#ifdef CONFIG_MEMCG

slab->memcg_data = (unsigned long)&meta->objcg | MEMCG_DATA_OBJCGS;
#endif
}
-

- return 0;
}

static bool __init kfence_init_pool_early(void)
{
- unsigned long addr;
-
if (!__kfence_pool)
return false;

- addr = kfence_init_pool();
-
- if (!addr) {
- /*
- * The pool is live and will never be deallocated from this point on.
- * Ignore the pool object from the kmemleak phys object tree, as it would
- * otherwise overlap with allocations returned by kfence_alloc(), which
- * are registered with kmemleak through the slab post-alloc hook.
- */
- kmemleak_ignore_phys(__pa(__kfence_pool));
- return true;
- }
+ if (!arch_kfence_init_pool())
+ goto free;

+ kfence_init_pool();
+ /*
+ * The pool is live and will never be deallocated from this point on.
+ * Ignore the pool object from the kmemleak phys object tree, as it would
+ * otherwise overlap with allocations returned by kfence_alloc(), which
+ * are registered with kmemleak through the slab post-alloc hook.
+ */
+ kmemleak_ignore_phys(__pa(__kfence_pool));
+ return true;
+free:
/*
* Only release unprotected pages, and do not try to go back and change
* page attributes due to risk of failing to do so as well. If changing
@@ -607,27 +591,7 @@ static bool __init kfence_init_pool_early(void)
* fails for the first page, and therefore expect addr==__kfence_pool in
* most failure cases.
*/
- memblock_free_late(__pa(addr), KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool));
- __kfence_pool = NULL;
- return false;
-}
-
-static bool kfence_init_pool_late(void)
-{
- unsigned long addr, free_size;
-
- addr = kfence_init_pool();
-
- if (!addr)
- return true;
-
- /* Same as above. */
- free_size = KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool);
-#ifdef CONFIG_CONTIG_ALLOC
- free_contig_range(page_to_pfn(virt_to_page((void *)addr)), free_size / PAGE_SIZE);
-#else
- free_pages_exact((void *)addr, free_size);
-#endif
+ memblock_free_late(__pa(__kfence_pool), KFENCE_POOL_SIZE);
__kfence_pool = NULL;
return false;
}
@@ -830,30 +794,50 @@ void __init kfence_init(void)
kfence_init_enable();
}

-static int kfence_init_late(void)
-{
- const unsigned long nr_pages = KFENCE_POOL_SIZE / PAGE_SIZE;
#ifdef CONFIG_CONTIG_ALLOC
- struct page *pages;
+static inline void *kfence_pool_alloc(void)
+{
+ struct page *page = alloc_contig_pages(KFENCE_POOL_SIZE / PAGE_SIZE,
+ GFP_KERNEL, first_online_node, NULL);

- pages = alloc_contig_pages(nr_pages, GFP_KERNEL, first_online_node, NULL);
- if (!pages)
- return -ENOMEM;
- __kfence_pool = page_to_virt(pages);
+ return page ? page_to_virt(page) : NULL;
+}
+
+static inline void kfence_pool_free(const void *ptr)
+{
+ free_contig_range(page_to_pfn(virt_to_page(ptr)), KFENCE_POOL_SIZE / PAGE_SIZE);
+}
#else
+static inline void *kfence_pool_alloc(void)
+{
BUILD_BUG_ON_MSG(get_order(KFENCE_POOL_SIZE) > MAX_ORDER,

"CONFIG_KFENCE_NUM_OBJECTS is too large for buddy allocator");

- __kfence_pool = alloc_pages_exact(KFENCE_POOL_SIZE, GFP_KERNEL);
+ return alloc_pages_exact(KFENCE_POOL_SIZE, GFP_KERNEL);
+}
+
+static inline void kfence_pool_free(const void *ptr)
+{
+ free_pages_exact(virt_to_page(ptr), KFENCE_POOL_SIZE);
+}
+#endif
+
+static int kfence_init_late(void)
+{
+ if (__kfence_pool)
+ return 0;
+
+ __kfence_pool = kfence_pool_alloc();
if (!__kfence_pool)
return -ENOMEM;
-#endif

- if (!kfence_init_pool_late()) {
- pr_err("%s failed\n", __func__);
+ if (!arch_kfence_init_pool()) {
+ kfence_pool_free(__kfence_pool);
+ __kfence_pool = NULL;
return -EBUSY;
}

+ kfence_init_pool();
kfence_init_enable();
kfence_debugfs_init();

@@ -862,8 +846,8 @@ static int kfence_init_late(void)

static int kfence_enable_late(void)
{
- if (!__kfence_pool)
- return kfence_init_late();
+ if (kfence_init_late())
+ return -ENOMEM;

WRITE_ONCE(kfence_enabled, true);
queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
@@ -1054,8 +1038,9 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
if (!is_kfence_address((void *)addr))
return false;

- if (!READ_ONCE(kfence_enabled)) /* If disabled at runtime ... */
- return kfence_unprotect(addr); /* ... unprotect and proceed. */
+ /* If disabled at runtime ... unprotect and proceed. */
+ if (!READ_ONCE(kfence_enabled))
+ goto out;

atomic_long_inc(&counters[KFENCE_COUNTER_BUGS]);

@@ -1079,7 +1064,7 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
}

if (!to_report)
- goto out;
+ goto report;

raw_spin_lock_irqsave(&to_report->lock, flags);
to_report->unprotected_page = addr;
@@ -1093,7 +1078,7 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
} else {
to_report = addr_to_metadata(addr);
if (!to_report)
- goto out;
+ goto report;

raw_spin_lock_irqsave(&to_report->lock, flags);
error_type = KFENCE_ERROR_UAF;
@@ -1105,7 +1090,7 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
*/
}

-out:
+report:
if (to_report) {
kfence_report_error(addr, is_write, regs, to_report, error_type);
raw_spin_unlock_irqrestore(&to_report->lock, flags);
@@ -1113,6 +1098,7 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs
/* This may be a UAF or OOB access, but we can't be sure. */
kfence_report_error(addr, is_write, regs, NULL, KFENCE_ERROR_INVALID);
}
-
- return kfence_unprotect(addr); /* Unprotect and let access proceed. */
+out:
+ kfence_unprotect(addr);
+ return true;
}
--
2.11.0

[Muchun Song]

The CONFIG_KFENCE_NUM_OBJECTS is limited by kconfig and vary from 1 to
65535, so CONFIG_KFENCE_NUM_OBJECTS cannot be equabl to or smaller than
0. Removing it to simplify code.

Signed-off-by: Muchun Song <songm...@bytedance.com>
---

mm/kfence/core.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 5726bf2ae13c..41befcb3b069 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -115,7 +115,6 @@ EXPORT_SYMBOL(__kfence_pool); /* Export for test modules. */
* Per-object metadata, with one-to-one mapping of object metadata to
* backing pages (in __kfence_pool).
*/
-static_assert(CONFIG_KFENCE_NUM_OBJECTS > 0);
struct kfence_metadata kfence_metadata[CONFIG_KFENCE_NUM_OBJECTS];

/* Freelist with available objects. */
--
2.11.0

[Muchun Song]

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 <songm...@bytedance.com>
---

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. */
+#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;
-
- /*
- * 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;

diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
index b5d66a69200d..d479f9c8afb1 100644
--- a/mm/kfence/kfence_test.c
+++ b/mm/kfence/kfence_test.c
@@ -637,19 +637,6 @@ static void test_gfpzero(struct kunit *test)
KUNIT_EXPECT_FALSE(test, report_available());
}

-static void test_invalid_access(struct kunit *test)
-{
- const struct expect_report expect = {
- .type = KFENCE_ERROR_INVALID,
- .fn = test_invalid_access,
- .addr = &__kfence_pool[10],
- .is_write = false,
- };
-
- READ_ONCE(__kfence_pool[10]);
- KUNIT_EXPECT_TRUE(test, report_matches(&expect));
-}
-
/* Test SLAB_TYPESAFE_BY_RCU works. */
static void test_memcache_typesafe_by_rcu(struct kunit *test)
{
@@ -787,7 +774,6 @@ static struct kunit_case kfence_test_cases[] = {
KUNIT_CASE(test_kmalloc_aligned_oob_write),
KUNIT_CASE(test_shrink_memcache),
KUNIT_CASE(test_memcache_ctor),
- KUNIT_CASE(test_invalid_access),
KUNIT_CASE(test_gfpzero),
KUNIT_CASE(test_memcache_typesafe_by_rcu),
KUNIT_CASE(test_krealloc),
--
2.11.0

[Muchun Song]

Replace ALIGN_DOWN(x, PAGE_SIZE) with PAGE_ALIGN_DOWN(x) to simplify
the code a bit.

Signed-off-by: Muchun Song <songm...@bytedance.com>
---

mm/kfence/core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index f205b860f460..dbfb79a4d624 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -230,17 +230,17 @@ static bool alloc_covered_contains(u32 alloc_stack_hash)

static inline void kfence_protect(unsigned long addr)
{

- kfence_protect_page(ALIGN_DOWN(addr, PAGE_SIZE), true);
+ kfence_protect_page(PAGE_ALIGN_DOWN(addr), true);

}

static inline void kfence_unprotect(unsigned long addr)

{
- kfence_protect_page(ALIGN_DOWN(addr, PAGE_SIZE), false);
+ kfence_protect_page(PAGE_ALIGN_DOWN(addr), false);

}

static inline unsigned long metadata_to_pageaddr(const struct kfence_metadata *meta)
{

- return ALIGN_DOWN(meta->addr, PAGE_SIZE);
+ return PAGE_ALIGN_DOWN(meta->addr);
}

/*
@@ -308,7 +308,7 @@ static inline bool check_canary_byte(u8 *addr)
/* __always_inline this to ensure we won't do an indirect call to fn. */
static __always_inline void for_each_canary(const struct kfence_metadata *meta, bool (*fn)(u8 *))
{
- const unsigned long pageaddr = ALIGN_DOWN(meta->addr, PAGE_SIZE);
+ const unsigned long pageaddr = PAGE_ALIGN_DOWN(meta->addr);
unsigned long addr;

/*
@@ -455,7 +455,7 @@ static void kfence_guarded_free(void *addr, struct kfence_metadata *meta, bool z
}

/* Detect racy use-after-free, or incorrect reallocation of this page by KFENCE. */
- kcsan_begin_scoped_access((void *)ALIGN_DOWN((unsigned long)addr, PAGE_SIZE), PAGE_SIZE,
+ kcsan_begin_scoped_access((void *)PAGE_ALIGN_DOWN((unsigned long)addr), PAGE_SIZE,
KCSAN_ACCESS_SCOPED | KCSAN_ACCESS_WRITE | KCSAN_ACCESS_ASSERT,
&assert_page_exclusive);

@@ -464,7 +464,7 @@ static void kfence_guarded_free(void *addr, struct kfence_metadata *meta, bool z

/* Restore page protection if there was an OOB access. */
if (meta->unprotected_page) {
- memzero_explicit((void *)ALIGN_DOWN(meta->unprotected_page, PAGE_SIZE), PAGE_SIZE);
+ memzero_explicit((void *)PAGE_ALIGN_DOWN(meta->unprotected_page), PAGE_SIZE);
kfence_protect(meta->unprotected_page);
meta->unprotected_page = 0;
}
--
2.11.0

[Marco Elver]

On Tue, 28 Mar 2023 at 11:58, 'Muchun Song' via kasan-dev
<kasa...@googlegroups.com> wrote:
>
> Check kfence pool size at building time to expose problem ASAP.
>
> Signed-off-by: Muchun Song <songm...@bytedance.com>
> ---
> mm/kfence/core.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index de62a84d4830..6781af1dfa66 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -841,10 +841,9 @@ static int kfence_init_late(void)
> return -ENOMEM;
> __kfence_pool = page_to_virt(pages);
> #else
> - if (nr_pages > MAX_ORDER_NR_PAGES) {
> - pr_warn("KFENCE_NUM_OBJECTS too large for buddy allocator\n");
> - return -EINVAL;
> - }
> + BUILD_BUG_ON_MSG(get_order(KFENCE_POOL_SIZE) > MAX_ORDER,
> + "CONFIG_KFENCE_NUM_OBJECTS is too large for buddy allocator");
> +

It's perfectly valid to want to use KFENCE with a very large pool that
is initialized on boot, and simply sacrifice the ability to initialize
late.

Nack.

[Marco Elver]

This WARN and bailing here has helped us catch an issue early before
[1] - and because KFENCE ought to be enabled as a debugging tool, the
philosophy is to be failure tolerant and not crash the system here,
hence the "return false".

[1] https://lore.kernel.org/lkml/Y3bCV6Vc...@elver.google.com/

We're relying on the architecture doing the "right thing", but it's
not entirely unlikely that the arch ends up doing the wrong thing due
to some bug like above (i.e. arch_kfence_init_pool() is faulty).

Nack.

[Marco Elver]

On Tue, 28 Mar 2023 at 11:58, Muchun Song <songm...@bytedance.com> wrote:
>

> There are three similar loops to initialize kfence pool, we could merge
> all of them into one loop to simplify the code and make code more
> efficient.
>
> Signed-off-by: Muchun Song <songm...@bytedance.com>

Reviewed-by: Marco Elver <el...@google.com>

[Marco Elver]

On Tue, 28 Mar 2023 at 11:59, 'Muchun Song' via kasan-dev
<kasa...@googlegroups.com> wrote:
>
> Replace ALIGN_DOWN(x, PAGE_SIZE) with PAGE_ALIGN_DOWN(x) to simplify
> the code a bit.
>
> Signed-off-by: Muchun Song <songm...@bytedance.com>

Reviewed-by: Marco Elver <el...@google.com>

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20230328095807.7014-7-songmuchun%40bytedance.com.

[Marco Elver]

Actually: can you retain this comment somewhere?

[Muchun Song]

Sure, I'll move this to right place.

Thanks.

[Marco Elver]

The last page is just a normal guard page? I.e. the last 2 pages are:
<object page> | <guard 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.

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.

The test can be retained by doing an access to a guard page in between
2 unallocated objects. But it's probably not that easy to reliably set
that up (could try to allocate 2 objects and see if they're next to
each other, then free them).

[Muchun Song]

You are right. I didn’t realize this.

Thanks

>
> Nack.

[Muchun Song]

A good example.

>
> We're relying on the architecture doing the "right thing", but it's
> not entirely unlikely that the arch ends up doing the wrong thing due
> to some bug like above (i.e. arch_kfence_init_pool() is faulty).

Got it. I’ll drop this one next version.

Thanks

>
> Nack.

[Muchun Song]

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.

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, it's not easy to trigger it 100%. So I removed the test.

[Marco Elver]

Yes, you're right.

Thanks,
-- Marco

[Alexander Potapenko]

On Tue, Mar 28, 2023 at 11:58 AM Muchun Song <songm...@bytedance.com> wrote:
>
> The CONFIG_KFENCE_NUM_OBJECTS is limited by kconfig and vary from 1 to
> 65535, so CONFIG_KFENCE_NUM_OBJECTS cannot be equabl to or smaller than

Nit: "equal"

> 0. Removing it to simplify code.
>
> Signed-off-by: Muchun Song <songm...@bytedance.com>

Reviewed-by: Alexander Potapenko <gli...@google.com>