Add support for eXclusive Page Frame Ownership

mm/xpfo.c new file +63 -0

new file mode 100644
index 000000000000..3cd45f68b5ad
@@ -0,0 +1,208 @@
+/*
+ * Copyright (C) 2017 Hewlett Packard Enterprise Development, L.P.
+ * Copyright (C) 2016 Brown University. All rights reserved.
+ *
+ * Authors:
+ *   Juerg Haefliger <[email protected]>
+ *   Vasileios P. Kemerlis <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/page_ext.h>
+#include <linux/xpfo.h>
+
+#include <asm/tlbflush.h>
+
+/* XPFO page state flags */
+enum xpfo_flags {
+	XPFO_PAGE_USER,		/* Page is allocated to user-space */
+	XPFO_PAGE_UNMAPPED,	/* Page is unmapped from the linear map */
+};
+
+/* Per-page XPFO house-keeping data */
+struct xpfo {
+	unsigned long flags;	/* Page state */
+	bool inited;		/* Map counter and lock initialized */
+	atomic_t mapcount;	/* Counter for balancing map/unmap requests */
+	spinlock_t maplock;	/* Lock to serialize map/unmap requests */
+};
+
+DEFINE_STATIC_KEY_FALSE(xpfo_inited);
+
+static bool xpfo_disabled __initdata;
+
+static int __init noxpfo_param(char *str)
+{
+	xpfo_disabled = true;
+
+	return 0;
+}
+
+early_param("noxpfo", noxpfo_param);
+
+static bool __init need_xpfo(void)
+{
+	if (xpfo_disabled) {
+		printk(KERN_INFO "XPFO disabled\n");
+		return false;
+	}
+
+	return true;
+}
+
+static void init_xpfo(void)
+{
+	printk(KERN_INFO "XPFO enabled\n");
+	static_branch_enable(&xpfo_inited);
+}
+

arch/x86/mm/xpfo.c new file +31 -0

+/* Update a single kernel page table entry */
+inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
+{
+	unsigned int level;
+	pgprot_t msk_clr;
+	pte_t *pte = lookup_address((unsigned long)kaddr, &level);
+
+	BUG_ON(!pte);
+
+	switch (level) {
+	case PG_LEVEL_4K:
+		set_pte_atomic(pte, pfn_pte(page_to_pfn(page), canon_pgprot(prot)));
+		break;
+	case PG_LEVEL_2M:
+		/* We need to check if it's a 2M page or 1GB page before retrieve
+		 * pgprot info, as each one will be extracted from a different
+		 * page table levels */
+		msk_clr = pmd_pgprot(*(pmd_t*)pte);
+	case PG_LEVEL_1G: {
+		struct cpa_data cpa;
+		int do_split;
+
+		msk_clr = pud_pgprot(*(pud_t*)pte);
+
+		memset(&cpa, 0, sizeof(cpa));
+		cpa.vaddr = kaddr;
+		cpa.pages = &page;
+		cpa.mask_set = prot;
+		cpa.mask_clr = msk_clr;
+		cpa.numpages = 1;
+		cpa.flags = 0;

arch/x86/mm/xpfo.c new file +11 -0

+
+		break;
+	}
+	case PG_LEVEL_512G:
+		/* fallthrough, splitting infrastructure doesn't
+		 * support 512G pages. */
+	default:
+		BUG();
+	}
+
+}

Quoted 31 lines · Tycho Andersen

arch/x86/mm/xpfo.c new file +31 -0

+/* Update a single kernel page table entry */
+inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
+{
+	unsigned int level;
+	pgprot_t msk_clr;
+	pte_t *pte = lookup_address((unsigned long)kaddr, &level);
+
+	BUG_ON(!pte);
+
+	switch (level) {
+	case PG_LEVEL_4K:
+		set_pte_atomic(pte, pfn_pte(page_to_pfn(page), canon_pgprot(prot)));
+		break;
+	case PG_LEVEL_2M:
+		/* We need to check if it's a 2M page or 1GB page before retrieve
+		 * pgprot info, as each one will be extracted from a different
+		 * page table levels */
+		msk_clr = pmd_pgprot(*(pmd_t*)pte);
+	case PG_LEVEL_1G: {
+		struct cpa_data cpa;
+		int do_split;
+
+		msk_clr = pud_pgprot(*(pud_t*)pte);
+
+		memset(&cpa, 0, sizeof(cpa));
+		cpa.vaddr = kaddr;
+		cpa.pages = &page;
+		cpa.mask_set = prot;
+		cpa.mask_clr = msk_clr;
+		cpa.numpages = 1;
+		cpa.flags = 0;

arch/x86/mm/xpfo.c line 60

Quoted 10 lines

arch/x86/mm/xpfo.c new file +10 -0

+		cpa.curpage = 0;
+		cpa.force_split = 0;
+
+
+		do_split = try_preserve_large_page(pte, (unsigned long)kaddr, &cpa);
+		if (do_split) {
+			spin_lock(&cpa_lock);
+			BUG_ON(split_large_page(&cpa, pte, (unsigned long)kaddr));
+			spin_unlock(&cpa_lock);
+		}

This doesn't work in atomic contexts:

[   28.263571] BUG: sleeping function called from invalid context at
mm/page_alloc.c:4048
[   28.263575] in_atomic(): 1, irqs_disabled(): 1, pid: 2433, name:
gnome-terminal
[   28.263576] INFO: lockdep is turned off.
[   28.263578] irq event stamp: 0
[   28.263580] hardirqs last  enabled at (0): [<          (null)>]
(null)
[   28.263584] hardirqs last disabled at (0): [<ffffffff840af28a>]
copy_process.part.25+0x62a/0x1e90
[   28.263587] softirqs last  enabled at (0): [<ffffffff840af28a>]
copy_process.part.25+0x62a/0x1e90
[   28.263588] softirqs last disabled at (0): [<          (null)>]
(null)
[   28.263591] CPU: 0 PID: 2433 Comm: gnome-terminal Tainted: G        W
4.13.0-rc5-xpfo+ #86
[   28.263592] Hardware name: LENOVO 20BTS1N700/20BTS1N700, BIOS N14ET28W
(1.06 ) 03/12/2015
[   28.263593] Call Trace:
[   28.263598]  dump_stack+0x8e/0xd6
[   28.263601]  ___might_sleep+0x164/0x250
[   28.263604]  __might_sleep+0x4a/0x80
[   28.263607]  __alloc_pages_nodemask+0x2b3/0x3e0
[   28.263611]  alloc_pages_current+0x6a/0xe0
[   28.263614]  split_large_page+0x4e/0x360
[   28.263618]  set_kpte+0x12c/0x150
[   28.263623]  xpfo_kunmap+0x7e/0xa0
[   28.263627]  wp_page_copy+0x16e/0x800
[   28.263631]  do_wp_page+0x9a/0x580
[   28.263633]  __handle_mm_fault+0xb1c/0x1130
[   28.263638]  handle_mm_fault+0x178/0x350
[   28.263641]  __do_page_fault+0x26e/0x510
[   28.263644]  do_page_fault+0x30/0x80
[   28.263647]  page_fault+0x28/0x30

split_large_page calls alloc_page with GFP_KERNEL. switching to use GFP_ATOMIC in this path works locally for me.

I'll do that for the next version

From: Juerg Haefliger <[email protected]>

Signed-off-by Juerg Haefliger <[email protected]>

Tested-by Tycho Andersen <[email protected]>

---
 include/linux/xpfo.h | 4 ++++
 lib/swiotlb.c        | 3 ++-
 mm/xpfo.c            | 9 +++++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

include/linux/xpfo.h +4 -0

index 1ff2d1976837..6b61f7b820f4 100644
@@ -27,6 +27,8 @@ void xpfo_kunmap(void *kaddr, struct page *page);
 void xpfo_alloc_pages(struct page *page, int order, gfp_t gfp);
 void xpfo_free_pages(struct page *page, int order);
 
+bool xpfo_page_is_unmapped(struct page *page);
+
 #else /* !CONFIG_XPFO */
 
 static inline void xpfo_kmap(void *kaddr, struct page *page) { }

@@ -34,6 +36,8 @@ 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_free_pages(struct page *page, int order) { }
 
+static inline bool xpfo_page_is_unmapped(struct page *page) { return false; }
+
 #endif /* CONFIG_XPFO */
 
 #endif /* _LINUX_XPFO_H */

lib/swiotlb.c +2 -1

index a8d74a733a38..d4fee5ca2d9e 100644
@@ -420,8 +420,9 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr,
 {
 	unsigned long pfn = PFN_DOWN(orig_addr);
 	unsigned char *vaddr = phys_to_virt(tlb_addr);
+	struct page *page = pfn_to_page(pfn);
 
-	if (PageHighMem(pfn_to_page(pfn))) {
+	if (PageHighMem(page) || xpfo_page_is_unmapped(page)) {
 		/* The buffer does not have a mapping.  Map it in and copy */
 		unsigned int offset = orig_addr & ~PAGE_MASK;
 		char *buffer;

mm/xpfo.c +0 -0

index 3cd45f68b5ad..3f305f31a072 100644
@@ -206,3 +206,12 @@ void xpfo_kunmap(void *kaddr, struct page *page)
 	spin_unlock_irqrestore(&xpfo->maplock, flags);
 }

Signature · 1 line

2.11.0

mm/xpfo.c void xpfo_kunmap(void *kaddr, struct page *page) · line 217

Quoted 8 lines · Tycho Andersen

mm/xpfo.c +8 -0

+inline bool xpfo_page_is_unmapped(struct page *page)
+{
+	if (!static_branch_unlikely(&xpfo_inited))
+		return false;
+
+	return test_bit(XPFO_PAGE_UNMAPPED, &lookup_xpfo(page)->flags);
+}
+EXPORT_SYMBOL(xpfo_page_is_unmapped);

How can it be inline and 'EXPORT_SYMBOL' ? And why make it inline? It surely does not need to be access that often?

static inline void *skcipher_map(struct scatter_walk *walk)

{
        struct page *page = scatterwalk_page(walk);

        return (PageHighMem(page) ? kmap_atomic(page) : page_address(page)) +
               offset_in_page(walk->offset);
}

Quoted 8 lines · Tycho Andersen

lib/swiotlb.c +1 -0

@@ -420,8 +420,9 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr,
 {
 	unsigned long pfn = PFN_DOWN(orig_addr);
 	unsigned char *vaddr = phys_to_virt(tlb_addr);
+	struct page *page = pfn_to_page(pfn);
 

lib/swiotlb.c static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr, · line 428

Quoted 5 lines

lib/swiotlb.c +1 -1

-	if (PageHighMem(pfn_to_page(pfn))) {
+	if (PageHighMem(page) || xpfo_page_is_unmapped(page)) {
 		/* The buffer does not have a mapping.  Map it in and copy */
 		unsigned int offset = orig_addr & ~PAGE_MASK;
 		char *buffer;

This is a little scary. I wonder how many more of these are in the kernel, like:

Quoted 7 lines

static inline void *skcipher_map(struct scatter_walk *walk)

{
        struct page *page = scatterwalk_page(walk);

        return (PageHighMem(page) ? kmap_atomic(page) : page_address(page)) +
               offset_in_page(walk->offset);
}

Is there any better way to catch these? Like, can we add some debugging to check for XPFO pages in __va()?

Quoted 10 lines

Quoted 8 lines

static inline void *skcipher_map(struct scatter_walk *walk)

{
        struct page *page = scatterwalk_page(walk);

        return (PageHighMem(page) ? kmap_atomic(page) : page_address(page)) +
               offset_in_page(walk->offset);
}

Is there any better way to catch these? Like, can we add some debugging to check for XPFO pages in __va()?

Yes, and perhaps also a debugging check in PageHighMem?

Would __va have caught either of the two cases you've pointed out?

From: Juerg Haefliger <[email protected]>

Add a hook for flushing a single TLB entry on arm64.

Signed-off-by Juerg Haefliger <[email protected]>

Tested-by Tycho Andersen <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm64/include/asm/tlbflush.h +0 -0

index af1c76981911..8e0c49105d3e 100644

Quoted 18 lines · Tycho Andersen

From: Juerg Haefliger <[email protected]>

Add a hook for flushing a single TLB entry on arm64.

Signed-off-by Juerg Haefliger <[email protected]>

Tested-by Tycho Andersen <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm64/include/asm/tlbflush.h +0 -0

index af1c76981911..8e0c49105d3e 100644
@@ -184,6 +184,14 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end
 	isb();
 }
 

arch/arm64/include/asm/tlbflush.h static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end · line 193

Quoted 7 lines

arch/arm64/include/asm/tlbflush.h +7 -0

+static inline void __flush_tlb_one(unsigned long addr)
+{
+	dsb(ishst);
+	__tlbi(vaae1is, addr >> 12);
+	dsb(ish);
+	isb();
+}

Is this going to be called by generic code?

It would be nice if we could drop 'kernel' into the name, to make it clear this is intended to affect the kernel mappings, which have different maintenance requirements to user mappings.

We should be able to implement this more simply as:

flush_tlb_kernel_page(unsigned long addr)

{
	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
}

Hi Mark,

Quoted 10 lines · Mark Rutland

arch/arm64/include/asm/tlbflush.h static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end · line 193

Quoted 7 lines · Tycho Andersen

arch/arm64/include/asm/tlbflush.h +7 -0

+static inline void __flush_tlb_one(unsigned long addr)
+{
+	dsb(ishst);
+	__tlbi(vaae1is, addr >> 12);
+	dsb(ish);
+	isb();
+}

Is this going to be called by generic code?

Yes, it's called in mm/xpfo.c:xpfo_kunmap.

Quoted 3 lines

It would be nice if we could drop 'kernel' into the name, to make it clear this is intended to affect the kernel mappings, which have different maintenance requirements to user mappings.

It's named __flush_tlb_one after the x86 (and a few other arches) function of the same name. I can change it to flush_tlb_kernel_page, but then we'll need some x86-specific code to map the name as well.

Maybe since it's called from generic code that's warranted though? I'll change the implementation for now, let me know what you want to do about the name.

Quoted 20 lines · Tycho Andersen

Hi Mark,

Quoted 10 lines · Mark Rutland

arch/arm64/include/asm/tlbflush.h static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end · line 193

Quoted 7 lines · Tycho Andersen

arch/arm64/include/asm/tlbflush.h +7 -0

+static inline void __flush_tlb_one(unsigned long addr)
+{
+	dsb(ishst);
+	__tlbi(vaae1is, addr >> 12);
+	dsb(ish);
+	isb();
+}

Is this going to be called by generic code?

Yes, it's called in mm/xpfo.c:xpfo_kunmap.

Quoted 3 lines

It would be nice if we could drop 'kernel' into the name, to make it clear this is intended to affect the kernel mappings, which have different maintenance requirements to user mappings.

Quoted 7 lines

It's named __flush_tlb_one after the x86 (and a few other arches) function of the same name. I can change it to flush_tlb_kernel_page, but then we'll need some x86-specific code to map the name as well.

Maybe since it's called from generic code that's warranted though? I'll change the implementation for now, let me know what you want to do about the name.

I think it would be preferable to do so, to align with flush_tlb_kernel_range(), which is an existing generic interface.

That said, is there any reason not to use flush_tlb_kernel_range() directly?

That said, is there any reason not to use flush_tlb_kernel_range() directly?

Hi Mark,

Quoted 2 lines · Mark Rutland

That said, is there any reason not to use flush_tlb_kernel_range() directly?

So it turns out that there is a difference between __flush_tlb_one() and flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which I think is enough here).

Quoted 10 lines · Tycho Andersen

Hi Mark,

Quoted 2 lines · Mark Rutland

That said, is there any reason not to use flush_tlb_kernel_range() directly?

So it turns out that there is a difference between __flush_tlb_one() and flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which I think is enough here).

That sounds suspicious; I don't think that __flush_tlb_one() is sufficient.

If you only do local TLB maintenance, then the page is left accessible to other CPUs via the (stale) kernel mappings. i.e. the page isn't exclusively mapped by userspace.

From 1d1b0a18d56cf1634072096231bfbaa96cb2aa16 Mon Sep 17 00:00:00 2001

Quoted 15 lines · Mark Rutland

Quoted 10 lines · Tycho Andersen

Hi Mark,

Quoted 2 lines · Mark Rutland

That said, is there any reason not to use flush_tlb_kernel_range() directly?

So it turns out that there is a difference between __flush_tlb_one() and flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which I think is enough here).

That sounds suspicious; I don't think that __flush_tlb_one() is sufficient.

Quoted 3 lines

If you only do local TLB maintenance, then the page is left accessible to other CPUs via the (stale) kernel mappings. i.e. the page isn't exclusively mapped by userspace.

I thought so too, so I tried to test it with something like the patch below. But it correctly failed for me when using __flush_tlb_one(). I suppose I'm doing something wrong in the test, but I'm not sure what.

Tycho

From 1d1b0a18d56cf1634072096231bfbaa96cb2aa16 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <[email protected]>
Date: Tue, 22 Aug 2017 18:07:12 -0600
Subject: [PATCH] add XPFO_SMP test

Signed-off-by Tycho Andersen <[email protected]>

---
 drivers/misc/lkdtm.h      |   1 +
 drivers/misc/lkdtm_core.c |   1 +
 drivers/misc/lkdtm_xpfo.c | 139 ++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 130 insertions(+), 11 deletions(-)

drivers/misc/lkdtm.h +1 -0

index fc53546113c1..34a6ee37f216 100644
@@ -67,5 +67,6 @@ void lkdtm_USERCOPY_KERNEL(void);
 /* lkdtm_xpfo.c */
 void lkdtm_XPFO_READ_USER(void);
 void lkdtm_XPFO_READ_USER_HUGE(void);
+void lkdtm_XPFO_SMP(void);
 
 #endif

drivers/misc/lkdtm_core.c +1 -0

index 164bc404f416..9544e329de4b 100644
@@ -237,6 +237,7 @@ struct crashtype crashtypes[] = {
 	CRASHTYPE(USERCOPY_KERNEL),
 	CRASHTYPE(XPFO_READ_USER),
 	CRASHTYPE(XPFO_READ_USER_HUGE),
+	CRASHTYPE(XPFO_SMP),
 };
 
 

drivers/misc/lkdtm_xpfo.c +128 -11

index c72509128eb3..7600fdcae22f 100644
@@ -4,22 +4,27 @@
 
 #include "lkdtm.h"
 
+#include <linux/cpumask.h>
 #include <linux/mman.h>
 #include <linux/uaccess.h>
 #include <linux/xpfo.h>
+#include <linux/kthread.h>
 
-void read_user_with_flags(unsigned long flags)
+#include <linux/delay.h>
+#include <linux/sched/task.h>
+
+#define XPFO_DATA 0xdeadbeef
+
+static unsigned long do_map(unsigned long flags)
 {
-	unsigned long user_addr, user_data = 0xdeadbeef;
-	phys_addr_t phys_addr;
-	void *virt_addr;
+	unsigned long user_addr, user_data = XPFO_DATA;
 
 	user_addr = vm_mmap(NULL, 0, PAGE_SIZE,
 			    PROT_READ | PROT_WRITE | PROT_EXEC,
 			    flags, 0);
 	if (user_addr >= TASK_SIZE) {
 		pr_warn("Failed to allocate user memory\n");
-		return;
+		return 0;
 	}
 
 	if (copy_to_user((void __user *)user_addr, &user_data,

@@ -28,25 +33,61 @@ void read_user_with_flags(unsigned long flags)
 		goto free_user;
 	}
 
+	return user_addr;
+
+free_user:
+	vm_munmap(user_addr, PAGE_SIZE);
+	return 0;
+}
+
+static unsigned long *user_to_kernel(unsigned long user_addr)
+{
+	phys_addr_t phys_addr;
+	void *virt_addr;
+
 	phys_addr = user_virt_to_phys(user_addr);
 	if (!phys_addr) {
 		pr_warn("Failed to get physical address of user memory\n");
-		goto free_user;
+		return 0;
 	}
 
 	virt_addr = phys_to_virt(phys_addr);
 	if (phys_addr != virt_to_phys(virt_addr)) {
 		pr_warn("Physical address of user memory seems incorrect\n");
-		goto free_user;
+		return 0;
 	}
 
+	return virt_addr;
+}
+
+static void read_map(unsigned long *virt_addr)
+{
 	pr_info("Attempting bad read from kernel address %p\n", virt_addr);
-	if (*(unsigned long *)virt_addr == user_data)
-		pr_info("Huh? Bad read succeeded?!\n");
+	if (*(unsigned long *)virt_addr == XPFO_DATA)
+		pr_err("FAIL: Bad read succeeded?!\n");
 	else
-		pr_info("Huh? Bad read didn't fail but data is incorrect?!\n");
+		pr_err("FAIL: Bad read didn't fail but data is incorrect?!\n");
+}
+
+static void read_user_with_flags(unsigned long flags)
+{
+	unsigned long user_addr, *kernel;
+
+	user_addr = do_map(flags);
+	if (!user_addr) {
+		pr_err("FAIL: map failed\n");
+		return;
+	}
+
+	kernel = user_to_kernel(user_addr);
+	if (!kernel) {
+		pr_err("FAIL: user to kernel conversion failed\n");
+		goto free_user;
+	}
+
+	read_map(kernel);
 
- free_user:
+free_user:
 	vm_munmap(user_addr, PAGE_SIZE);
 }
 

@@ -60,3 +101,79 @@ void lkdtm_XPFO_READ_USER_HUGE(void)
 {
 	read_user_with_flags(MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB);
 }
+
+struct smp_arg {
+	struct completion map_done;
+	unsigned long *virt_addr;
+	unsigned int cpu;
+};
+
+static int smp_reader(void *parg)
+{
+	struct smp_arg *arg = parg;
+
+	if (arg->cpu != smp_processor_id()) {
+		pr_err("FAIL: scheduled on wrong CPU?\n");
+		return 0;
+	}
+
+	wait_for_completion(&arg->map_done);
+
+	if (arg->virt_addr)
+		read_map(arg->virt_addr);
+
+	return 0;
+}
+
+/* The idea here is to read from the kernel's map on a different thread than
+ * did the mapping (and thus the TLB flushing), to make sure that the page
+ * faults on other cores too.
+ */
+void lkdtm_XPFO_SMP(void)
+{
+	unsigned long user_addr;
+	struct task_struct *thread;
+	int ret;
+	struct smp_arg arg;
+
+	init_completion(&arg.map_done);
+
+	if (num_online_cpus() < 2) {
+		pr_err("not enough to do a multi cpu test\n");
+		return;
+	}
+
+	arg.cpu = (smp_processor_id() + 1) % num_online_cpus();
+	thread = kthread_create(smp_reader, &arg, "lkdtm_xpfo_test");
+	if (IS_ERR(thread)) {
+		pr_err("couldn't create kthread? %ld\n", PTR_ERR(thread));
+		return;
+	}
+
+	kthread_bind(thread, arg.cpu);
+	get_task_struct(thread);
+	wake_up_process(thread);
+
+	user_addr = do_map(MAP_PRIVATE | MAP_ANONYMOUS);
+	if (user_addr) {
+		arg.virt_addr = user_to_kernel(user_addr);
+		/* child thread checks for failure */
+	}
+
+	complete(&arg.map_done);
+
+	/* there must be a better way to do this. */
+	while (1) {
+		if (thread->exit_state)
+			break;
+		msleep_interruptible(100);
+	}
+
+	ret = kthread_stop(thread);
+	if (ret != SIGKILL)
+		pr_err("FAIL: thread wasn't killed: %d\n", ret);
+	put_task_struct(thread);
+
+	if (user_addr)
+		vm_munmap(user_addr, PAGE_SIZE);
+}

Signature · 1 line

2.11.0

Quoted 16 lines · Tycho Andersen

Quoted 15 lines · Mark Rutland

Quoted 10 lines · Tycho Andersen

Hi Mark,

Quoted 2 lines · Mark Rutland

That said, is there any reason not to use flush_tlb_kernel_range() directly?

So it turns out that there is a difference between __flush_tlb_one() and flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which I think is enough here).

That sounds suspicious; I don't think that __flush_tlb_one() is sufficient.

Quoted 7 lines

Quoted 3 lines

If you only do local TLB maintenance, then the page is left accessible to other CPUs via the (stale) kernel mappings. i.e. the page isn't exclusively mapped by userspace.

I thought so too, so I tried to test it with something like the patch below. But it correctly failed for me when using __flush_tlb_one(). I suppose I'm doing something wrong in the test, but I'm not sure what.

I suspect the issue is that you use a completion to synchronise the mapping.

The reader thread will block (i.e. it we go into schedule() and something else will run), and I guess that on x86, that the context-switch this entails upon completion happens to invalidate the TLBs.

Instead, you could serialise the update with the reader doing:

	/* spin until address is published to us */
	addr = smp_cond_load_acquire(arg->virt_addr, VAL != NULL);

	read_map(addr);

... and the writer doing:

	user_addr = do_map(...)

	...

	smp_store_release(arg->virt_addr, user_addr);

There would still be a chance of a context-switch, but it wouldn't be mandatory.

As an aside, it looks like DEBUG_PAGEALLOC on x86 has the problem w.r.t. under-invalidating, juding by the comments in x86's __kernel_map_pages(). It only invalidates the local TLBs, even though it should do it on all CPUs.

Thanks,
Mark.

Quoted 227 lines

Tycho

From 1d1b0a18d56cf1634072096231bfbaa96cb2aa16 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <[email protected]>
Date: Tue, 22 Aug 2017 18:07:12 -0600
Subject: [PATCH] add XPFO_SMP test

Signed-off-by Tycho Andersen <[email protected]>

---
 drivers/misc/lkdtm.h      |   1 +
 drivers/misc/lkdtm_core.c |   1 +
 drivers/misc/lkdtm_xpfo.c | 139 ++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 130 insertions(+), 11 deletions(-)

drivers/misc/lkdtm.h +1 -0

index fc53546113c1..34a6ee37f216 100644
@@ -67,5 +67,6 @@ void lkdtm_USERCOPY_KERNEL(void);
 /* lkdtm_xpfo.c */
 void lkdtm_XPFO_READ_USER(void);
 void lkdtm_XPFO_READ_USER_HUGE(void);
+void lkdtm_XPFO_SMP(void);
 
 #endif

drivers/misc/lkdtm_core.c +1 -0

index 164bc404f416..9544e329de4b 100644
@@ -237,6 +237,7 @@ struct crashtype crashtypes[] = {
 	CRASHTYPE(USERCOPY_KERNEL),
 	CRASHTYPE(XPFO_READ_USER),
 	CRASHTYPE(XPFO_READ_USER_HUGE),
+	CRASHTYPE(XPFO_SMP),
 };
 
 

drivers/misc/lkdtm_xpfo.c +128 -11

index c72509128eb3..7600fdcae22f 100644
@@ -4,22 +4,27 @@
 
 #include "lkdtm.h"
 
+#include <linux/cpumask.h>
 #include <linux/mman.h>
 #include <linux/uaccess.h>
 #include <linux/xpfo.h>
+#include <linux/kthread.h>
 
-void read_user_with_flags(unsigned long flags)
+#include <linux/delay.h>
+#include <linux/sched/task.h>
+
+#define XPFO_DATA 0xdeadbeef
+
+static unsigned long do_map(unsigned long flags)
 {
-	unsigned long user_addr, user_data = 0xdeadbeef;
-	phys_addr_t phys_addr;
-	void *virt_addr;
+	unsigned long user_addr, user_data = XPFO_DATA;
 
 	user_addr = vm_mmap(NULL, 0, PAGE_SIZE,
 			    PROT_READ | PROT_WRITE | PROT_EXEC,
 			    flags, 0);
 	if (user_addr >= TASK_SIZE) {
 		pr_warn("Failed to allocate user memory\n");
-		return;
+		return 0;
 	}
 
 	if (copy_to_user((void __user *)user_addr, &user_data,

@@ -28,25 +33,61 @@ void read_user_with_flags(unsigned long flags)
 		goto free_user;
 	}
 
+	return user_addr;
+
+free_user:
+	vm_munmap(user_addr, PAGE_SIZE);
+	return 0;
+}
+
+static unsigned long *user_to_kernel(unsigned long user_addr)
+{
+	phys_addr_t phys_addr;
+	void *virt_addr;
+
 	phys_addr = user_virt_to_phys(user_addr);
 	if (!phys_addr) {
 		pr_warn("Failed to get physical address of user memory\n");
-		goto free_user;
+		return 0;
 	}
 
 	virt_addr = phys_to_virt(phys_addr);
 	if (phys_addr != virt_to_phys(virt_addr)) {
 		pr_warn("Physical address of user memory seems incorrect\n");
-		goto free_user;
+		return 0;
 	}
 
+	return virt_addr;
+}
+
+static void read_map(unsigned long *virt_addr)
+{
 	pr_info("Attempting bad read from kernel address %p\n", virt_addr);
-	if (*(unsigned long *)virt_addr == user_data)
-		pr_info("Huh? Bad read succeeded?!\n");
+	if (*(unsigned long *)virt_addr == XPFO_DATA)
+		pr_err("FAIL: Bad read succeeded?!\n");
 	else
-		pr_info("Huh? Bad read didn't fail but data is incorrect?!\n");
+		pr_err("FAIL: Bad read didn't fail but data is incorrect?!\n");
+}
+
+static void read_user_with_flags(unsigned long flags)
+{
+	unsigned long user_addr, *kernel;
+
+	user_addr = do_map(flags);
+	if (!user_addr) {
+		pr_err("FAIL: map failed\n");
+		return;
+	}
+
+	kernel = user_to_kernel(user_addr);
+	if (!kernel) {
+		pr_err("FAIL: user to kernel conversion failed\n");
+		goto free_user;
+	}
+
+	read_map(kernel);
 
- free_user:
+free_user:
 	vm_munmap(user_addr, PAGE_SIZE);
 }
 

@@ -60,3 +101,79 @@ void lkdtm_XPFO_READ_USER_HUGE(void)
 {
 	read_user_with_flags(MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB);
 }
+
+struct smp_arg {
+	struct completion map_done;
+	unsigned long *virt_addr;
+	unsigned int cpu;
+};
+
+static int smp_reader(void *parg)
+{
+	struct smp_arg *arg = parg;
+
+	if (arg->cpu != smp_processor_id()) {
+		pr_err("FAIL: scheduled on wrong CPU?\n");
+		return 0;
+	}
+
+	wait_for_completion(&arg->map_done);
+
+	if (arg->virt_addr)
+		read_map(arg->virt_addr);
+
+	return 0;
+}
+
+/* The idea here is to read from the kernel's map on a different thread than
+ * did the mapping (and thus the TLB flushing), to make sure that the page
+ * faults on other cores too.
+ */
+void lkdtm_XPFO_SMP(void)
+{
+	unsigned long user_addr;
+	struct task_struct *thread;
+	int ret;
+	struct smp_arg arg;
+
+	init_completion(&arg.map_done);
+
+	if (num_online_cpus() < 2) {
+		pr_err("not enough to do a multi cpu test\n");
+		return;
+	}
+
+	arg.cpu = (smp_processor_id() + 1) % num_online_cpus();
+	thread = kthread_create(smp_reader, &arg, "lkdtm_xpfo_test");
+	if (IS_ERR(thread)) {
+		pr_err("couldn't create kthread? %ld\n", PTR_ERR(thread));
+		return;
+	}
+
+	kthread_bind(thread, arg.cpu);
+	get_task_struct(thread);
+	wake_up_process(thread);
+
+	user_addr = do_map(MAP_PRIVATE | MAP_ANONYMOUS);
+	if (user_addr) {
+		arg.virt_addr = user_to_kernel(user_addr);
+		/* child thread checks for failure */
+	}
+
+	complete(&arg.map_done);
+
+	/* there must be a better way to do this. */
+	while (1) {
+		if (thread->exit_state)
+			break;
+		msleep_interruptible(100);
+	}
+
+	ret = kthread_stop(thread);
+	if (ret != SIGKILL)
+		pr_err("FAIL: thread wasn't killed: %d\n", ret);
+	put_task_struct(thread);
+
+	if (user_addr)
+		vm_munmap(user_addr, PAGE_SIZE);
+}

Signature · 1 line

2.11.0

Quoted 10 lines · Tycho Andersen

Hi Mark,

Quoted 2 lines · Mark Rutland

That said, is there any reason not to use flush_tlb_kernel_range() directly?

So it turns out that there is a difference between __flush_tlb_one() and flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which I think is enough here).

That sounds suspicious; I don't think that __flush_tlb_one() is sufficient.

If you only do local TLB maintenance, then the page is left accessible to other CPUs via the (stale) kernel mappings. i.e. the page isn't exclusively mapped by userspace.

Thanks,
Mark.

Quoted 15 lines · Mark Rutland

Quoted 10 lines · Tycho Andersen

Hi Mark,

Quoted 2 lines · Mark Rutland

That said, is there any reason not to use flush_tlb_kernel_range() directly?

So it turns out that there is a difference between __flush_tlb_one() and flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which I think is enough here).

That sounds suspicious; I don't think that __flush_tlb_one() is sufficient.

Quoted 3 lines

If you only do local TLB maintenance, then the page is left accessible to other CPUs via the (stale) kernel mappings. i.e. the page isn't exclusively mapped by userspace.

We flush all CPUs to get rid of stale entries when a new page is allocated to userspace that was previously allocated to the kernel. Is that the scenario you were thinking of?

...Juerg

Quoted 3 lines

Thanks,
Mark.

Quoted 18 lines · Juerg Haefliger

Quoted 15 lines · Mark Rutland

Quoted 10 lines · Tycho Andersen

Hi Mark,

Quoted 2 lines · Mark Rutland

That said, is there any reason not to use flush_tlb_kernel_range() directly?

So it turns out that there is a difference between __flush_tlb_one() and flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which I think is enough here).

That sounds suspicious; I don't think that __flush_tlb_one() is sufficient.

Quoted 7 lines

Quoted 3 lines

If you only do local TLB maintenance, then the page is left accessible to other CPUs via the (stale) kernel mappings. i.e. the page isn't exclusively mapped by userspace.

We flush all CPUs to get rid of stale entries when a new page is allocated to userspace that was previously allocated to the kernel. Is that the scenario you were thinking of?

I think there are two cases, the one you describe above, where the pages are first allocated, and a second one, where e.g. the pages are mapped into the kernel because of DMA or whatever. In the case you describe above, I think we're doing the right thing (which is why my test worked correctly, because it tested this case).

In the second case, when the pages are unmapped (i.e. the kernel is done doing DMA), do we need to flush the other CPUs TLBs? I think the current code is not quite correct, because if multiple tasks (CPUs) map the pages, only the TLB of the last one is flushed when the mapping is cleared, because the tlb is only flushed when ->mapcount drops to zero, leaving stale entries in the other TLBs. It's not clear to me what to do about this case.

Thoughts?

Tycho

Quoted 7 lines

...Juerg

Quoted 3 lines

Thanks,
Mark.

Quoted 33 lines · Tycho Andersen

Quoted 18 lines · Juerg Haefliger

Quoted 15 lines · Mark Rutland

Quoted 10 lines · Tycho Andersen

Hi Mark,

Quoted 2 lines · Mark Rutland

That said, is there any reason not to use flush_tlb_kernel_range() directly?

So it turns out that there is a difference between __flush_tlb_one() and flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which I think is enough here).

That sounds suspicious; I don't think that __flush_tlb_one() is sufficient.

Quoted 7 lines

Quoted 3 lines

If you only do local TLB maintenance, then the page is left accessible to other CPUs via the (stale) kernel mappings. i.e. the page isn't exclusively mapped by userspace.

We flush all CPUs to get rid of stale entries when a new page is allocated to userspace that was previously allocated to the kernel. Is that the scenario you were thinking of?

I think there are two cases, the one you describe above, where the pages are first allocated, and a second one, where e.g. the pages are mapped into the kernel because of DMA or whatever. In the case you describe above, I think we're doing the right thing (which is why my test worked correctly, because it tested this case).

Quoted 7 lines

In the second case, when the pages are unmapped (i.e. the kernel is done doing DMA), do we need to flush the other CPUs TLBs? I think the current code is not quite correct, because if multiple tasks (CPUs) map the pages, only the TLB of the last one is flushed when the mapping is cleared, because the tlb is only flushed when ->mapcount drops to zero, leaving stale entries in the other TLBs. It's not clear to me what to do about this case.

For this to happen, multiple CPUs need to have the same userspace page mapped at the same time. Is this a valid scenario?

Quoted 42 lines · Juerg Haefliger

Quoted 40 lines · Tycho Andersen

Quoted 18 lines · Juerg Haefliger

Quoted 15 lines · Mark Rutland

Quoted 10 lines · Tycho Andersen

Hi Mark,

Quoted 2 lines · Mark Rutland

That said, is there any reason not to use flush_tlb_kernel_range() directly?

So it turns out that there is a difference between __flush_tlb_one() and flush_tlb_kernel_range() on x86: flush_tlb_kernel_range() flushes all the TLBs via on_each_cpu(), where as __flush_tlb_one() only flushes the local TLB (which I think is enough here).

That sounds suspicious; I don't think that __flush_tlb_one() is sufficient.

Quoted 7 lines

Quoted 3 lines

If you only do local TLB maintenance, then the page is left accessible to other CPUs via the (stale) kernel mappings. i.e. the page isn't exclusively mapped by userspace.

We flush all CPUs to get rid of stale entries when a new page is allocated to userspace that was previously allocated to the kernel. Is that the scenario you were thinking of?

I think there are two cases, the one you describe above, where the pages are first allocated, and a second one, where e.g. the pages are mapped into the kernel because of DMA or whatever. In the case you describe above, I think we're doing the right thing (which is why my test worked correctly, because it tested this case).

In the second case, when the pages are unmapped (i.e. the kernel is done doing DMA), do we need to flush the other CPUs TLBs? I think the current code is not quite correct, because if multiple tasks (CPUs) map the pages, only the TLB of the last one is flushed when the mapping is cleared, because the tlb is only flushed when ->mapcount drops to zero, leaving stale entries in the other TLBs. It's not clear to me what to do about this case.

Quoted 2 lines

For this to happen, multiple CPUs need to have the same userspace page mapped at the same time. Is this a valid scenario?

I believe so. I think you could trigger that with a multi-threaded application running across several CPUs. All those threads would share the same page tables.

From a3a8f9da00bed910e086805f3e71b9e5e1b898b4 Mon Sep 17 00:00:00 2001

From: Juerg Haefliger <[email protected]>

Enable support for eXclusive Page Frame Ownership (XPFO) for arm64 and provide a hook for updating a single kernel page table entry (which is required by the generic XPFO code).

At the moment, only 64k page sizes are supported.

Signed-off-by Juerg Haefliger <[email protected]>

Tested-by Tycho Andersen <[email protected]>

---
 arch/arm64/Kconfig     |  1 +
 arch/arm64/mm/Makefile |  2 ++
 arch/arm64/mm/xpfo.c   | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

arch/arm64/Kconfig +1 -0

index dfd908630631..2ddae41e0793 100644
@@ -121,6 +121,7 @@ config ARM64
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
 	select THREAD_INFO_IN_TASK
+	select ARCH_SUPPORTS_XPFO if ARM64_64K_PAGES
 	help
 	  ARM 64-bit (AArch64) Linux support.
 

arch/arm64/mm/Makefile +2 -0

index 9b0ba191e48e..22e5cab543d8 100644
@@ -11,3 +11,5 @@ KASAN_SANITIZE_physaddr.o	+= n
 
 obj-$(CONFIG_KASAN)		+= kasan_init.o
 KASAN_SANITIZE_kasan_init.o	:= n
+
+obj-$(CONFIG_XPFO)		+= xpfo.o

arch/arm64/mm/xpfo.c new file +38 -0

new file mode 100644
index 000000000000..de03a652d48a
@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2017 Hewlett Packard Enterprise Development, L.P.
+ * Copyright (C) 2016 Brown University. All rights reserved.
+ *
+ * Authors:
+ *   Juerg Haefliger <[email protected]>
+ *   Vasileios P. Kemerlis <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/mm.h>
+#include <linux/module.h>
+
+#include <asm/tlbflush.h>
+
+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */
+static pte_t *lookup_address(unsigned long addr)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+
+	pgd = pgd_offset_k(addr);
+	if (pgd_none(*pgd))
+		return NULL;
+
+	BUG_ON(pgd_bad(*pgd));
+
+	pud = pud_offset(pgd, addr);
+	if (pud_none(*pud))
+		return NULL;
+

arch/arm64/mm/xpfo.c new file +16 -0

+
+/* Update a single kernel page table entry */
+inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
+{
+	pte_t *pte = lookup_address((unsigned long)kaddr);
+
+	set_pte(pte, pfn_pte(page_to_pfn(page), prot));
+}
+
+inline void xpfo_flush_kernel_page(struct page *page, int order)
+{
+	unsigned long kaddr = (unsigned long)page_address(page);
+	unsigned long size = PAGE_SIZE;
+
+	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);
+}

Quoted 6 lines · Tycho Andersen

From: Juerg Haefliger <[email protected]>

Enable support for eXclusive Page Frame Ownership (XPFO) for arm64 and provide a hook for updating a single kernel page table entry (which is required by the generic XPFO code).

Quoted 2 lines

At the moment, only 64k page sizes are supported.

Can you add a note somewhere explaining this limitation or what's on the TODO list?

Quoted 74 lines

Signed-off-by Juerg Haefliger <[email protected]>

Tested-by Tycho Andersen <[email protected]>

---
 arch/arm64/Kconfig     |  1 +
 arch/arm64/mm/Makefile |  2 ++
 arch/arm64/mm/xpfo.c   | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

arch/arm64/Kconfig +1 -0

index dfd908630631..2ddae41e0793 100644
@@ -121,6 +121,7 @@ config ARM64
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
 	select THREAD_INFO_IN_TASK
+	select ARCH_SUPPORTS_XPFO if ARM64_64K_PAGES
 	help
 	  ARM 64-bit (AArch64) Linux support.
 

arch/arm64/mm/Makefile +2 -0

index 9b0ba191e48e..22e5cab543d8 100644
@@ -11,3 +11,5 @@ KASAN_SANITIZE_physaddr.o	+= n
 
 obj-$(CONFIG_KASAN)		+= kasan_init.o
 KASAN_SANITIZE_kasan_init.o	:= n
+
+obj-$(CONFIG_XPFO)		+= xpfo.o

arch/arm64/mm/xpfo.c new file +38 -0

new file mode 100644
index 000000000000..de03a652d48a
@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2017 Hewlett Packard Enterprise Development, L.P.
+ * Copyright (C) 2016 Brown University. All rights reserved.
+ *
+ * Authors:
+ *   Juerg Haefliger <[email protected]>
+ *   Vasileios P. Kemerlis <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/mm.h>
+#include <linux/module.h>
+
+#include <asm/tlbflush.h>
+
+/*
+ * Lookup the page table entry for a virtual address and return a pointer to
+ * the entry. Based on x86 tree.
+ */
+static pte_t *lookup_address(unsigned long addr)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+
+	pgd = pgd_offset_k(addr);
+	if (pgd_none(*pgd))
+		return NULL;
+
+	BUG_ON(pgd_bad(*pgd));
+
+	pud = pud_offset(pgd, addr);
+	if (pud_none(*pud))
+		return NULL;
+

arch/arm64/mm/xpfo.c line 48

Quoted 10 lines

arch/arm64/mm/xpfo.c new file +10 -0

+	BUG_ON(pud_bad(*pud));
+
+	pmd = pmd_offset(pud, addr);
+	if (pmd_none(*pmd))
+		return NULL;
+
+	BUG_ON(pmd_bad(*pmd));
+
+	return pte_offset_kernel(pmd, addr);
+}

We already have much of this logic implemented for kernel_page_present in arch/arm64/mm/pageattr.c, we should move this into there and make this common, similar to x86

Quoted 17 lines

arch/arm64/mm/xpfo.c new file +16 -0

+
+/* Update a single kernel page table entry */
+inline void set_kpte(void *kaddr, struct page *page, pgprot_t prot)
+{
+	pte_t *pte = lookup_address((unsigned long)kaddr);
+
+	set_pte(pte, pfn_pte(page_to_pfn(page), prot));
+}
+
+inline void xpfo_flush_kernel_page(struct page *page, int order)
+{
+	unsigned long kaddr = (unsigned long)page_address(page);
+	unsigned long size = PAGE_SIZE;
+
+	flush_tlb_kernel_range(kaddr, kaddr + (1 << order) * size);
+}

From: Juerg Haefliger <[email protected]>

XPFO (eXclusive Page Frame Ownership) doesn't support section mappings yet, so disable it if XPFO is turned on.

Signed-off-by Juerg Haefliger <[email protected]>

Tested-by Tycho Andersen <[email protected]>

---
 arch/arm64/mm/mmu.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

arch/arm64/mm/mmu.c +12 -0

index f1eb15e0e864..38026b3ccb46 100644
@@ -176,6 +176,18 @@ static void alloc_init_cont_pte(pmd_t *pmd, unsigned long addr,
 	} while (addr = next, addr != end);
 }
 
+static inline bool use_section_mapping(unsigned long addr, unsigned long next,
+				unsigned long phys)
+{
+	if (IS_ENABLED(CONFIG_XPFO))
+		return false;
+
+	if (((addr | next | phys) & ~SECTION_MASK) != 0)
+		return false;
+
+	return true;
+}
+
 static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
 		     phys_addr_t phys, pgprot_t prot,
 		     phys_addr_t (*pgtable_alloc)(void), int flags)

Quoted 37 lines · Tycho Andersen

From: Juerg Haefliger <[email protected]>

XPFO (eXclusive Page Frame Ownership) doesn't support section mappings yet, so disable it if XPFO is turned on.

Signed-off-by Juerg Haefliger <[email protected]>

Tested-by Tycho Andersen <[email protected]>

---
 arch/arm64/mm/mmu.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

arch/arm64/mm/mmu.c +12 -0

index f1eb15e0e864..38026b3ccb46 100644
@@ -176,6 +176,18 @@ static void alloc_init_cont_pte(pmd_t *pmd, unsigned long addr,
 	} while (addr = next, addr != end);
 }
 
+static inline bool use_section_mapping(unsigned long addr, unsigned long next,
+				unsigned long phys)
+{
+	if (IS_ENABLED(CONFIG_XPFO))
+		return false;
+
+	if (((addr | next | phys) & ~SECTION_MASK) != 0)
+		return false;
+
+	return true;
+}
+
 static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
 		     phys_addr_t phys, pgprot_t prot,
 		     phys_addr_t (*pgtable_alloc)(void), int flags)

@@ -190,7 +202,7 @@ static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
 		next = pmd_addr_end(addr, end);
 

arch/arm64/mm/mmu.c static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end, · line 208

Quoted 7 lines

arch/arm64/mm/mmu.c +1 -1

 		/* try section mapping first */
-		if (((addr | next | phys) & ~SECTION_MASK) == 0 &&
+		if (use_section_mapping(addr, next, phys) &&
 		    (flags & NO_BLOCK_MAPPINGS) == 0) {
 			pmd_set_huge(pmd, phys, prot);
 

There is already similar logic to disable section mappings for debug_pagealloc at the start of map_mem, can you take advantage of that?

You're suggesting something like this instead? Seems to work fine.

Quoted 4 lines · Laura Abbott

Quoted 3 lines · Tycho Andersen

(quoted diff) +0 -0

@@ -190,7 +202,7 @@ static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
 		next = pmd_addr_end(addr, end);
 

Quoted 11 lines

Review static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end, · line 208

Quoted 7 lines

(quoted diff) +1 -1

 		/* try section mapping first */
-		if (((addr | next | phys) & ~SECTION_MASK) == 0 &&
+		if (use_section_mapping(addr, next, phys) &&
 		    (flags & NO_BLOCK_MAPPINGS) == 0) {
 			pmd_set_huge(pmd, phys, prot);
 

There is already similar logic to disable section mappings for debug_pagealloc at the start of map_mem, can you take advantage of that?

You're suggesting something like this instead? Seems to work fine.

arch/arm64/mm/mmu.c +0 -0

index 38026b3ccb46..3b2c17bbbf12 100644

Hi,

Quoted 25 lines · Tycho Andersen

Quoted 4 lines · Laura Abbott

Quoted 3 lines · Tycho Andersen

(quoted diff) +0 -0

@@ -190,7 +202,7 @@ static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
 		next = pmd_addr_end(addr, end);
 

Quoted 11 lines

Review static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end, · line 208

Quoted 7 lines

(quoted diff) +1 -1

 		/* try section mapping first */
-		if (((addr | next | phys) & ~SECTION_MASK) == 0 &&
+		if (use_section_mapping(addr, next, phys) &&
 		    (flags & NO_BLOCK_MAPPINGS) == 0) {
 			pmd_set_huge(pmd, phys, prot);
 

There is already similar logic to disable section mappings for debug_pagealloc at the start of map_mem, can you take advantage of that?

You're suggesting something like this instead? Seems to work fine.

arch/arm64/mm/mmu.c +0 -0

index 38026b3ccb46..3b2c17bbbf12 100644
@@ -434,6 +434,8 @@ static void __init map_mem(pgd_t *pgd)
 

arch/arm64/mm/mmu.c static void __init map_mem(pgd_t *pgd) · line 439

Quoted 5 lines

arch/arm64/mm/mmu.c +2 -0

 	if (debug_pagealloc_enabled())
 		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+	if (IS_ENABLED(CONFIG_XPFO))
+		flags |= NO_BLOCK_MAPPINGS;
 

IIUC, XPFO carves out individual pages just like DEBUG_PAGEALLOC, so you'll also need NO_CONT_MAPPINGS.

Quoted 28 lines · Mark Rutland

Hi,

Quoted 25 lines · Tycho Andersen

Quoted 4 lines · Laura Abbott

Quoted 3 lines · Tycho Andersen

(quoted diff) +0 -0

@@ -190,7 +202,7 @@ static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
 		next = pmd_addr_end(addr, end);
 

Quoted 11 lines

Review static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end, · line 208

Quoted 7 lines

(quoted diff) +1 -1

 		/* try section mapping first */
-		if (((addr | next | phys) & ~SECTION_MASK) == 0 &&
+		if (use_section_mapping(addr, next, phys) &&
 		    (flags & NO_BLOCK_MAPPINGS) == 0) {
 			pmd_set_huge(pmd, phys, prot);
 

There is already similar logic to disable section mappings for debug_pagealloc at the start of map_mem, can you take advantage of that?

You're suggesting something like this instead? Seems to work fine.

arch/arm64/mm/mmu.c +0 -0

index 38026b3ccb46..3b2c17bbbf12 100644
@@ -434,6 +434,8 @@ static void __init map_mem(pgd_t *pgd)
 

Quoted 8 lines

arch/arm64/mm/mmu.c static void __init map_mem(pgd_t *pgd) · line 439

Quoted 5 lines

arch/arm64/mm/mmu.c +2 -0

 	if (debug_pagealloc_enabled())
 		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+	if (IS_ENABLED(CONFIG_XPFO))
+		flags |= NO_BLOCK_MAPPINGS;
 

IIUC, XPFO carves out individual pages just like DEBUG_PAGEALLOC, so you'll also need NO_CONT_MAPPINGS.

Yes, thanks!

Tycho

Quoted 38 lines · Tycho Andersen

Quoted 36 lines · Mark Rutland

Hi,

Quoted 24 lines · Tycho Andersen

Quoted 4 lines · Laura Abbott

Quoted 3 lines · Tycho Andersen

(quoted diff) +0 -0

@@ -190,7 +202,7 @@ static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
 		next = pmd_addr_end(addr, end);
 

Quoted 11 lines

Review static void init_pmd(pud_t *pud, unsigned long addr, unsigned long end, · line 208

Quoted 7 lines

(quoted diff) +1 -1

 		/* try section mapping first */
-		if (((addr | next | phys) & ~SECTION_MASK) == 0 &&
+		if (use_section_mapping(addr, next, phys) &&
 		    (flags & NO_BLOCK_MAPPINGS) == 0) {
 			pmd_set_huge(pmd, phys, prot);
 

There is already similar logic to disable section mappings for debug_pagealloc at the start of map_mem, can you take advantage of that?

You're suggesting something like this instead? Seems to work fine.

arch/arm64/mm/mmu.c +0 -0

index 38026b3ccb46..3b2c17bbbf12 100644

arch/arm64/mm/mmu.c static void __init map_mem(pgd_t *pgd) · line 439

Quoted 6 lines

arch/arm64/mm/mmu.c +2 -0

@@ -434,6 +434,8 @@ static void __init map_mem(pgd_t *pgd)
 
 	if (debug_pagealloc_enabled())
 		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
+	if (IS_ENABLED(CONFIG_XPFO))
+		flags |= NO_BLOCK_MAPPINGS;
 

IIUC, XPFO carves out individual pages just like DEBUG_PAGEALLOC, so you'll also need NO_CONT_MAPPINGS.

Quoted 4 lines

Yes, thanks!

Tycho

Setting NO_CONT_MAPPINGS fixes the TLB conflict aborts I was seeing on my machine.

From: Juerg Haefliger <[email protected]>

If the page is unmapped by XPFO, a data cache flush results in a fatal page fault. So don't flush in that case.

Quoted 4 lines · Tycho Andersen

From: Juerg Haefliger <[email protected]>

If the page is unmapped by XPFO, a data cache flush results in a fatal page fault. So don't flush in that case.

Do you have an example callchain where that happens? We might need to shuffle things around to cater for that case.

arch/arm64/mm/flush.c void sync_icache_aliases(void *kaddr, unsigned long len) · line 39

Quoted 11 lines

arch/arm64/mm/flush.c +3 -1

@@ -30,7 +31,9 @@ void sync_icache_aliases(void *kaddr, unsigned long len)
 	unsigned long addr = (unsigned long)kaddr;
 
 	if (icache_is_aliasing()) {
-		__clean_dcache_area_pou(kaddr, len);
+		/* Don't flush if the page is unmapped by XPFO */
+		if (!xpfo_page_is_unmapped(virt_to_page(kaddr)))
+			__clean_dcache_area_pou(kaddr, len);
 		__flush_icache_all();
 	} else {
 		flush_icache_range(addr, addr + len);

I don't think this patch is correct. If data cache maintenance is required in the absence of XPFO, I don't see why it wouldn't be required in the presence of XPFO.

I'm not immediately sure why the non-aliasing case misses data cache maintenance. I couldn't spot where that happens otherwise.

From another look, it seems that flush_icache_range() performs the

Quoted 4 lines · Tycho Andersen

From: Juerg Haefliger <[email protected]>

If the page is unmapped by XPFO, a data cache flush results in a fatal page fault. So don't flush in that case.

Do you have an example callchain where that happens? We might need to shuffle things around to cater for that case.

arch/arm64/mm/flush.c void sync_icache_aliases(void *kaddr, unsigned long len) · line 39

Quoted 11 lines

arch/arm64/mm/flush.c +3 -1

@@ -30,7 +31,9 @@ void sync_icache_aliases(void *kaddr, unsigned long len)
 	unsigned long addr = (unsigned long)kaddr;
 
 	if (icache_is_aliasing()) {
-		__clean_dcache_area_pou(kaddr, len);
+		/* Don't flush if the page is unmapped by XPFO */
+		if (!xpfo_page_is_unmapped(virt_to_page(kaddr)))
+			__clean_dcache_area_pou(kaddr, len);
 		__flush_icache_all();
 	} else {
 		flush_icache_range(addr, addr + len);

I don't think this patch is correct. If data cache maintenance is required in the absence of XPFO, I don't see why it wouldn't be required in the presence of XPFO.

I'm not immediately sure why the non-aliasing case misses data cache maintenance. I couldn't spot where that happens otherwise.

On a more general note, in future it would be good to Cc the arm64 maintainers and the linux-arm-kernel mailing list for patches affecting arm64.

Hi Mark,

First, thanks for taking a look!

Quoted 8 lines · Mark Rutland

Quoted 4 lines · Tycho Andersen

From: Juerg Haefliger <[email protected]>

If the page is unmapped by XPFO, a data cache flush results in a fatal page fault. So don't flush in that case.

Do you have an example callchain where that happens? We might need to shuffle things around to cater for that case.

Here's one from the other branch (i.e. xpfo_page_is_unmapped() is true):

[   15.487293] CPU: 2 PID: 1633 Comm: plymouth Not tainted 4.13.0-rc4-c2+ #242
[   15.487295] Hardware name: Hardkernel ODROID-C2 (DT)
[   15.487297] Call trace:
[   15.487313] [<ffff0000080884f0>] dump_backtrace+0x0/0x248
[   15.487317] [<ffff00000808878c>] show_stack+0x14/0x20
[   15.487324] [<ffff000008b3e1b8>] dump_stack+0x98/0xb8
[   15.487329] [<ffff000008098bb4>] sync_icache_aliases+0x84/0x98
[   15.487332] [<ffff000008098c74>] __sync_icache_dcache+0x64/0x88
[   15.487337] [<ffff0000081d4814>] alloc_set_pte+0x4ec/0x6b8
[   15.487342] [<ffff00000819d920>] filemap_map_pages+0x350/0x360
[   15.487344] [<ffff0000081d4ccc>] do_fault+0x28c/0x568
[   15.487347] [<ffff0000081d67a0>] __handle_mm_fault+0x410/0xd08
[   15.487350] [<ffff0000081d7164>] handle_mm_fault+0xcc/0x1a8
[   15.487352] [<ffff000008098580>] do_page_fault+0x270/0x380
[   15.487355] [<ffff00000808128c>] do_mem_abort+0x3c/0x98
[   15.487358] Exception stack(0xffff800061dabe20 to 0xffff800061dabf50)
[   15.487362] be20: 0000000000000000 0000800062e19000 ffffffffffffffff 0000ffff8f64ddc8
[   15.487365] be40: ffff800061dabe80 ffff000008238810 ffff800061d80330 0000000000000018
[   15.487368] be60: ffffffffffffffff 0000ffff8f5ba958 ffff800061d803d0 ffff800067132e18
[   15.487370] be80: 0000000000000000 ffff800061d80d08 0000000000000000 0000000000000019
[   15.487373] bea0: 000000002bd3d0f0 0000000000000000 0000000000000019 ffff800067132e00
[   15.487376] bec0: 0000000000000000 0000ffff8f657220 0000000000000000 0000000000000000
[   15.487379] bee0: 8080808000000000 0000000000000000 0000000080808080 fefefeff6f6b6467
[   15.487381] bf00: 7f7f7f7f7f7f7f7f 000000002bd3fb40 0101010101010101 0000000000000020
[   15.487384] bf20: 00000000004072b0 00000000004072e0 0000000000000000 0000ffff8f6b2000
[   15.487386] bf40: 0000ffff8f66b190 0000ffff8f576380
[   15.487389] [<ffff000008082b74>] el0_da+0x20/0x24

Quoted 15 lines

arch/arm64/mm/flush.c void sync_icache_aliases(void *kaddr, unsigned long len) · line 39

Quoted 11 lines

arch/arm64/mm/flush.c +3 -1

@@ -30,7 +31,9 @@ void sync_icache_aliases(void *kaddr, unsigned long len)
 	unsigned long addr = (unsigned long)kaddr;
 
 	if (icache_is_aliasing()) {
-		__clean_dcache_area_pou(kaddr, len);
+		/* Don't flush if the page is unmapped by XPFO */
+		if (!xpfo_page_is_unmapped(virt_to_page(kaddr)))
+			__clean_dcache_area_pou(kaddr, len);
 		__flush_icache_all();
 	} else {
 		flush_icache_range(addr, addr + len);

I don't think this patch is correct. If data cache maintenance is required in the absence of XPFO, I don't see why it wouldn't be required in the presence of XPFO.

Ok. I suppose we could do re-map like we do for dma; or is there some re-arrangement of things you can see that would help?

Quoted 2 lines

On a more general note, in future it would be good to Cc the arm64 maintainers and the linux-arm-kernel mailing list for patches affecting arm64.

Yes, I thought about doing that for the series, but since it has x86 patches too, I didn't want to spam everyone :). I'll just add x86/arm lists to CC in the patches in the future. If there's some better way, let me know.

From: Juerg Haefliger <[email protected]>

Pages that are unmapped by XPFO need to be mapped before and unmapped again after (to restore the original state) the __dma_{map,unmap}_area() operations to prevent fatal page faults.

Signed-off-by Juerg Haefliger <[email protected]>

Signed-off-by Tycho Andersen <[email protected]>

---
 arch/arm64/include/asm/cacheflush.h | 11 +++++++++
 arch/arm64/mm/dma-mapping.c         | 32 +++++++++++++-------------
 arch/arm64/mm/xpfo.c                | 45 +++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 16 deletions(-)

arch/arm64/include/asm/cacheflush.h +11 -0

index d74a284abdc2..b6a462e3b2f9 100644
@@ -93,6 +93,17 @@ extern void __dma_map_area(const void *, size_t, int);
 extern void __dma_unmap_area(const void *, size_t, int);
 extern void __dma_flush_area(const void *, size_t);
 
+#ifdef CONFIG_XPFO
+#include <linux/xpfo.h>
+#define _dma_map_area(addr, size, dir) \
+	xpfo_dma_map_unmap_area(true, addr, size, dir)
+#define _dma_unmap_area(addr, size, dir) \
+	xpfo_dma_map_unmap_area(false, addr, size, dir)
+#else
+#define _dma_map_area(addr, size, dir) __dma_map_area(addr, size, dir)
+#define _dma_unmap_area(addr, size, dir) __dma_unmap_area(addr, size, dir)
+#endif
+
 /*
  * Copy user data from/to a page which is mapped into a different
  * processes address space.  Really, we want to allow our "user

arch/arm64/mm/dma-mapping.c +16 -16

index f27d4dd04384..a79f200786ab 100644
@@ -204,7 +204,7 @@ static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
 	dev_addr = swiotlb_map_page(dev, page, offset, size, dir, attrs);
 	if (!is_device_dma_coherent(dev) &&
 	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
-		__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
+		_dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
 
 	return dev_addr;
 }

@@ -216,7 +216,7 @@ static void __swiotlb_unmap_page(struct device *dev, dma_addr_t dev_addr,
 {
 	if (!is_device_dma_coherent(dev) &&
 	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
-		__dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
+		_dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
 	swiotlb_unmap_page(dev, dev_addr, size, dir, attrs);
 }
 

@@ -231,8 +231,8 @@ static int __swiotlb_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
 	if (!is_device_dma_coherent(dev) &&
 	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
 		for_each_sg(sgl, sg, ret, i)
-			__dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
-				       sg->length, dir);
+			_dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
+				      sg->length, dir);
 
 	return ret;
 }

@@ -248,8 +248,8 @@ static void __swiotlb_unmap_sg_attrs(struct device *dev,
 	if (!is_device_dma_coherent(dev) &&
 	    (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
 		for_each_sg(sgl, sg, nelems, i)
-			__dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
-					 sg->length, dir);
+			_dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
+					sg->length, dir);
 	swiotlb_unmap_sg_attrs(dev, sgl, nelems, dir, attrs);
 }
 

@@ -258,7 +258,7 @@ static void __swiotlb_sync_single_for_cpu(struct device *dev,
 					  enum dma_data_direction dir)
 {
 	if (!is_device_dma_coherent(dev))
-		__dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
+		_dma_unmap_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
 	swiotlb_sync_single_for_cpu(dev, dev_addr, size, dir);
 }
 

@@ -268,7 +268,7 @@ static void __swiotlb_sync_single_for_device(struct device *dev,
 {
 	swiotlb_sync_single_for_device(dev, dev_addr, size, dir);
 	if (!is_device_dma_coherent(dev))
-		__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
+		_dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
 }
 
 static void __swiotlb_sync_sg_for_cpu(struct device *dev,

@@ -280,8 +280,8 @@ static void __swiotlb_sync_sg_for_cpu(struct device *dev,
 
 	if (!is_device_dma_coherent(dev))
 		for_each_sg(sgl, sg, nelems, i)
-			__dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
-					 sg->length, dir);
+			_dma_unmap_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
+					sg->length, dir);
 	swiotlb_sync_sg_for_cpu(dev, sgl, nelems, dir);
 }
 

@@ -295,8 +295,8 @@ static void __swiotlb_sync_sg_for_device(struct device *dev,
 	swiotlb_sync_sg_for_device(dev, sgl, nelems, dir);
 	if (!is_device_dma_coherent(dev))
 		for_each_sg(sgl, sg, nelems, i)
-			__dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
-				       sg->length, dir);
+			_dma_map_area(phys_to_virt(dma_to_phys(dev, sg->dma_address)),
+				      sg->length, dir);
 }
 
 static int __swiotlb_mmap_pfn(struct vm_area_struct *vma,

@@ -758,7 +758,7 @@ static void __iommu_sync_single_for_cpu(struct device *dev,
 		return;
 
 	phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), dev_addr);
-	__dma_unmap_area(phys_to_virt(phys), size, dir);
+	_dma_unmap_area(phys_to_virt(phys), size, dir);
 }
 
 static void __iommu_sync_single_for_device(struct device *dev,

@@ -771,7 +771,7 @@ static void __iommu_sync_single_for_device(struct device *dev,
 		return;
 
 	phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), dev_addr);
-	__dma_map_area(phys_to_virt(phys), size, dir);
+	_dma_map_area(phys_to_virt(phys), size, dir);
 }
 
 static dma_addr_t __iommu_map_page(struct device *dev, struct page *page,

@@ -811,7 +811,7 @@ static void __iommu_sync_sg_for_cpu(struct device *dev,
 		return;
 
 	for_each_sg(sgl, sg, nelems, i)
-		__dma_unmap_area(sg_virt(sg), sg->length, dir);
+		_dma_unmap_area(sg_virt(sg), sg->length, dir);
 }
 
 static void __iommu_sync_sg_for_device(struct device *dev,

@@ -825,7 +825,7 @@ static void __iommu_sync_sg_for_device(struct device *dev,
 		return;
 
 	for_each_sg(sgl, sg, nelems, i)
-		__dma_map_area(sg_virt(sg), sg->length, dir);
+		_dma_map_area(sg_virt(sg), sg->length, dir);
 }
 
 static int __iommu_map_sg_attrs(struct device *dev, struct scatterlist *sgl,

arch/arm64/mm/xpfo.c +2 -0

index de03a652d48a..c4deb2b720cf 100644
@@ -11,8 +11,10 @@
  * the Free Software Foundation.
  */
 
+#include <linux/highmem.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/xpfo.h>
 
 #include <asm/tlbflush.h>
 

arch/arm64/mm/xpfo.c +21 -0

+
+	/* Map the remaining pages */
+	for (i = 1; i < possible_pages; i++) {
+		if (page_to_virt(page + i) >= addr + size)
+			break;
+
+		if (xpfo_page_is_unmapped(page + i))
+			buf[i] = kmap_atomic(page + i);
+	}
+
+	if (map)
+		__dma_map_area(addr, size, dir);
+	else
+		__dma_unmap_area(addr, size, dir);
+
+	for (i = 0; i < possible_pages; i++)
+		if (buf[i])
+			kunmap_atomic(buf[i]);
+
+	local_irq_restore(flags);
+}

Signature · 1 line

2.11.0

arch/arm64/mm/xpfo.c inline void xpfo_flush_kernel_page(struct page *page, int order) · line 68

Quoted 2 lines · Tycho Andersen

arch/arm64/mm/xpfo.c +2 -0

+
+inline void xpfo_dma_map_unmap_area(bool map, const void *addr, size_t size,

And inline? You sure about that? It is quite a lot of code to duplicate in all of those call-sites.

arch/arm64/mm/xpfo.c inline void xpfo_flush_kernel_page(struct page *page, int order) · line 69

Quoted 1 line

arch/arm64/mm/xpfo.c +1 -0

+				    int dir)

Not enum dma_data_direction ?

arch/arm64/mm/xpfo.c inline void xpfo_flush_kernel_page(struct page *page, int order) · line 79

Quoted 10 lines

arch/arm64/mm/xpfo.c +10 -0

+{
+	unsigned long flags;
+	struct page *page = virt_to_page(addr);
+
+	/*
+	 * +2 here because we really want
+	 * ceil(size / PAGE_SIZE), not floor(), and one extra in case things are
+	 * not page aligned
+	 */
+	int i, possible_pages = size / PAGE_SIZE + 2;

Could you use the PAGE_SHIFT macro instead? Or PFN_UP ?

And there is also the PAGE_ALIGN macro...

arch/arm64/mm/xpfo.c inline void xpfo_flush_kernel_page(struct page *page, int order) · line 80

Quoted 1 line

arch/arm64/mm/xpfo.c +1 -0

+	void *buf[possible_pages];

What if you just did 'void *buf[possible_pages] = { };'

Wouldn't that eliminate the need for the memset?

arch/arm64/mm/xpfo.c inline void xpfo_flush_kernel_page(struct page *page, int order) · line 84

Quoted 4 lines

arch/arm64/mm/xpfo.c +4 -0

+
+	memset(buf, 0, sizeof(void *) * possible_pages);
+
+	local_irq_save(flags);

?? Why?