Messages in this thread | | | Subject | Re: [RFC][PATCH v3 1/2] mm/page_poison.c: Enable PAGE_POISONING as a separate option | From | Laura Abbott <> | Date | Fri, 26 Feb 2016 14:21:03 -0800 |
| |
On 02/25/2016 09:34 PM, Jianyu Zhan wrote: > On Fri, Feb 26, 2016 at 12:45 PM, Laura Abbott <labbott@redhat.com> wrote: >> Do you have some suggestion on wording here? I'm not sure what else to >> say besides poison patterns to differentiate from hardware poison. >> > > > Is the below wording OK? > > > config PAGE_POISONING > bool > bool "Poison pages after freeing" > select PAGE_EXTENSION > select PAGE_POISONING_NO_SANITY if HIBERNATION > ---help--- > Fill the pages with poison patterns after free_pages() and verify > the patterns before alloc_pages. The filling of the memory helps > reduce the risk of information leaks from freed data. This does > have a potential performance impact. > > Note that "poison" here is not the same thing as that in "HWPoison" > for CONFIG_MEMORY_FAILURE, in which "poison" is just a nomenclature > borrowed from Intel , for the processor support for > "poisoned" memory, an > adaptive method for flagging and recovering from memory errors >
Okay, I see what you are getting at here. This sounds okay.
>> >>>> >>>> +config PAGE_POISONING_NO_SANITY >>>> + depends on PAGE_POISONING >>>> + bool "Only poison, don't sanity check" >>>> + ---help--- >>>> + Skip the sanity checking on alloc, only fill the pages with >>>> + poison on free. This reduces some of the overhead of the >>>> + poisoning feature. >>>> + >>>> + If you are only interested in sanitization, say Y. Otherwise >>>> + say N. >>>> diff --git a/mm/Makefile b/mm/Makefile >>>> index fb1a7948c107..ec59c071b4f9 100644 >>>> --- a/mm/Makefile >>>> +++ b/mm/Makefile >>>> @@ -13,7 +13,6 @@ KCOV_INSTRUMENT_slob.o := n >>>> KCOV_INSTRUMENT_slab.o := n >>>> KCOV_INSTRUMENT_slub.o := n >>>> KCOV_INSTRUMENT_page_alloc.o := n >>>> -KCOV_INSTRUMENT_debug-pagealloc.o := n >>>> KCOV_INSTRUMENT_kmemleak.o := n >>>> KCOV_INSTRUMENT_kmemcheck.o := n >>>> KCOV_INSTRUMENT_memcontrol.o := n >>>> @@ -63,9 +62,6 @@ obj-$(CONFIG_SPARSEMEM_VMEMMAP) += sparse-vmemmap.o >>>> obj-$(CONFIG_SLOB) += slob.o >>>> obj-$(CONFIG_MMU_NOTIFIER) += mmu_notifier.o >>>> obj-$(CONFIG_KSM) += ksm.o >>>> -ifndef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC >>>> - obj-$(CONFIG_DEBUG_PAGEALLOC) += debug-pagealloc.o >>>> -endif >>>> obj-$(CONFIG_PAGE_POISONING) += page_poison.o >>>> obj-$(CONFIG_SLAB) += slab.o >>>> obj-$(CONFIG_SLUB) += slub.o >>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>> index a34c359d8e81..0bdb3cfd83b5 100644 >>>> --- a/mm/page_alloc.c >>>> +++ b/mm/page_alloc.c >>>> @@ -1026,6 +1026,7 @@ static bool free_pages_prepare(struct page *page, >>>> unsigned int order) >>>> PAGE_SIZE << order); >>>> } >>>> arch_free_page(page, order); >>>> + kernel_poison_pages(page, 1 << order, 0); >>>> kernel_map_pages(page, 1 << order, 0); >>>> >>>> return true; >>>> @@ -1497,6 +1498,7 @@ static int prep_new_page(struct page *page, >>>> unsigned int order, gfp_t gfp_flags, >>>> >>>> arch_alloc_page(page, order); >>>> kernel_map_pages(page, 1 << order, 1); >>>> + kernel_poison_pages(page, 1 << order, 1); >>>> kasan_alloc_pages(page, order); >>>> >>>> if (gfp_flags & __GFP_ZERO) >>>> diff --git a/mm/page_poison.c b/mm/page_poison.c >>>> index 92ead727b8f0..884a6f854432 100644 >>>> --- a/mm/page_poison.c >>>> +++ b/mm/page_poison.c >>>> @@ -80,7 +80,7 @@ static void poison_page(struct page *page) >>>> kunmap_atomic(addr); >>>> } >>>> >>>> -void poison_pages(struct page *page, int n) >>>> +static void poison_pages(struct page *page, int n) >>>> { >>>> int i; >>>> >>>> @@ -101,6 +101,9 @@ static void check_poison_mem(unsigned char *mem, >>>> size_t bytes) >>>> unsigned char *start; >>>> unsigned char *end; >>>> >>>> + if (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY)) >>>> + return; >>>> + >>>> start = memchr_inv(mem, PAGE_POISON, bytes); >>>> if (!start) >>>> return; >>>> @@ -113,9 +116,9 @@ static void check_poison_mem(unsigned char *mem, >>>> size_t bytes) >>>> if (!__ratelimit(&ratelimit)) >>>> return; >>>> else if (start == end && single_bit_flip(*start, PAGE_POISON)) >>>> - printk(KERN_ERR "pagealloc: single bit error\n"); >>>> + pr_err("pagealloc: single bit error\n"); >>>> else >>>> - printk(KERN_ERR "pagealloc: memory corruption\n"); >>>> + pr_err("pagealloc: memory corruption\n"); >>>> >>>> print_hex_dump(KERN_ERR, "", DUMP_PREFIX_ADDRESS, 16, 1, start, >>>> end - start + 1, 1); >>>> @@ -135,10 +138,28 @@ static void unpoison_page(struct page *page) >>>> kunmap_atomic(addr); >>>> } >>>> >>>> -void unpoison_pages(struct page *page, int n) >>>> +static void unpoison_pages(struct page *page, int n) >>>> { >>>> int i; >>>> >>>> for (i = 0; i < n; i++) >>>> unpoison_page(page + i); >>>> } >>>> + >>>> +void kernel_poison_pages(struct page *page, int numpages, int enable) >>>> +{ >>>> + if (!page_poisoning_enabled()) >>>> + return; >>>> + >>>> + if (enable) >>>> + unpoison_pages(page, numpages); >>>> + else >>>> + poison_pages(page, numpages); >>>> +} >>>> + >>>> +#ifndef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC >>>> +void __kernel_map_pages(struct page *page, int numpages, int enable) >>>> +{ >>>> + /* This function does nothing, all work is done via poison pages >>>> */ >>>> +} >>>> +#endif >>> >>> >>> IMHO, kernel_map_pages is originally incorporated for debugging page >>> allocation. >>> And latter for archs that do not support arch-specific page poisoning, >>> a software poisoning >>> method was used. >>> >>> So I think it is not appropriate to use two interfaces in the alloc/free >>> hooks. >>> >>> The kernel_poison_pages actually should be an implementation detail >>> and should be hided >>> in the kernel_map_pages interface. >>> >> >> We want to have the poisoning independent of anything that kernel_map_pages >> does. It was originally added for software poisoning for arches that >> didn't have the full ARCH_SUPPORTS_DEBUG_PAGEALLOC support but there's >> nothing that specifically ties it to mapping. It's beneficial even when >> we aren't mapping/unmapping the pages so putting it in kernel_map_pages >> would defeat what we're trying to accomplish here. >> > > Ok, fair enough. If so, I suggest you add this clarification into the > code, or as least, in > the changelog.
Sounds fine.
> > > Thanks, > Jianyu Zhan >
Thanks, Laura
| |