diff mbox series

[5/6] mm: kfence: change kfence pool page layout

Message ID 20230328095807.7014-6-songmuchun@bytedance.com
State New
Headers
Series Simplify kfence code |

Commit Message

Muchun Song March 28, 2023, 9:58 a.m. UTC 
  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 <songmuchun@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(-)

Comments

Marco Elver March 28, 2023, 12:59 p.m. UTC | #1 
On Tue, 28 Mar 2023 at 11:58, 'Muchun Song' via kasan-dev
<kasan-dev@googlegroups.com> 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 <songmuchun@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. */

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.

> -       /*
> -        * 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.

> 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),

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 March 28, 2023, 1:32 p.m. UTC | #2 
> On Mar 28, 2023, at 20:59, Marco Elver <elver@google.com> wrote:
> 
> On Tue, 28 Mar 2023 at 11:58, 'Muchun Song' via kasan-dev
> <kasan-dev@googlegroups.com> 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 <songmuchun@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. */
> 
> The last page is just a normal guard page? I.e. the last 2 pages are:
> <object page> | <guard page>

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?

> 
>> 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),
> 
> 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).

Yes, it's not easy to trigger it 100%. So I removed the test.
Marco Elver March 28, 2023, 2:12 p.m. UTC | #3 
On Tue, 28 Mar 2023 at 15:33, Muchun Song <muchun.song@linux.dev> wrote:
>
>
>
> > On Mar 28, 2023, at 20:59, Marco Elver <elver@google.com> wrote:
> >
> > On Tue, 28 Mar 2023 at 11:58, 'Muchun Song' via kasan-dev
> > <kasan-dev@googlegroups.com> 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 <songmuchun@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. */
> >
> > The last page is just a normal guard page? I.e. the last 2 pages are:
> > <object page> | <guard page>
>
> 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
diff mbox series

Patch

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),