Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1730189 > unrolled thread

Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame Ownership (XPFO)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Yisheng Xie <xieyisheng1@huawei.com> - 2017-09-11 09:30 +0200
    Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Tycho Andersen <tycho@docker.com> - 2017-09-11 17:00 +0200
      Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Juerg Haefliger <juerg.haefliger@canonical.com> - 2017-09-11 18:10 +0200
        Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Tycho Andersen <tycho@docker.com> - 2017-09-11 19:00 +0200
        Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Yisheng Xie <xieyisheng1@huawei.com> - 2017-09-12 10:10 +0200
          Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Tycho Andersen <tycho@docker.com> - 2017-09-12 16:40 +0200
            Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Tycho Andersen <tycho@docker.com> - 2017-09-12 20:20 +0200
              Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Yisheng Xie <xieyisheng1@huawei.com> - 2017-09-14 08:20 +0200
              Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Dave Hansen <dave.hansen@intel.com> - 2017-09-21 01:50 +0200
                Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Dave Hansen <dave.hansen@intel.com> - 2017-09-21 02:10 +0200
                Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame  Ownership (XPFO) Tycho Andersen <tycho@docker.com> - 2017-09-21 02:10 +0200

#1730189 — Re: [PATCH v6 03/11] mm, x86: Add support for eXclusive Page Frame Ownership (XPFO)

Hi Tycho,

On 2017/9/8 1:36, Tycho Andersen wrote:
> From: Juerg Haefliger <juerg.haefliger@canonical.com>
> 
> This patch adds support for XPFO which protects against 'ret2dir' kernel
> attacks. The basic idea is to enforce exclusive ownership of page frames
> by either the kernel or userspace, unless explicitly requested by the
> kernel. Whenever a page destined for userspace is allocated, it is
> unmapped from physmap (the kernel's page table). When such a page is
> reclaimed from userspace, it is mapped back to physmap.
> 
> Additional fields in the page_ext struct are used for XPFO housekeeping,
> specifically:
>   - two flags to distinguish user vs. kernel pages and to tag unmapped
>     pages.
>   - a reference counter to balance kmap/kunmap operations.
>   - a lock to serialize access to the XPFO fields.
> 
> This patch is based on the work of Vasileios P. Kemerlis et al. who
> published their work in this paper:
>   http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf
> 
> [...]
> +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
> +{
> +	int i, flush_tlb = 0;
> +	struct xpfo *xpfo;
> +
> +	if (!static_branch_unlikely(&xpfo_inited))
> +		return;
> +
> +	for (i = 0; i < (1 << order); i++)  {
> +		xpfo = lookup_xpfo(page + i);
> +		if (!xpfo)
> +			continue;
> +
> +		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
> +		     "xpfo: unmapped page being allocated\n");
> +
> +		/* Initialize the map lock and map counter */
> +		if (unlikely(!xpfo->inited)) {
> +			spin_lock_init(&xpfo->maplock);
> +			atomic_set(&xpfo->mapcount, 0);
> +			xpfo->inited = true;
> +		}
> +		WARN(atomic_read(&xpfo->mapcount),
> +		     "xpfo: already mapped page being allocated\n");
> +
> +		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
> +			/*
> +			 * Tag the page as a user page and flush the TLB if it
> +			 * was previously allocated to the kernel.
> +			 */
> +			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
> +				flush_tlb = 1;

I'm not sure whether I am miss anything, however, when the page was previously allocated
to kernel,  should we unmap the physmap (the kernel's page table) here? For we allocate
the page to user now

Yisheng Xie
Thanks

[toc] | [next] | [standalone]

#1730379

Hi Yisheng,

On Mon, Sep 11, 2017 at 03:24:09PM +0800, Yisheng Xie wrote:
> > +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
> > +{
> > +	int i, flush_tlb = 0;
> > +	struct xpfo *xpfo;
> > +
> > +	if (!static_branch_unlikely(&xpfo_inited))
> > +		return;
> > +
> > +	for (i = 0; i < (1 << order); i++)  {
> > +		xpfo = lookup_xpfo(page + i);
> > +		if (!xpfo)
> > +			continue;
> > +
> > +		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
> > +		     "xpfo: unmapped page being allocated\n");
> > +
> > +		/* Initialize the map lock and map counter */
> > +		if (unlikely(!xpfo->inited)) {
> > +			spin_lock_init(&xpfo->maplock);
> > +			atomic_set(&xpfo->mapcount, 0);
> > +			xpfo->inited = true;
> > +		}
> > +		WARN(atomic_read(&xpfo->mapcount),
> > +		     "xpfo: already mapped page being allocated\n");
> > +
> > +		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
> > +			/*
> > +			 * Tag the page as a user page and flush the TLB if it
> > +			 * was previously allocated to the kernel.
> > +			 */
> > +			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
> > +				flush_tlb = 1;
> 
> I'm not sure whether I am miss anything, however, when the page was previously allocated
> to kernel,  should we unmap the physmap (the kernel's page table) here? For we allocate
> the page to user now

Yes, I think you're right. Oddly, the XPFO_READ_USER test works
correctly for me, but I think (?) should not because of this bug...

Tycho

[toc] | [prev] | [next] | [standalone]

#1730418

On 09/11/2017 04:50 PM, Tycho Andersen wrote:
> Hi Yisheng,
> 
> On Mon, Sep 11, 2017 at 03:24:09PM +0800, Yisheng Xie wrote:
>>> +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
>>> +{
>>> +	int i, flush_tlb = 0;
>>> +	struct xpfo *xpfo;
>>> +
>>> +	if (!static_branch_unlikely(&xpfo_inited))
>>> +		return;
>>> +
>>> +	for (i = 0; i < (1 << order); i++)  {
>>> +		xpfo = lookup_xpfo(page + i);
>>> +		if (!xpfo)
>>> +			continue;
>>> +
>>> +		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
>>> +		     "xpfo: unmapped page being allocated\n");
>>> +
>>> +		/* Initialize the map lock and map counter */
>>> +		if (unlikely(!xpfo->inited)) {
>>> +			spin_lock_init(&xpfo->maplock);
>>> +			atomic_set(&xpfo->mapcount, 0);
>>> +			xpfo->inited = true;
>>> +		}
>>> +		WARN(atomic_read(&xpfo->mapcount),
>>> +		     "xpfo: already mapped page being allocated\n");
>>> +
>>> +		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
>>> +			/*
>>> +			 * Tag the page as a user page and flush the TLB if it
>>> +			 * was previously allocated to the kernel.
>>> +			 */
>>> +			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
>>> +				flush_tlb = 1;
>>
>> I'm not sure whether I am miss anything, however, when the page was previously allocated
>> to kernel,  should we unmap the physmap (the kernel's page table) here? For we allocate
>> the page to user now
>> 
> Yes, I think you're right. Oddly, the XPFO_READ_USER test works
> correctly for me, but I think (?) should not because of this bug...

IIRC, this is an optimization carried forward from the initial
implementation. The assumption is that the kernel will map the user
buffer so it's not unmapped on allocation but only on the first (and
subsequent) call of kunmap. I.e.:
 - alloc  -> noop
 - kmap   -> noop
 - kunmap -> unmapped from the kernel
 - kmap   -> mapped into the kernel
 - kunmap -> unmapped from the kernel
and so on until:
 - free   -> mapped back into the kernel

I'm not sure if that make sense though since it leaves a window.

...Juerg



> Tycho
> 

[toc] | [prev] | [next] | [standalone]

#1730450

On Mon, Sep 11, 2017 at 06:03:55PM +0200, Juerg Haefliger wrote:
> 
> 
> On 09/11/2017 04:50 PM, Tycho Andersen wrote:
> > Hi Yisheng,
> > 
> > On Mon, Sep 11, 2017 at 03:24:09PM +0800, Yisheng Xie wrote:
> >>> +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
> >>> +{
> >>> +	int i, flush_tlb = 0;
> >>> +	struct xpfo *xpfo;
> >>> +
> >>> +	if (!static_branch_unlikely(&xpfo_inited))
> >>> +		return;
> >>> +
> >>> +	for (i = 0; i < (1 << order); i++)  {
> >>> +		xpfo = lookup_xpfo(page + i);
> >>> +		if (!xpfo)
> >>> +			continue;
> >>> +
> >>> +		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
> >>> +		     "xpfo: unmapped page being allocated\n");
> >>> +
> >>> +		/* Initialize the map lock and map counter */
> >>> +		if (unlikely(!xpfo->inited)) {
> >>> +			spin_lock_init(&xpfo->maplock);
> >>> +			atomic_set(&xpfo->mapcount, 0);
> >>> +			xpfo->inited = true;
> >>> +		}
> >>> +		WARN(atomic_read(&xpfo->mapcount),
> >>> +		     "xpfo: already mapped page being allocated\n");
> >>> +
> >>> +		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
> >>> +			/*
> >>> +			 * Tag the page as a user page and flush the TLB if it
> >>> +			 * was previously allocated to the kernel.
> >>> +			 */
> >>> +			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
> >>> +				flush_tlb = 1;
> >>
> >> I'm not sure whether I am miss anything, however, when the page was previously allocated
> >> to kernel,  should we unmap the physmap (the kernel's page table) here? For we allocate
> >> the page to user now
> >> 
> > Yes, I think you're right. Oddly, the XPFO_READ_USER test works
> > correctly for me, but I think (?) should not because of this bug...
> 
> IIRC, this is an optimization carried forward from the initial
> implementation. The assumption is that the kernel will map the user
> buffer so it's not unmapped on allocation but only on the first (and

Does the kernel always map it, though? e.g. in the case of
XPFO_READ_USER, I'm not sure where the kernel would do a kmap() of the
test's user buffer.

Tycho

> subsequent) call of kunmap. I.e.:
>  - alloc  -> noop
>  - kmap   -> noop
>  - kunmap -> unmapped from the kernel
>  - kmap   -> mapped into the kernel
>  - kunmap -> unmapped from the kernel
> and so on until:
>  - free   -> mapped back into the kernel
> 
> I'm not sure if that make sense though since it leaves a window.
> 
> ...Juerg
> 
> 
> 
> > Tycho
> > 

[toc] | [prev] | [next] | [standalone]

#1730727

On 2017/9/12 0:03, Juerg Haefliger wrote:
> 
> 
> On 09/11/2017 04:50 PM, Tycho Andersen wrote:
>> Hi Yisheng,
>>
>> On Mon, Sep 11, 2017 at 03:24:09PM +0800, Yisheng Xie wrote:
>>>> +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
>>>> +{
>>>> +	int i, flush_tlb = 0;
>>>> +	struct xpfo *xpfo;
>>>> +
>>>> +	if (!static_branch_unlikely(&xpfo_inited))
>>>> +		return;
>>>> +
>>>> +	for (i = 0; i < (1 << order); i++)  {
>>>> +		xpfo = lookup_xpfo(page + i);
>>>> +		if (!xpfo)
>>>> +			continue;
>>>> +
>>>> +		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
>>>> +		     "xpfo: unmapped page being allocated\n");
>>>> +
>>>> +		/* Initialize the map lock and map counter */
>>>> +		if (unlikely(!xpfo->inited)) {
>>>> +			spin_lock_init(&xpfo->maplock);
>>>> +			atomic_set(&xpfo->mapcount, 0);
>>>> +			xpfo->inited = true;
>>>> +		}
>>>> +		WARN(atomic_read(&xpfo->mapcount),
>>>> +		     "xpfo: already mapped page being allocated\n");
>>>> +
>>>> +		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
>>>> +			/*
>>>> +			 * Tag the page as a user page and flush the TLB if it
>>>> +			 * was previously allocated to the kernel.
>>>> +			 */
>>>> +			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
>>>> +				flush_tlb = 1;
>>>
>>> I'm not sure whether I am miss anything, however, when the page was previously allocated
>>> to kernel,  should we unmap the physmap (the kernel's page table) here? For we allocate
>>> the page to user now
>>>
>> Yes, I think you're right. Oddly, the XPFO_READ_USER test works

Hi Tycho,
Could you share this test? I'd like to know how it works.

Thanks

>> correctly for me, but I think (?) should not because of this bug...
> 
> IIRC, this is an optimization carried forward from the initial
> implementation. 
Hi Juerg,

hmm.. If below is the first version, then it seems this exist from the first version:
https://patchwork.kernel.org/patch/8437451/

> The assumption is that the kernel will map the user
> buffer so it's not unmapped on allocation but only on the first (and
> subsequent) call of kunmap.

IMO, before a page is allocated, it is in buddy system, which means it is free
and no other 'map' on the page except direct map. Then if the page is allocated
to user, XPFO should unmap the direct map. otherwise the ret2dir may works at
this window before it is freed. Or maybe I'm still missing anything.

Thanks
Yisheng Xie

>  I.e.:
>  - alloc  -> noop
>  - kmap   -> noop
>  - kunmap -> unmapped from the kernel
>  - kmap   -> mapped into the kernel
>  - kunmap -> unmapped from the kernel
> and so on until:
>  - free   -> mapped back into the kernel
> 
> I'm not sure if that make sense though since it leaves a window.
> 
> ...Juerg
> 
> 
> 
>> Tycho
>>
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1730890

On Tue, Sep 12, 2017 at 04:05:22PM +0800, Yisheng Xie wrote:
> 
> 
> On 2017/9/12 0:03, Juerg Haefliger wrote:
> > 
> > 
> > On 09/11/2017 04:50 PM, Tycho Andersen wrote:
> >> Hi Yisheng,
> >>
> >> On Mon, Sep 11, 2017 at 03:24:09PM +0800, Yisheng Xie wrote:
> >>>> +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
> >>>> +{
> >>>> +	int i, flush_tlb = 0;
> >>>> +	struct xpfo *xpfo;
> >>>> +
> >>>> +	if (!static_branch_unlikely(&xpfo_inited))
> >>>> +		return;
> >>>> +
> >>>> +	for (i = 0; i < (1 << order); i++)  {
> >>>> +		xpfo = lookup_xpfo(page + i);
> >>>> +		if (!xpfo)
> >>>> +			continue;
> >>>> +
> >>>> +		WARN(test_bit(XPFO_PAGE_UNMAPPED, &xpfo->flags),
> >>>> +		     "xpfo: unmapped page being allocated\n");
> >>>> +
> >>>> +		/* Initialize the map lock and map counter */
> >>>> +		if (unlikely(!xpfo->inited)) {
> >>>> +			spin_lock_init(&xpfo->maplock);
> >>>> +			atomic_set(&xpfo->mapcount, 0);
> >>>> +			xpfo->inited = true;
> >>>> +		}
> >>>> +		WARN(atomic_read(&xpfo->mapcount),
> >>>> +		     "xpfo: already mapped page being allocated\n");
> >>>> +
> >>>> +		if ((gfp & GFP_HIGHUSER) == GFP_HIGHUSER) {
> >>>> +			/*
> >>>> +			 * Tag the page as a user page and flush the TLB if it
> >>>> +			 * was previously allocated to the kernel.
> >>>> +			 */
> >>>> +			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
> >>>> +				flush_tlb = 1;
> >>>
> >>> I'm not sure whether I am miss anything, however, when the page was previously allocated
> >>> to kernel,  should we unmap the physmap (the kernel's page table) here? For we allocate
> >>> the page to user now
> >>>
> >> Yes, I think you're right. Oddly, the XPFO_READ_USER test works
> 
> Hi Tycho,
> Could you share this test? I'd like to know how it works.

See the last patch in the series.

> >> correctly for me, but I think (?) should not because of this bug...
> > 
> > IIRC, this is an optimization carried forward from the initial
> > implementation. 
> Hi Juerg,
> 
> hmm.. If below is the first version, then it seems this exist from the first version:
> https://patchwork.kernel.org/patch/8437451/
> 
> > The assumption is that the kernel will map the user
> > buffer so it's not unmapped on allocation but only on the first (and
> > subsequent) call of kunmap.
> 
> IMO, before a page is allocated, it is in buddy system, which means it is free
> and no other 'map' on the page except direct map. Then if the page is allocated
> to user, XPFO should unmap the direct map. otherwise the ret2dir may works at
> this window before it is freed. Or maybe I'm still missing anything.

I agree that it seems broken. I'm just not sure why the test doesn't
fail. It's certainly worth understanding.

Tycho

[toc] | [prev] | [next] | [standalone]

#1731137

Hi Yisheng,

> On Tue, Sep 12, 2017 at 04:05:22PM +0800, Yisheng Xie wrote:
> > IMO, before a page is allocated, it is in buddy system, which means it is free
> > and no other 'map' on the page except direct map. Then if the page is allocated
> > to user, XPFO should unmap the direct map. otherwise the ret2dir may works at
> > this window before it is freed. Or maybe I'm still missing anything.
> 
> I agree that it seems broken. I'm just not sure why the test doesn't
> fail. It's certainly worth understanding.

Ok, so I think what's going on is that the page *is* mapped and unmapped by the
kernel as Juerg described, but only in certain cases. See prep_new_page(),
which has the following:

	if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
		for (i = 0; i < (1 << order); i++)
			clear_highpage(page + i);

clear_highpage() maps and unmaps the pages, so that's why xpfo works with this
set.

I tried with CONFIG_PAGE_POISONING_ZERO=y and page_poison=y, and the
XPFO_READ_USER test does not fail, i.e. the read succeeds. So, I think we need
to include this zeroing condition in xpfo_alloc_pages(), something like the
patch below. Unfortunately, this fails to boot for me, probably for an
unrelated reason that I'll look into.

Thanks a lot!

Tycho


From bfc21a6438cf8c56741af94cac939f1b0f63752c Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tycho@docker.com>
Date: Tue, 12 Sep 2017 12:06:41 -0600
Subject: [PATCH] draft of unmapping patch

Signed-off-by: Tycho Andersen <tycho@docker.com>
---
 include/linux/xpfo.h |  5 +++--
 mm/compaction.c      |  2 +-
 mm/internal.h        |  2 +-
 mm/page_alloc.c      | 10 ++++++----
 mm/xpfo.c            | 10 ++++++++--
 5 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/linux/xpfo.h b/include/linux/xpfo.h
index b24be9ac4a2d..c991bf7f051d 100644
--- a/include/linux/xpfo.h
+++ b/include/linux/xpfo.h
@@ -29,7 +29,7 @@ void xpfo_flush_kernel_tlb(struct page *page, int order);
 
 void xpfo_kmap(void *kaddr, struct page *page);
 void xpfo_kunmap(void *kaddr, struct page *page);
-void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp);
+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map);
 void xpfo_free_pages(struct page *page, int order);
 
 bool xpfo_page_is_unmapped(struct page *page);
@@ -49,7 +49,8 @@ void xpfo_temp_unmap(const void *addr, size_t size, void **mapping,
 
 static inline void xpfo_kmap(void *kaddr, struct page *page) { }
 static inline void xpfo_kunmap(void *kaddr, struct page *page) { }
-static inline void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp) { }
+static inline void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp,
+				    bool will_map) { }
 static inline void xpfo_free_pages(struct page *page, int order) { }
 
 static inline bool xpfo_page_is_unmapped(struct page *page) { return false; }
diff --git a/mm/compaction.c b/mm/compaction.c
index fb548e4c7bd4..9a222258e65c 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -76,7 +76,7 @@ static void map_pages(struct list_head *list)
 		order = page_private(page);
 		nr_pages = 1 << order;
 
-		post_alloc_hook(page, order, __GFP_MOVABLE);
+		post_alloc_hook(page, order, __GFP_MOVABLE, false);
 		if (order)
 			split_page(page, order);
 
diff --git a/mm/internal.h b/mm/internal.h
index 4ef49fc55e58..1a0331ec2b2d 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -165,7 +165,7 @@ extern void __free_pages_bootmem(struct page *page, unsigned long pfn,
 					unsigned int order);
 extern void prep_compound_page(struct page *page, unsigned int order);
 extern void post_alloc_hook(struct page *page, unsigned int order,
-					gfp_t gfp_flags);
+					gfp_t gfp_flags, bool will_map);
 extern int user_min_free_kbytes;
 
 #if defined CONFIG_COMPACTION || defined CONFIG_CMA
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 09fdf1bad21f..f73809847c58 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1750,7 +1750,7 @@ static bool check_new_pages(struct page *page, unsigned int order)
 }
 
 inline void post_alloc_hook(struct page *page, unsigned int order,
-				gfp_t gfp_flags)
+				gfp_t gfp_flags, bool will_map)
 {
 	set_page_private(page, 0);
 	set_page_refcounted(page);
@@ -1759,18 +1759,20 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	kernel_map_pages(page, 1 << order, 1);
 	kernel_poison_pages(page, 1 << order, 1);
 	kasan_alloc_pages(page, order);
-	xpfo_alloc_pages(page, order, gfp_flags);
+	xpfo_alloc_pages(page, order, gfp_flags, will_map);
 	set_page_owner(page, order, gfp_flags);
 }
 
+extern bool xpfo_test;
 static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
 							unsigned int alloc_flags)
 {
 	int i;
+	bool needs_zero = !free_pages_prezeroed() && (gfp_flags & __GFP_ZERO);
 
-	post_alloc_hook(page, order, gfp_flags);
+	post_alloc_hook(page, order, gfp_flags, needs_zero);
 
-	if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
+	if (needs_zero)
 		for (i = 0; i < (1 << order); i++)
 			clear_highpage(page + i);
 
diff --git a/mm/xpfo.c b/mm/xpfo.c
index ca5d4d1838f9..dd25e24213fe 100644
--- a/mm/xpfo.c
+++ b/mm/xpfo.c
@@ -86,7 +86,7 @@ static inline struct xpfo *lookup_xpfo(struct page *page)
 	return (void *)page_ext + page_xpfo_ops.offset;
 }
 
-void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
+void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map)
 {
 	int i, flush_tlb = 0;
 	struct xpfo *xpfo;
@@ -116,8 +116,14 @@ void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
 			 * Tag the page as a user page and flush the TLB if it
 			 * was previously allocated to the kernel.
 			 */
-			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
+			bool was_user = !test_and_set_bit(XPFO_PAGE_USER,
+							  &xpfo->flags);
+
+			if (was_user || !will_map) {
+				set_kpte(page_address(page + i), page + i,
+					 __pgprot(0));
 				flush_tlb = 1;
+			}
 		} else {
 			/* Tag the page as a non-user (kernel) page */
 			clear_bit(XPFO_PAGE_USER, &xpfo->flags);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1732057

Hi Tycho,

On 2017/9/13 2:13, Tycho Andersen wrote:
> Hi Yisheng,
> 
>> On Tue, Sep 12, 2017 at 04:05:22PM +0800, Yisheng Xie wrote:
>>> IMO, before a page is allocated, it is in buddy system, which means it is free
>>> and no other 'map' on the page except direct map. Then if the page is allocated
>>> to user, XPFO should unmap the direct map. otherwise the ret2dir may works at
>>> this window before it is freed. Or maybe I'm still missing anything.
>>
>> I agree that it seems broken. I'm just not sure why the test doesn't
>> fail. It's certainly worth understanding.
> 
> Ok, so I think what's going on is that the page *is* mapped and unmapped by the
> kernel as Juerg described, but only in certain cases. See prep_new_page(),
> which has the following:
> 
> 	if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
> 		for (i = 0; i < (1 << order); i++)
> 			clear_highpage(page + i);
> 
> clear_highpage() maps and unmaps the pages, so that's why xpfo works with this
> set.
Oh, I really missed this point. For we need zero the memory before user get them.

Thanks a lot for figuring out.

> 
> I tried with CONFIG_PAGE_POISONING_ZERO=y and page_poison=y, and the
> XPFO_READ_USER test does not fail, i.e. the read succeeds. So, I think we need
> to include this zeroing condition in xpfo_alloc_pages(), something like the
> patch below. Unfortunately, this fails to boot for me, probably for an
> unrelated reason that I'll look into.
Yes, seems need to fix in this case, and I also a litter puzzle about why boot fail.

Thanks
Yisheng Xie

> 
> Thanks a lot!
> 
> Tycho
> 
> 
>>From bfc21a6438cf8c56741af94cac939f1b0f63752c Mon Sep 17 00:00:00 2001
> From: Tycho Andersen <tycho@docker.com>
> Date: Tue, 12 Sep 2017 12:06:41 -0600
> Subject: [PATCH] draft of unmapping patch
> 
> Signed-off-by: Tycho Andersen <tycho@docker.com>
> ---
>  include/linux/xpfo.h |  5 +++--
>  mm/compaction.c      |  2 +-
>  mm/internal.h        |  2 +-
>  mm/page_alloc.c      | 10 ++++++----
>  mm/xpfo.c            | 10 ++++++++--
>  5 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/xpfo.h b/include/linux/xpfo.h
> index b24be9ac4a2d..c991bf7f051d 100644
> --- a/include/linux/xpfo.h
> +++ b/include/linux/xpfo.h
> @@ -29,7 +29,7 @@ void xpfo_flush_kernel_tlb(struct page *page, int order);
>  
>  void xpfo_kmap(void *kaddr, struct page *page);
>  void xpfo_kunmap(void *kaddr, struct page *page);
> -void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp);
> +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map);
>  void xpfo_free_pages(struct page *page, int order);
>  
>  bool xpfo_page_is_unmapped(struct page *page);
> @@ -49,7 +49,8 @@ void xpfo_temp_unmap(const void *addr, size_t size, void **mapping,
>  
>  static inline void xpfo_kmap(void *kaddr, struct page *page) { }
>  static inline void xpfo_kunmap(void *kaddr, struct page *page) { }
> -static inline void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp) { }
> +static inline void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp,
> +				    bool will_map) { }
>  static inline void xpfo_free_pages(struct page *page, int order) { }
>  
>  static inline bool xpfo_page_is_unmapped(struct page *page) { return false; }
> diff --git a/mm/compaction.c b/mm/compaction.c
> index fb548e4c7bd4..9a222258e65c 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -76,7 +76,7 @@ static void map_pages(struct list_head *list)
>  		order = page_private(page);
>  		nr_pages = 1 << order;
>  
> -		post_alloc_hook(page, order, __GFP_MOVABLE);
> +		post_alloc_hook(page, order, __GFP_MOVABLE, false);
>  		if (order)
>  			split_page(page, order);
>  
> diff --git a/mm/internal.h b/mm/internal.h
> index 4ef49fc55e58..1a0331ec2b2d 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -165,7 +165,7 @@ extern void __free_pages_bootmem(struct page *page, unsigned long pfn,
>  					unsigned int order);
>  extern void prep_compound_page(struct page *page, unsigned int order);
>  extern void post_alloc_hook(struct page *page, unsigned int order,
> -					gfp_t gfp_flags);
> +					gfp_t gfp_flags, bool will_map);
>  extern int user_min_free_kbytes;
>  
>  #if defined CONFIG_COMPACTION || defined CONFIG_CMA
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 09fdf1bad21f..f73809847c58 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1750,7 +1750,7 @@ static bool check_new_pages(struct page *page, unsigned int order)
>  }
>  
>  inline void post_alloc_hook(struct page *page, unsigned int order,
> -				gfp_t gfp_flags)
> +				gfp_t gfp_flags, bool will_map)
>  {
>  	set_page_private(page, 0);
>  	set_page_refcounted(page);
> @@ -1759,18 +1759,20 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>  	kernel_map_pages(page, 1 << order, 1);
>  	kernel_poison_pages(page, 1 << order, 1);
>  	kasan_alloc_pages(page, order);
> -	xpfo_alloc_pages(page, order, gfp_flags);
> +	xpfo_alloc_pages(page, order, gfp_flags, will_map);
>  	set_page_owner(page, order, gfp_flags);
>  }
>  
> +extern bool xpfo_test;
>  static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
>  							unsigned int alloc_flags)
>  {
>  	int i;
> +	bool needs_zero = !free_pages_prezeroed() && (gfp_flags & __GFP_ZERO);
>  
> -	post_alloc_hook(page, order, gfp_flags);
> +	post_alloc_hook(page, order, gfp_flags, needs_zero);
>  
> -	if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
> +	if (needs_zero)
>  		for (i = 0; i < (1 << order); i++)
>  			clear_highpage(page + i);
>  
> diff --git a/mm/xpfo.c b/mm/xpfo.c
> index ca5d4d1838f9..dd25e24213fe 100644
> --- a/mm/xpfo.c
> +++ b/mm/xpfo.c
> @@ -86,7 +86,7 @@ static inline struct xpfo *lookup_xpfo(struct page *page)
>  	return (void *)page_ext + page_xpfo_ops.offset;
>  }
>  
> -void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
> +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map)
>  {
>  	int i, flush_tlb = 0;
>  	struct xpfo *xpfo;
> @@ -116,8 +116,14 @@ void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
>  			 * Tag the page as a user page and flush the TLB if it
>  			 * was previously allocated to the kernel.
>  			 */
> -			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
> +			bool was_user = !test_and_set_bit(XPFO_PAGE_USER,
> +							  &xpfo->flags);
> +
> +			if (was_user || !will_map) {
> +				set_kpte(page_address(page + i), page + i,
> +					 __pgprot(0));
>  				flush_tlb = 1;
> +			}
>  		} else {
>  			/* Tag the page as a non-user (kernel) page */
>  			clear_bit(XPFO_PAGE_USER, &xpfo->flags);
> 

[toc] | [prev] | [next] | [standalone]

#1736268

On 09/12/2017 11:13 AM, Tycho Andersen wrote:
> -void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
> +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map)
>  {
>  	int i, flush_tlb = 0;
>  	struct xpfo *xpfo;
> @@ -116,8 +116,14 @@ void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
>  			 * Tag the page as a user page and flush the TLB if it
>  			 * was previously allocated to the kernel.
>  			 */
> -			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
> +			bool was_user = !test_and_set_bit(XPFO_PAGE_USER,
> +							  &xpfo->flags);
> +
> +			if (was_user || !will_map) {
> +				set_kpte(page_address(page + i), page + i,
> +					 __pgprot(0));
>  				flush_tlb = 1;
> +			}

Shouldn't the "was_user" be "was_kernel"?

Also, the way this now works, let's say we have a nice, 2MB pmd_t (page
table entry) mapping a nice, 2MB page in the allocator.  Then it gets
allocated to userspace.  We do

	for (i = 0; i < (1 << order); i++)  {
		...
		set_kpte(page_address(page + i), page+i, __pgprot(0));
	}

The set_kpte() will take the nice, 2MB mapping and break it down into
512 4k mappings, all pointing to a non-present PTE, in a newly-allocated
PTE page.  So, you get the same result and waste 4k of memory in the
process, *AND* make it slower because we added a level to the page tables.

I think you actually want to make a single set_kpte() call at the end of
the function.  That's faster and preserves the large page in the direct
mapping.

[toc] | [prev] | [next] | [standalone]

#1736274

On 09/20/2017 05:02 PM, Tycho Andersen wrote:
> ...and makes it easier to pair tlb flushes with changing the
> protections. I guess we still need the for loop, because we need to
> set/unset the xpfo bits as necessary, but I'll switch it to a single
> set_kpte(). This also implies that the xpfo bits should all be the
> same on every page in the mapping, which I think is true.

FWIW, it's a bit bonkers to keep all this duplicate xpfo metadata for
compound pages.  You could probably get away with only keeping it for
the head page.

[toc] | [prev] | [next] | [standalone]

#1736276

On Wed, Sep 20, 2017 at 04:46:41PM -0700, Dave Hansen wrote:
> On 09/12/2017 11:13 AM, Tycho Andersen wrote:
> > -void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
> > +void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp, bool will_map)
> >  {
> >  	int i, flush_tlb = 0;
> >  	struct xpfo *xpfo;
> > @@ -116,8 +116,14 @@ void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp)
> >  			 * Tag the page as a user page and flush the TLB if it
> >  			 * was previously allocated to the kernel.
> >  			 */
> > -			if (!test_and_set_bit(XPFO_PAGE_USER, &xpfo->flags))
> > +			bool was_user = !test_and_set_bit(XPFO_PAGE_USER,
> > +							  &xpfo->flags);
> > +
> > +			if (was_user || !will_map) {
> > +				set_kpte(page_address(page + i), page + i,
> > +					 __pgprot(0));
> >  				flush_tlb = 1;
> > +			}
> 
> Shouldn't the "was_user" be "was_kernel"?

Oof, yes, thanks.

> Also, the way this now works, let's say we have a nice, 2MB pmd_t (page
> table entry) mapping a nice, 2MB page in the allocator.  Then it gets
> allocated to userspace.  We do
> 
> 	for (i = 0; i < (1 << order); i++)  {
> 		...
> 		set_kpte(page_address(page + i), page+i, __pgprot(0));
> 	}
> 
> The set_kpte() will take the nice, 2MB mapping and break it down into
> 512 4k mappings, all pointing to a non-present PTE, in a newly-allocated
> PTE page.  So, you get the same result and waste 4k of memory in the
> process, *AND* make it slower because we added a level to the page tables.
> 
> I think you actually want to make a single set_kpte() call at the end of
> the function.  That's faster and preserves the large page in the direct
> mapping.

...and makes it easier to pair tlb flushes with changing the
protections. I guess we still need the for loop, because we need to
set/unset the xpfo bits as necessary, but I'll switch it to a single
set_kpte(). This also implies that the xpfo bits should all be the
same on every page in the mapping, which I think is true.

This will be a nice change, thanks!

Tycho

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web