On Fri, Feb 26, 2016 at 12:45 PM, Laura Abbott <labb...@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
>
>>>
>>> +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.
Thanks,
Jianyu Zhan
