x86: hold mm->page_table_lock while doing vmalloc_sync

Take mm->page_table_lock while syncing the vmalloc region. This prevents a race with the Xen pagetable pin/unpin code, which expects that the page_table_lock is already held. If this race occurs, then Xen can see an inconsistent page type (a page can either be read/write or a pagetable page, and pin/unpin converts it between them), which will cause either the pin or the set_p[gm]d to fail; either will crash the kernel.

Commit-ID:  617d34d9e5d8326ec8f188c616aa06ac59d083fe
Gitweb:     http://git.kernel.org/tip/617d34d9e5d8326ec8f188c616aa06ac59d083fe
Author:     Jeremy Fitzhardinge <[email protected]>
AuthorDate: Tue, 21 Sep 2010 12:01:51 -0700
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Tue, 19 Oct 2010 13:57:08 -0700

Commit-ID:  bfac67208d6d2a84fe7980187d3d2f21424dde59
Gitweb:     http://git.kernel.org/tip/bfac67208d6d2a84fe7980187d3d2f21424dde59
Author:     tip-bot for Jeremy Fitzhardinge <[email protected]>

Quoted 3 lines

Commit-ID:  bfac67208d6d2a84fe7980187d3d2f21424dde59
Gitweb:     http://git.kernel.org/tip/bfac67208d6d2a84fe7980187d3d2f21424dde59
Author:     tip-bot for Jeremy Fitzhardinge <[email protected]>

Looks like git (or your script) picked up the first From: in the mail as author...

Quoted 2 lines

Commit-ID: 617d34d9e5d8326ec8f188c616aa06ac59d083fe Gitweb:

Quoted 4 lines

Author:     Jeremy Fitzhardinge <[email protected]>
AuthorDate: Tue, 21 Sep 2010 12:01:51 -0700
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Tue, 19 Oct 2010 13:57:08 -0700

(This is a constant problem, by the way: people seems to want to prefix, not suffix, their commentary; "cover letter" style.) I suspect git needs to change, but it's hard to know exactly how to change it.

From: "H. Peter Anvin" <[email protected]>
Date: Wed, Oct 20, 2010 at 12:53:43PM -0700

Quoted 3 lines

(This is a constant problem, by the way: people seems to want to prefix, not suffix, their commentary; "cover letter" style.) I suspect git needs to change, but it's hard to know exactly how to change it.

That's easy, remove the cover letter style when inlining patches further down the mail.

But yeah, I'm still trying to decide for me which mail attribution format (set attribution="..." in mutt) is better/clearer:

From: foo
Date: bar

or

On <date>, <person> said:

Hmm...

... and that confused the scripts. The way git wants it commentary information should go *after* the "---" line.

(This is a constant problem, by the way: people seems to want to prefix, not suffix, their commentary; "cover letter" style.) I suspect git needs to change, but it's hard to know exactly how to change it.

Quoted 5 lines

From: foo
Date: bar

or

Quoted 4 lines · On <date>, <person> said:

Hmm...

Well, the latter can at least be edited out.

The former *really* gunks up git.

Take mm->page_table_lock while syncing the vmalloc region. This prevents a race with the Xen pagetable pin/unpin code, which expects that the page_table_lock is already held. If this race occurs, then Xen can see an inconsistent page type (a page can either be read/write or a pagetable page, and pin/unpin converts it between them), which will cause either the pin or the set_p[gm]d to fail; either will crash the kernel.

vmalloc_sync_all() should be called rarely, so this extra use of page_table_lock should not interfere with its normal users.

The mm pointer is stashed in the pgd page's index field, as that won't be otherwise used for pgd pages.

Bug reported by Ian Campbell <[email protected]> Derived from a patch by Jan Beulich <[email protected]>

Signed-off-by Jeremy Fitzhardinge <[email protected]>

arch/x86/include/asm/pgtable.h +2 -0

index a34c785..422b363 100644
@@ -28,6 +28,8 @@ extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
 extern spinlock_t pgd_lock;
 extern struct list_head pgd_list;
 
+extern struct mm_struct *pgd_page_get_mm(struct page *page);
+
 #ifdef CONFIG_PARAVIRT
 #include <asm/paravirt.h>
 #else  /* !CONFIG_PARAVIRT */

arch/x86/mm/fault.c +18 -1

index 4c4508e..b7f9ae1 100644
@@ -229,7 +229,16 @@ void vmalloc_sync_all(void)
 
 		spin_lock_irqsave(&pgd_lock, flags);
 		list_for_each_entry(page, &pgd_list, lru) {
-			if (!vmalloc_sync_one(page_address(page), address))
+			spinlock_t *pgt_lock;
+			int ret;
+
+			pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
+
+			spin_lock(pgt_lock);
+			ret = vmalloc_sync_one(page_address(page), address);
+			spin_unlock(pgt_lock);
+
+			if (!ret)
 				break;
 		}
 		spin_unlock_irqrestore(&pgd_lock, flags);

@@ -341,11 +350,19 @@ void vmalloc_sync_all(void)
 		spin_lock_irqsave(&pgd_lock, flags);
 		list_for_each_entry(page, &pgd_list, lru) {
 			pgd_t *pgd;
+			spinlock_t *pgt_lock;
+
 			pgd = (pgd_t *)page_address(page) + pgd_index(address);
+
+			pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
+			spin_lock(pgt_lock);
+
 			if (pgd_none(*pgd))
 				set_pgd(pgd, *pgd_ref);
 			else
 				BUG_ON(pgd_page_vaddr(*pgd) != pgd_page_vaddr(*pgd_ref));
+
+			spin_unlock(pgt_lock);
 		}
 		spin_unlock_irqrestore(&pgd_lock, flags);
 	}

arch/x86/mm/pgtable.c +17 -3

index 5c4ee42..c70e57d 100644
@@ -87,7 +87,19 @@ static inline void pgd_list_del(pgd_t *pgd)
 #define UNSHARED_PTRS_PER_PGD				\
 	(SHARED_KERNEL_PMD ? KERNEL_PGD_BOUNDARY : PTRS_PER_PGD)
 
-static void pgd_ctor(pgd_t *pgd)
+
+static void pgd_set_mm(pgd_t *pgd, struct mm_struct *mm)
+{
+	BUILD_BUG_ON(sizeof(virt_to_page(pgd)->index) < sizeof(mm));
+	virt_to_page(pgd)->index = (pgoff_t)mm;
+}
+
+struct mm_struct *pgd_page_get_mm(struct page *page)
+{
+	return (struct mm_struct *)page->index;
+}
+
+static void pgd_ctor(struct mm_struct *mm, pgd_t *pgd)
 {
 	/* If the pgd points to a shared pagetable level (either the
 	   ptes in non-PAE, or shared PMD in PAE), then just copy the

@@ -105,8 +117,10 @@ static void pgd_ctor(pgd_t *pgd)
 	}
 
 	/* list required to sync kernel mapping updates */
-	if (!SHARED_KERNEL_PMD)
+	if (!SHARED_KERNEL_PMD) {
+		pgd_set_mm(pgd, mm);
 		pgd_list_add(pgd);
+	}
 }
 
 static void pgd_dtor(pgd_t *pgd)

@@ -272,7 +286,7 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
 	 */
 	spin_lock_irqsave(&pgd_lock, flags);
 
-	pgd_ctor(pgd);
+	pgd_ctor(mm, pgd);
 	pgd_prepopulate_pmd(mm, pgd, pmds);
 
 	spin_unlock_irqrestore(&pgd_lock, flags);

Ping? Have you had any thoughts about possible x86-64 problems with this?

Thanks,
J

Quoted 4 lines · Jeremy Fitzhardinge

Ping? Have you had any thoughts about possible x86-64 problems with this?

Thanks,
J

I didn't find any, so I already committed your patch to tip:x86/mm two days ago.

Now, it's worth noting that Linus has asked us to get rid of vmalloc_sync_all() completely, which would be a good thing in the longer term.

Hello,

Larry (CC'ed) found a problem with the patch in subject. When USE_SPLIT_PTLOCKS is not defined (NR_CPUS == 2) it will deadlock in ptep_clear_flush_notify in rmap.c because it's sending IPIs with the page_table_lock already held, and the other CPUs now spins on the page_table_lock with irq disabled, so the IPI never runs. With CONFIG_TRANSPARENT_HUGEPAGE=y this deadlocks happens even with USE_SPLIT_PTLOCKS defined so it become visible but it needs to be fixed regardless (for NR_CPUS == 2).

I'd like to understand why the pgd_lock needs irq disabled, it sounds too easy that I can just remove the _irqsave, doesn't it?

A pgd_free comment says it can run from irq. page_table_lock having to be taken there is for Xen only, but other archs also uses spin_lock_irqsave(pgd_lock) so I guess it's either common code, or it's superfluous and not another Xen special requirement.

If we could remove that _irqsave like below it'd solve it... But clearly something must be taking the pgd_lock from irq. (using a rwlock would also be possible as long as nobody takes it in write mode during irq, but if it's pgd_free that really runs in irq, that would need the write_lock so it wouldn't be a solution).

I'm trying this fix and the VM_BUG_ON never triggered yet.

In short: who takes the pgd_lock from an irq? (__mmdrop shouldn't but maybe I'm overlooking some aio bit?)

======
Subject: fix pgd_lock deadlock

From: Andrea Arcangeli <[email protected]>

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

Apparently nobody takes the pgd_lock from irq so the _irqsave can be removed.

Signed-off-by Andrea Arcangeli <[email protected]>

---
 arch/x86/mm/fault.c    |    7 ++++---
 arch/x86/mm/init_64.c  |    7 ++++---
 arch/x86/mm/pageattr.c |   21 +++++++++++----------
 arch/x86/mm/pgtable.c  |   10 ++++++----
 arch/x86/xen/mmu.c     |   10 ++++------
 5 files changed, 29 insertions(+), 26 deletions(-)

arch/x86/mm/fault.c +3 -2

@@ -230,14 +230,15 @@ void vmalloc_sync_all(void)
 	     address >= TASK_SIZE && address < FIXADDR_TOP;
 	     address += PMD_SIZE) {
 
-		unsigned long flags;
 		struct page *page;
 
-		spin_lock_irqsave(&pgd_lock, flags);
+		VM_BUG_ON(in_interrupt());
+		spin_lock(&pgd_lock);
 		list_for_each_entry(page, &pgd_list, lru) {
 			spinlock_t *pgt_lock;
 			pmd_t *ret;
 
+			/* the pgt_lock only for Xen */
 			pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
 
 			spin_lock(pgt_lock);

arch/x86/mm/fault.c +0 -0

 	}
 }
 

arch/x86/mm/init_64.c +4 -3

@@ -105,18 +105,19 @@ void sync_global_pgds(unsigned long star
 
 	for (address = start; address <= end; address += PGDIR_SIZE) {
 		const pgd_t *pgd_ref = pgd_offset_k(address);
-		unsigned long flags;
 		struct page *page;
 
 		if (pgd_none(*pgd_ref))
 			continue;
 
-		spin_lock_irqsave(&pgd_lock, flags);
+		VM_BUG_ON(in_interrupt());
+		spin_lock(&pgd_lock);
 		list_for_each_entry(page, &pgd_list, lru) {
 			pgd_t *pgd;
 			spinlock_t *pgt_lock;
 
 			pgd = (pgd_t *)page_address(page) + pgd_index(address);
+			/* the pgt_lock only for Xen */
 			pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
 			spin_lock(pgt_lock);
 

@@ -128,7 +129,7 @@ void sync_global_pgds(unsigned long star
 
 			spin_unlock(pgt_lock);
 		}
-		spin_unlock_irqrestore(&pgd_lock, flags);
+		spin_unlock(&pgd_lock);
 	}
 }
 

arch/x86/mm/pageattr.c +11 -10

@@ -57,12 +57,11 @@ static unsigned long direct_pages_count[
 
 void update_page_count(int level, unsigned long pages)
 {
-	unsigned long flags;
-
 	/* Protect against CPA */
-	spin_lock_irqsave(&pgd_lock, flags);
+	VM_BUG_ON(in_interrupt());
+	spin_lock(&pgd_lock);
 	direct_pages_count[level] += pages;
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 }
 
 static void split_page_count(int level)

@@ -402,7 +401,7 @@ static int
 try_preserve_large_page(pte_t *kpte, unsigned long address,
 			struct cpa_data *cpa)
 {
-	unsigned long nextpage_addr, numpages, pmask, psize, flags, addr, pfn;
+	unsigned long nextpage_addr, numpages, pmask, psize, addr, pfn;
 	pte_t new_pte, old_pte, *tmp;
 	pgprot_t old_prot, new_prot, req_prot;
 	int i, do_split = 1;

@@ -411,7 +410,8 @@ try_preserve_large_page(pte_t *kpte, uns
 	if (cpa->force_split)
 		return 1;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	VM_BUG_ON(in_interrupt());
+	spin_lock(&pgd_lock);
 	/*
 	 * Check for races, another CPU might have split this page
 	 * up already:

@@ -506,14 +506,14 @@ try_preserve_large_page(pte_t *kpte, uns
 	}
 
 out_unlock:
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 
 	return do_split;
 }
 
 static int split_large_page(pte_t *kpte, unsigned long address)
 {
-	unsigned long flags, pfn, pfninc = 1;
+	unsigned long pfn, pfninc = 1;
 	unsigned int i, level;
 	pte_t *pbase, *tmp;
 	pgprot_t ref_prot;

@@ -527,7 +527,8 @@ static int split_large_page(pte_t *kpte,
 	if (!base)
 		return -ENOMEM;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	VM_BUG_ON(in_interrupt());
+	spin_lock(&pgd_lock);
 	/*
 	 * Check for races, another CPU might have split this page
 	 * up for us already:

@@ -599,7 +600,7 @@ out_unlock:
 	 */
 	if (base)
 		__free_page(base);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 
 	return 0;
 }

arch/x86/mm/pgtable.c +6 -4

@@ -126,9 +126,10 @@ static void pgd_dtor(pgd_t *pgd)
 	if (SHARED_KERNEL_PMD)
 		return;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	VM_BUG_ON(in_interrupt());
+	spin_lock(&pgd_lock);
 	pgd_list_del(pgd);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 }
 
 /*

@@ -280,12 +281,13 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
 	 * respect to anything walking the pgd_list, so that they
 	 * never see a partially populated pgd.
 	 */
-	spin_lock_irqsave(&pgd_lock, flags);
+	VM_BUG_ON(in_interrupt());
+	spin_lock(&pgd_lock);
 
 	pgd_ctor(mm, pgd);
 	pgd_prepopulate_pmd(mm, pgd, pmds);
 
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 
 	return pgd;
 

arch/x86/xen/mmu.c +4 -6

@@ -986,10 +986,9 @@ static void xen_pgd_pin(struct mm_struct
  */
 void xen_mm_pin_all(void)
 {
-	unsigned long flags;
 	struct page *page;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock(&pgd_lock);
 
 	list_for_each_entry(page, &pgd_list, lru) {
 		if (!PagePinned(page)) {

@@ -998,7 +997,7 @@ void xen_mm_pin_all(void)
 		}
 	}
 
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 }
 
 /*

@@ -1099,10 +1098,9 @@ static void xen_pgd_unpin(struct mm_stru
  */
 void xen_mm_unpin_all(void)
 {
-	unsigned long flags;
 	struct page *page;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock(&pgd_lock);
 
 	list_for_each_entry(page, &pgd_list, lru) {
 		if (PageSavePinned(page)) {

@@ -1112,7 +1110,7 @@ void xen_mm_unpin_all(void)
 		}
 	}
 
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock(&pgd_lock);
 }
 
 void xen_activate_mm(struct mm_struct *prev, struct mm_struct *next)

Hello,

Larry (CC'ed) found a problem with the patch in subject. When USE_SPLIT_PTLOCKS is not defined (NR_CPUS == 2) it will deadlock in ptep_clear_flush_notify in rmap.c because it's sending IPIs with the page_table_lock already held, and the other CPUs now spins on the page_table_lock with irq disabled, so the IPI never runs. With CONFIG_TRANSPARENT_HUGEPAGE=y this deadlocks happens even with USE_SPLIT_PTLOCKS defined so it become visible but it needs to be fixed regardless (for NR_CPUS == 2).

I'd like to understand why the pgd_lock needs irq disabled, it sounds too easy that I can just remove the _irqsave, doesn't it?

A pgd_free comment says it can run from irq. page_table_lock having to be taken there is for Xen only, but other archs also uses spin_lock_irqsave(pgd_lock) so I guess it's either common code, or it's superfluous and not another Xen special requirement.

If we could remove that _irqsave like below it'd solve it... But clearly something must be taking the pgd_lock from irq. (using a rwlock would also be possible as long as nobody takes it in write mode during irq, but if it's pgd_free that really runs in irq, that would need the write_lock so it wouldn't be a solution).

I'm trying this fix and the VM_BUG_ON never triggered yet.

In short: who takes the pgd_lock from an irq? (__mmdrop shouldn't but maybe I'm overlooking some aio bit?)

Quoted 10 lines · Andrea Arcangeli

Hello,

Larry (CC'ed) found a problem with the patch in subject. When USE_SPLIT_PTLOCKS is not defined (NR_CPUS == 2) it will deadlock in ptep_clear_flush_notify in rmap.c because it's sending IPIs with the page_table_lock already held, and the other CPUs now spins on the page_table_lock with irq disabled, so the IPI never runs. With CONFIG_TRANSPARENT_HUGEPAGE=y this deadlocks happens even with USE_SPLIT_PTLOCKS defined so it become visible but it needs to be fixed regardless (for NR_CPUS == 2).

What's "it" here? Do you mean vmalloc_sync_all? vmalloc_sync_one? What's the callchain?

Quoted 2 lines

spin_lock_irqsave(pgd_lock) so I guess it's either common code, or it's superfluous and not another Xen special requirement.

There's no special Xen requirement here.

mmdrop() can be called from interrupt context, but I don't know if it will ever drop the last reference from interrupt, so maybe you can get away with it.

arch/x86/mm/fault.c void vmalloc_sync_all(void) · line 251

Quoted 6 lines

arch/x86/mm/fault.c +1 -1

@@ -247,7 +248,7 @@ void vmalloc_sync_all(void)
 			if (!ret)
 				break;
 		}
-		spin_unlock_irqrestore(&pgd_lock, flags);
+		spin_unlock(&pgd_lock, flags);

Urp. Did this compile?

Quoted 11 lines · Jeremy Fitzhardinge

Quoted 10 lines · Andrea Arcangeli

Hello,

Larry (CC'ed) found a problem with the patch in subject. When USE_SPLIT_PTLOCKS is not defined (NR_CPUS == 2) it will deadlock in ptep_clear_flush_notify in rmap.c because it's sending IPIs with the page_table_lock already held, and the other CPUs now spins on the page_table_lock with irq disabled, so the IPI never runs. With CONFIG_TRANSPARENT_HUGEPAGE=y this deadlocks happens even with USE_SPLIT_PTLOCKS defined so it become visible but it needs to be fixed regardless (for NR_CPUS == 2).

Quoted 2 lines

What's "it" here? Do you mean vmalloc_sync_all? vmalloc_sync_one? What's the callchain?

Larry just answered to that. If something is unclear let me know. I never reproduced it, but it also can happen without THP enabled, you just need to set NR_CPUS to 2 during "make menuconfig".

Quoted 3 lines

Quoted 2 lines

spin_lock_irqsave(pgd_lock) so I guess it's either common code, or it's superfluous and not another Xen special requirement.

There's no special Xen requirement here.

That was my thought too considering the other archs...

Quoted 3 lines

mmdrop() can be called from interrupt context, but I don't know if it will ever drop the last reference from interrupt, so maybe you can get away with it.

Yes the issue is __mmdrop, so it'd be nice to figure if __mmdrop can also run from irq (or only mmdrop fast path which would be safe even without _irqsave).

Is this a Xen only thing? Or is mmdrop called from regular linux. Considering other archs also _irqsave I assume it's common code calling mmdrop (otherwise it means they cut-and-pasted a Xen dependency). This comment doesn't really tell me much.

static void pgd_dtor(pgd_t *pgd)

{
	unsigned long flags; /* can be called from interrupt context 	*/

	if (SHARED_KERNEL_PMD)
	   return;

	   VM_BUG_ON(in_interrupt());
	   spin_lock(&pgd_lock);

This comment tells the very __mmdrop can be called from irq context, not just mmdrop. But I didn't find where yet... Can you tell me?

Quoted 7 lines

arch/x86/mm/fault.c void vmalloc_sync_all(void) · line 251

Quoted 6 lines

arch/x86/mm/fault.c +1 -1

@@ -247,7 +248,7 @@ void vmalloc_sync_all(void)
 			if (!ret)
 				break;
 		}
-		spin_unlock_irqrestore(&pgd_lock, flags);
+		spin_unlock(&pgd_lock, flags);

Urp. Did this compile?

Yes it builds

I'm not reposting a version that builds for 32bit x86 too until we figure out the mmdrop thing...

No, I don't think there's any xen-specific code which calls mmdrop (at all, let alone in interrupt context). Erm, but I'm not sure where it does. I had a thinko that "schedule" would be one of those places, but calling that from interrupt context would cause much bigger problems :/

Quoted 1 line

static void pgd_dtor(pgd_t *pgd)

No. I don't think I wrote that comment. It possibly just some ancient lore that could have been correct at one point, or perhaps it never true.

Quoted 9 lines

Quoted 8 lines

arch/x86/mm/fault.c void vmalloc_sync_all(void) · line 251

Quoted 7 lines

arch/x86/mm/fault.c +1 -1

@@ -247,7 +248,7 @@ void vmalloc_sync_all(void)
 			if (!ret)
 				break;
 		}
-		spin_unlock_irqrestore(&pgd_lock, flags);
+		spin_unlock(&pgd_lock, flags);

Urp. Did this compile?

Yes it builds

(spin_unlock() shouldn't take a "flags" arg.)

Quoted 2 lines

I'm not reposting a version that builds for 32bit x86 too until we figure out the mmdrop thing...

Stick it in next and look for explosion reports?

Hello,

Without this patch we can deadlock in the page_table_lock with NR_CPUS < 4 or THP on, with this patch we hopefully won't deadlock in the pgd_lock (if taken from irq). I can't see anything taking it from irq (maybe aio? to check I also tried the libaio testuite with no apparent VM_BUG_ON triggering), so unless somebody sees it, I think we should apply it. I've been running for a while with this patch applied without apparent problems. Other archs may follow suit if it's proven that there's nothing taking the pgd_lock from irq.

===
Subject: fix pgd_lock deadlock

From: Andrea Arcangeli <[email protected]>

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

Quoted 11 lines · Andrea Arcangeli

Hello,

Without this patch we can deadlock in the page_table_lock with NR_CPUS < 4 or THP on, with this patch we hopefully won't deadlock in the pgd_lock (if taken from irq). I can't see anything taking it from irq (maybe aio? to check I also tried the libaio testuite with no apparent VM_BUG_ON triggering), so unless somebody sees it, I think we should apply it. I've been running for a while with this patch applied without apparent problems. Other archs may follow suit if it's proven that there's nothing taking the pgd_lock from irq.

Quoted 8 lines

===
Subject: fix pgd_lock deadlock

From: Andrea Arcangeli<[email protected]>

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

I really read this thing 5 times and still cannot make any sense of it.

You talk about page_table_lock and then fiddle with pgd_lock.

-ENOSENSE

tglx

Quoted 11 lines · Andrea Arcangeli

Hello,

Without this patch we can deadlock in the page_table_lock with NR_CPUS < 4 or THP on, with this patch we hopefully won't deadlock in the pgd_lock (if taken from irq). I can't see anything taking it from irq (maybe aio? to check I also tried the libaio testuite with no apparent VM_BUG_ON triggering), so unless somebody sees it, I think we should apply it. I've been running for a while with this patch applied without apparent problems. Other archs may follow suit if it's proven that there's nothing taking the pgd_lock from irq.

Quoted 8 lines

===
Subject: fix pgd_lock deadlock

From: Andrea Arcangeli <[email protected]>

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

I really read this thing 5 times and still cannot make any sense of it.

You talk about page_table_lock and then fiddle with pgd_lock.

-ENOSENSE

On Tue, Feb 15, 2011 at 08:26:51PM +0100, Thomas Gleixner wrote:

With NR_CPUs < 4, or with THP enabled, rmap.c will do spin_lock(&mm->page_table_lock) (or pte_offset_map_lock where the lock is still mm->page_table_lock and not the PT lock). Then it will send IPIs to flush the tlb of the other CPUs.

But the other CPU is running the vmalloc_sync_all, and it is trying to take the page_table_lock with irq disabled. It will never take the lock because the CPU waiting the IPI delivery holds it. And it will never run the IPI because it has irqs disabled.

Now the big question is if anything is taking the pgd_lock from irqs. Normal testing could never reveal it as even if it happens it has a slim chance to happen while the pgd_lock is already hold by normal kernel context. But the VM_BUG_ON(in_interrupt()) should hopefully have revealed it already if it ever happened, I hope.

Clearly we could try to fix it in other ways, but still if there's no reason to do the _irqsave this sounds a good idea to apply my fix anyway.

Quoted 11 lines · Andrea Arcangeli

On Tue, Feb 15, 2011 at 08:26:51PM +0100, Thomas Gleixner wrote:

With NR_CPUs < 4, or with THP enabled, rmap.c will do spin_lock(&mm->page_table_lock) (or pte_offset_map_lock where the lock is still mm->page_table_lock and not the PT lock). Then it will send IPIs to flush the tlb of the other CPUs.

But the other CPU is running the vmalloc_sync_all, and it is trying to take the page_table_lock with irq disabled. It will never take the lock because the CPU waiting the IPI delivery holds it. And it will never run the IPI because it has irqs disabled.

Ok, that makes sense :)

Quoted 6 lines

Now the big question is if anything is taking the pgd_lock from irqs. Normal testing could never reveal it as even if it happens it has a slim chance to happen while the pgd_lock is already hold by normal kernel context. But the VM_BUG_ON(in_interrupt()) should hopefully have revealed it already if it ever happened, I hope.

Quoted 3 lines

Clearly we could try to fix it in other ways, but still if there's no reason to do the _irqsave this sounds a good idea to apply my fix anyway.

Did you try with DEBUG_PAGEALLOC, which is calling into cpa quite a lot?

Another thing. You check for in_interrupt(), but what makes sure that the code which takes pgd_lock is never taken with interrupts disabled except during early boot ?

Quoted 3 lines · Thomas Gleixner

Another thing. You check for in_interrupt(), but what makes sure that the code which takes pgd_lock is never taken with interrupts disabled except during early boot ?

It's perfectly fine to take pgd_lock with irq disabled, as long as you don't pretend to take the page_table_lock too after that. So that's not a concern.

I removed _irqsave from all pgd_lock, and I doubt there's any code protected by pgd_lock that runs with irq disabled, but if there is, it's still ok and it especially shouldn't have used _irqsave.

The only real issue here to sort out, is if pgd_lock is ever taken from irq or not, and to me it looks like in_interrupt() should trigger if it is ever taken from irq, so it won't go unnoticed for long if this isn't ok.

I assume you run it with a lockdep enabled kernel as well, right ?

Quoted 1 line · Thomas Gleixner

I assume you run it with a lockdep enabled kernel as well, right ?

Yes, I always run with lockdep and prove locking enabled on my test box, not sure how it's meant to trigger more bugs in this case, the debug check that should be relevant for this is DEBUG_VM and that is enabled too of course. I didn't try DEBUG_PAGEALLOC yet.

Quoted 7 lines · Andrea Arcangeli

Quoted 1 line · Thomas Gleixner

I assume you run it with a lockdep enabled kernel as well, right ?

Yes, I always run with lockdep and prove locking enabled on my test box, not sure how it's meant to trigger more bugs in this case, the debug check that should be relevant for this is DEBUG_VM and that is enabled too of course. I didn't try DEBUG_PAGEALLOC yet.

I think what Thomas tried to tell you is that your VM_BUG_ON(in_interrupt()) is fully redundant if you have lockdep enabled.

Lockdep will warn you if a !irqsave lock is taken from IRQ context, since that is a clear inversion problem.

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Andrea Arcangeli

Quoted 1 line · Thomas Gleixner

I assume you run it with a lockdep enabled kernel as well, right ?

Yes, I always run with lockdep and prove locking enabled on my test box, not sure how it's meant to trigger more bugs in this case, the debug check that should be relevant for this is DEBUG_VM and that is enabled too of course. I didn't try DEBUG_PAGEALLOC yet.

Quoted 6 lines

I think what Thomas tried to tell you is that your VM_BUG_ON(in_interrupt()) is fully redundant if you have lockdep enabled.

Lockdep will warn you if a !irqsave lock is taken from IRQ context, since that is a clear inversion problem.

Ah I get it now, but I prefer to have it on an all my builds, and I don't keep lockdep on for all builds (but I keep DEBUG_VM on). It's still only debug code that no production system will ever deal with, so it should be good to exercise it in more than on debug .config considering it's very low overhead (pgd_lock is never taken in fast paths) so it's suitable for a VM_BUG_ON.

The point both Thomas and Peter tried to point out to you, that adding 7 instances of redundant debug checks:

(quoted diff) +7 -0

+		VM_BUG_ON(in_interrupt());
+		VM_BUG_ON(in_interrupt());
+	VM_BUG_ON(in_interrupt());
+	VM_BUG_ON(in_interrupt());
+	VM_BUG_ON(in_interrupt());
+	VM_BUG_ON(in_interrupt());
+	VM_BUG_ON(in_interrupt());

to arch/x86/ is not acceptable, for the reasons stated. Please remove it from the patch.

Subject: fix pgd_lock deadlock

From: Andrea Arcangeli<[email protected]>

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

Apparently nobody takes the pgd_lock from irq so the _irqsave can be removed.

Signed-off-by Andrea Arcangeli<[email protected]>

Did you try with DEBUG_PAGEALLOC, which is calling into cpa quite a lot?

Ok sorry, but whenever I'm asked if I tested my patch with lockdep, I think if it was tested for other bugs, not to remove debug code overlapping with some lockdep feature.

Quoted 2 lines · Thomas Gleixner

Did you try with DEBUG_PAGEALLOC, which is calling into cpa quite a lot?

I tried DEBUG_PAGEALLOC and it seems to work fine (in addition to lockdep), it doesn't spwan any debug check.

In addition to testing it (both prev patch and below one) I looked

into the code and the free_pages calling into pageattr->split_large_page apparently happens all at boot time.

Now one doubt remains if we need change_page_attr to run from irqs (not from DEBUG_PAGEALLOC though). Now is change_page_attr really sane to run from irqs? I thought __flush_tlb_all was delivering IPI (in that case it also wouldn't have been safe in the first place to run with irq disabled) but of course the "__" version is local, so after all maybe it's safe to run with interrupts too (I'd be amazed if somebody is calling it from irq, if not even DEBUG_PAGEALLOC does) but with the below patch it will remain safe from irq as far as the pgd_lock is concerned.

I think the previous patch was safe too though, avoiding VM manipulations from interrupts makes everything simpler. Normally only gart drivers should call it at init time to avoid prefetching of cachelines in the next 2m page with different (writeback) cache attributes of the pages physically aliased in the gart and mapped with different cache attribute, that init stuff happening from interrupt sounds weird. Anyway I post the below patch too as an alternative to still allow pageattr from irq.

With both patches the big dependency remains on __mmdrop not to run from irq. The alternative approach is to remove the page_table_lock from vmalloc_sync_all (which is only needed by Xen paravirt guest AFIK) and solve that problem in a different way, but I don't even know why they need it exactly, I tried not to impact that.

===
Subject: fix pgd_lock deadlock

From: Andrea Arcangeli <[email protected]>

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

pageattr also seems to take advantage the pgd_lock to serialize split_large_page which isn't necessary so split it off to a separate spinlock (as a read_lock wouldn't enough to serialize split_large_page). Then we can use a read-write lock to allow pageattr to keep taking it from interrupts, but without having to always clear interrupts for readers. Writers are only safe from regular kernel context and they must clear irqs before taking it.

Signed-off-by Andrea Arcangeli <[email protected]>

---
 arch/x86/include/asm/pgtable.h |    2 +-
 arch/x86/mm/fault.c            |   23 ++++++++++++++++++-----
 arch/x86/mm/init_64.c          |    6 +++---
 arch/x86/mm/pageattr.c         |   17 ++++++++++-------
 arch/x86/mm/pgtable.c          |   10 +++++-----
 arch/x86/xen/mmu.c             |   10 ++++------
 6 files changed, 41 insertions(+), 27 deletions(-)

arch/x86/mm/fault.c +18 -5

@@ -179,7 +179,21 @@ force_sig_info_fault(int si_signo, int s
 	force_sig_info(si_signo, &info, tsk);
 }
 
-DEFINE_SPINLOCK(pgd_lock);
+/*
+ * The pgd_lock only protects the pgd_list. It can be taken from
+ * interrupt context but only in read mode (there's no need to clear
+ * interrupts before taking it in read mode). Write mode can only be
+ * taken from regular kernel context (not from irqs) and it must
+ * disable irqs before taking it. This way it can still be taken in
+ * read mode from interrupt context (in case
+ * pageattr->split_large_page is ever called from interrupt context),
+ * but without forcing the pgd_list readers to clear interrupts before
+ * taking it (like vmalloc_sync_all() that then wants to take the
+ * page_table_lock while holding the pgd_lock, which can only be taken
+ * while irqs are left enabled to avoid deadlocking against the IPI
+ * delivery of an SMP TLB flush run with the page_table_lock hold).
+ */
+DEFINE_RWLOCK(pgd_lock);
 LIST_HEAD(pgd_list);
 
 #ifdef CONFIG_X86_32

@@ -229,15 +243,14 @@ void vmalloc_sync_all(void)
 	for (address = VMALLOC_START & PMD_MASK;
 	     address >= TASK_SIZE && address < FIXADDR_TOP;
 	     address += PMD_SIZE) {
-
-		unsigned long flags;
 		struct page *page;
 
-		spin_lock_irqsave(&pgd_lock, flags);
+		read_lock(&pgd_lock);
 		list_for_each_entry(page, &pgd_list, lru) {
 			spinlock_t *pgt_lock;
 			pmd_t *ret;
 
+			/* the pgt_lock only for Xen */
 			pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
 
 			spin_lock(pgt_lock);

@@ -247,7 +260,7 @@ void vmalloc_sync_all(void)
 			if (!ret)
 				break;
 		}
-		spin_unlock_irqrestore(&pgd_lock, flags);
+		read_unlock(&pgd_lock);
 	}
 }
 

arch/x86/mm/init_64.c +3 -3

@@ -105,18 +105,18 @@ void sync_global_pgds(unsigned long star
 
 	for (address = start; address <= end; address += PGDIR_SIZE) {
 		const pgd_t *pgd_ref = pgd_offset_k(address);
-		unsigned long flags;
 		struct page *page;
 
 		if (pgd_none(*pgd_ref))
 			continue;
 
-		spin_lock_irqsave(&pgd_lock, flags);
+		read_lock(&pgd_lock);
 		list_for_each_entry(page, &pgd_list, lru) {
 			pgd_t *pgd;
 			spinlock_t *pgt_lock;
 
 			pgd = (pgd_t *)page_address(page) + pgd_index(address);
+			/* the pgt_lock only for Xen */
 			pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
 			spin_lock(pgt_lock);
 

@@ -128,7 +128,7 @@ void sync_global_pgds(unsigned long star
 
 			spin_unlock(pgt_lock);
 		}
-		spin_unlock_irqrestore(&pgd_lock, flags);
+		read_unlock(&pgd_lock);
 	}
 }
 

arch/x86/mm/pageattr.c +10 -7

@@ -47,6 +47,7 @@ struct cpa_data {
  * splitting a large page entry along with changing the attribute.
  */
 static DEFINE_SPINLOCK(cpa_lock);
+static DEFINE_SPINLOCK(pgattr_lock);
 
 #define CPA_FLUSHTLB 1
 #define CPA_ARRAY 2

@@ -60,9 +61,9 @@ void update_page_count(int level, unsign
 	unsigned long flags;
 
 	/* Protect against CPA */
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock_irqsave(&pgattr_lock, flags);
 	direct_pages_count[level] += pages;
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock_irqrestore(&pgattr_lock, flags);
 }
 
 static void split_page_count(int level)

@@ -376,6 +377,7 @@ static void __set_pmd_pte(pte_t *kpte, u
 	if (!SHARED_KERNEL_PMD) {
 		struct page *page;
 
+		read_lock(&pgd_lock);
 		list_for_each_entry(page, &pgd_list, lru) {
 			pgd_t *pgd;
 			pud_t *pud;

@@ -386,6 +388,7 @@ static void __set_pmd_pte(pte_t *kpte, u
 			pmd = pmd_offset(pud, address);
 			set_pte_atomic((pte_t *)pmd, pte);
 		}
+		read_unlock(&pgd_lock);
 	}
 #endif
 }

@@ -403,7 +406,7 @@ try_preserve_large_page(pte_t *kpte, uns
 	if (cpa->force_split)
 		return 1;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock_irqsave(&pgattr_lock, flags);
 	/*
 	 * Check for races, another CPU might have split this page
 	 * up already:

@@ -498,7 +501,7 @@ try_preserve_large_page(pte_t *kpte, uns
 	}
 
 out_unlock:
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock_irqrestore(&pgattr_lock, flags);
 
 	return do_split;
 }

@@ -519,7 +522,7 @@ static int split_large_page(pte_t *kpte,
 	if (!base)
 		return -ENOMEM;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	spin_lock_irqsave(&pgattr_lock, flags);
 	/*
 	 * Check for races, another CPU might have split this page
 	 * up for us already:

@@ -587,11 +590,11 @@ static int split_large_page(pte_t *kpte,
 out_unlock:
 	/*
 	 * If we dropped out via the lookup_address check under
-	 * pgd_lock then stick the page back into the pool:
+	 * pgattr_lock then stick the page back into the pool:
 	 */
 	if (base)
 		__free_page(base);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	spin_unlock_irqrestore(&pgattr_lock, flags);
 
 	return 0;
 }

arch/x86/mm/pgtable.c +5 -5

@@ -121,14 +121,14 @@ static void pgd_ctor(struct mm_struct *m
 
 static void pgd_dtor(pgd_t *pgd)
 {
-	unsigned long flags; /* can be called from interrupt context */
+	unsigned long flags;
 
 	if (SHARED_KERNEL_PMD)
 		return;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	write_lock_irqsave(&pgd_lock, flags);
 	pgd_list_del(pgd);
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	write_unlock_irqrestore(&pgd_lock, flags);
 }
 
 /*

@@ -280,12 +280,12 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
 	 * respect to anything walking the pgd_list, so that they
 	 * never see a partially populated pgd.
 	 */
-	spin_lock_irqsave(&pgd_lock, flags);
+	write_lock_irqsave(&pgd_lock, flags);
 
 	pgd_ctor(mm, pgd);
 	pgd_prepopulate_pmd(mm, pgd, pmds);
 
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	write_unlock_irqrestore(&pgd_lock, flags);
 
 	return pgd;
 

arch/x86/xen/mmu.c +4 -6

@@ -986,10 +986,9 @@ static void xen_pgd_pin(struct mm_struct
  */
 void xen_mm_pin_all(void)
 {
-	unsigned long flags;
 	struct page *page;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	read_lock(&pgd_lock);
 
 	list_for_each_entry(page, &pgd_list, lru) {
 		if (!PagePinned(page)) {

@@ -998,7 +997,7 @@ void xen_mm_pin_all(void)
 		}
 	}
 
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	read_unlock(&pgd_lock);
 }
 
 /*

@@ -1099,10 +1098,9 @@ static void xen_pgd_unpin(struct mm_stru
  */
 void xen_mm_unpin_all(void)
 {
-	unsigned long flags;
 	struct page *page;
 
-	spin_lock_irqsave(&pgd_lock, flags);
+	read_lock(&pgd_lock);
 
 	list_for_each_entry(page, &pgd_list, lru) {
 		if (PageSavePinned(page)) {

@@ -1112,7 +1110,7 @@ void xen_mm_unpin_all(void)
 		}
 	}
 
-	spin_unlock_irqrestore(&pgd_lock, flags);
+	read_unlock(&pgd_lock);
 }
 
 void xen_activate_mm(struct mm_struct *prev, struct mm_struct *next)

arch/x86/include/asm/pgtable.h +1 -2

@@ -25,7 +25,7 @@
 extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
 #define ZERO_PAGE(vaddr) (virt_to_page(empty_zero_page))
 
-extern spinlock_t pgd_lock;
+extern rwlock_t pgd_lock;
 extern struct list_head pgd_list;
 
 extern struct mm_struct *pgd_page_get_mm(struct page *page);
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 2 lines · Thomas Gleixner

Did you try with DEBUG_PAGEALLOC, which is calling into cpa quite a lot?

I tried DEBUG_PAGEALLOC and it seems to work fine (in addition to lockdep), it doesn't spwan any debug check.

In addition to testing it (both prev patch and below one) I looked into the code and the free_pages calling into pageattr->split_large_page apparently happens all at boot time.

Now one doubt remains if we need change_page_attr to run from irqs (not from DEBUG_PAGEALLOC though). Now is change_page_attr really sane to run from irqs? I thought __flush_tlb_all was delivering IPI (in that case it also wouldn't have been safe in the first place to run with irq disabled) but of course the "__" version is local, so after all maybe it's safe to run with interrupts too (I'd be amazed if somebody is calling it from irq, if not even DEBUG_PAGEALLOC does) but with the below patch it will remain safe from irq as far as the pgd_lock is concerned.

I think the previous patch was safe too though, avoiding VM manipulations from interrupts makes everything simpler. Normally only gart drivers should call it at init time to avoid prefetching of cachelines in the next 2m page with different (writeback) cache attributes of the pages physically aliased in the gart and mapped with different cache attribute, that init stuff happening from interrupt sounds weird. Anyway I post the below patch too as an alternative to still allow pageattr from irq.

With both patches the big dependency remains on __mmdrop not to run from irq. The alternative approach is to remove the page_table_lock from vmalloc_sync_all (which is only needed by Xen paravirt guest AFIK) and solve that problem in a different way, but I don't even know why they need it exactly, I tried not to impact that.

So Xen needs all page tables protected when pinning/unpinning and extended page_table_lock to cover kernel range, which it does nowhere else AFAICS. But the places it extended are also taking the pgd_lock, so I wonder if Xen could just take the pgd_lock itself in these paths and we could revert page_table_lock back to cover user va only? Jeremy, could this work? Untested.

Quoted 6 lines · Johannes Weiner

So Xen needs all page tables protected when pinning/unpinning and extended page_table_lock to cover kernel range, which it does nowhere else AFAICS. But the places it extended are also taking the pgd_lock, so I wonder if Xen could just take the pgd_lock itself in these paths and we could revert page_table_lock back to cover user va only? Jeremy, could this work? Untested.

If this works for Xen, I definitely prefer this.

Still there's no point to insist on _irqsave if nothing takes the pgd_lock from irq, so my patch probably should be applied anyway or it's confusing and there's even a comment saying pgd_dtor is called from irq, if it's not it should be corrected. But then it becomes a cleanup notafix.

So Xen needs all page tables protected when pinning/unpinning and extended page_table_lock to cover kernel range, which it does nowhere else AFAICS. But the places it extended are also taking the pgd_lock, so I wonder if Xen could just take the pgd_lock itself in these paths and we could revert page_table_lock back to cover user va only? Jeremy, could this work? Untested.

Hannes

---
 arch/x86/include/asm/pgtable.h |    2 --
 arch/x86/mm/fault.c            |   14 ++------------
 arch/x86/mm/init_64.c          |    6 ------
 arch/x86/mm/pgtable.c          |   20 +++-----------------
 arch/x86/xen/mmu.c             |    8 ++++++++
 5 files changed, 13 insertions(+), 37 deletions(-)

arch/x86/include/asm/pgtable.h +0 -2

index 18601c8..8c0335a 100644
@@ -28,8 +28,6 @@ extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
 extern spinlock_t pgd_lock;
 extern struct list_head pgd_list;
 
-extern struct mm_struct *pgd_page_get_mm(struct page *page);
-
 #ifdef CONFIG_PARAVIRT
 #include <asm/paravirt.h>
 #else  /* !CONFIG_PARAVIRT */

arch/x86/mm/fault.c +2 -12

index 7d90ceb..5da4155 100644
@@ -234,19 +234,9 @@ void vmalloc_sync_all(void)
 		struct page *page;
 
 		spin_lock_irqsave(&pgd_lock, flags);
-		list_for_each_entry(page, &pgd_list, lru) {
-			spinlock_t *pgt_lock;
-			pmd_t *ret;
-
-			pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
-
-			spin_lock(pgt_lock);
-			ret = vmalloc_sync_one(page_address(page), address);
-			spin_unlock(pgt_lock);
-
-			if (!ret)
+		list_for_each_entry(page, &pgd_list, lru)
+			if (!vmalloc_sync_one(page_address(page), address))
 				break;
-		}
 		spin_unlock_irqrestore(&pgd_lock, flags);
 	}
 }

arch/x86/mm/init_64.c +0 -6

index 71a5929..9332f21 100644
@@ -114,19 +114,13 @@ void sync_global_pgds(unsigned long start, unsigned long end)
 		spin_lock_irqsave(&pgd_lock, flags);
 		list_for_each_entry(page, &pgd_list, lru) {
 			pgd_t *pgd;
-			spinlock_t *pgt_lock;
 
 			pgd = (pgd_t *)page_address(page) + pgd_index(address);
-			pgt_lock = &pgd_page_get_mm(page)->page_table_lock;
-			spin_lock(pgt_lock);
-
 			if (pgd_none(*pgd))
 				set_pgd(pgd, *pgd_ref);
 			else
 				BUG_ON(pgd_page_vaddr(*pgd)
 				       != pgd_page_vaddr(*pgd_ref));
-
-			spin_unlock(pgt_lock);
 		}
 		spin_unlock_irqrestore(&pgd_lock, flags);
 	}

arch/x86/mm/pgtable.c +3 -17

index 500242d..72107ab 100644
@@ -87,19 +87,7 @@ static inline void pgd_list_del(pgd_t *pgd)
 #define UNSHARED_PTRS_PER_PGD				\
 	(SHARED_KERNEL_PMD ? KERNEL_PGD_BOUNDARY : PTRS_PER_PGD)
 
-
-static void pgd_set_mm(pgd_t *pgd, struct mm_struct *mm)
-{
-	BUILD_BUG_ON(sizeof(virt_to_page(pgd)->index) < sizeof(mm));
-	virt_to_page(pgd)->index = (pgoff_t)mm;
-}
-
-struct mm_struct *pgd_page_get_mm(struct page *page)
-{
-	return (struct mm_struct *)page->index;
-}
-
-static void pgd_ctor(struct mm_struct *mm, pgd_t *pgd)
+static void pgd_ctor(pgd_t *pgd)
 {
 	/* If the pgd points to a shared pagetable level (either the
 	   ptes in non-PAE, or shared PMD in PAE), then just copy the

@@ -113,10 +101,8 @@ static void pgd_ctor(struct mm_struct *mm, pgd_t *pgd)
 	}
 
 	/* list required to sync kernel mapping updates */
-	if (!SHARED_KERNEL_PMD) {
-		pgd_set_mm(pgd, mm);
+	if (!SHARED_KERNEL_PMD)
 		pgd_list_add(pgd);
-	}
 }
 
 static void pgd_dtor(pgd_t *pgd)

@@ -282,7 +268,7 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
 	 */
 	spin_lock_irqsave(&pgd_lock, flags);
 
-	pgd_ctor(mm, pgd);
+	pgd_ctor(pgd);
 	pgd_prepopulate_pmd(mm, pgd, pmds);
 
 	spin_unlock_irqrestore(&pgd_lock, flags);

arch/x86/xen/mmu.c +8 -0

index 5e22810..97fbfce 100644
@@ -1021,7 +1021,11 @@ static void __xen_pgd_pin(struct mm_struct *mm, pgd_t *pgd)
 
 static void xen_pgd_pin(struct mm_struct *mm)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&pgd_lock, flags);
 	__xen_pgd_pin(mm, mm->pgd);
+	spin_unlock_irqrestore(&pgd_lock, flags);
 }
 
 /*

@@ -1140,7 +1144,11 @@ static void __xen_pgd_unpin(struct mm_struct *mm, pgd_t *pgd)
 
 static void xen_pgd_unpin(struct mm_struct *mm)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&pgd_lock, flags);
 	__xen_pgd_unpin(mm, mm->pgd);
+	spin_unlock_irqrestore(&pgd_lock, flags);
 }
 
 /*

Quoted 1 line

Quoted 1 line · Johannes Weiner

Quoted 8 lines · Andrea Arcangeli

Quoted 6 lines · Johannes Weiner

So Xen needs all page tables protected when pinning/unpinning and extended page_table_lock to cover kernel range, which it does nowhere else AFAICS. But the places it extended are also taking the pgd_lock, so I wonder if Xen could just take the pgd_lock itself in these paths and we could revert page_table_lock back to cover user va only? Jeremy, could this work? Untested.

Quoted 1 line

If this works for Xen, I definitely prefer this.

Below is real submission, with changelog and sign-off and all (except testing on Xen itself, sorry). I moved pgd_lock acquisition in this version to make the lock ordering perfectly clear. Xen people, could you have a look at this?

A possible alternative would be to acquire the page table lock in vmalloc_sync_all() only in the Xen case (perhaps by storing NULL into page->index in pgd_set_mm() when not running on Xen). This is utilizing the fact that there aren't (supposed to be - for non-pvops this is definitely the case) any TLB flush IPIs under Xen, and hence the race you're trying to fix doesn't exist there (while non-Xen doesn't need the extra locking).

Quoted 1 line

Quoted 1 line · Andrea Arcangeli

Quoted 7 lines · Jan Beulich

A possible alternative would be to acquire the page table lock in vmalloc_sync_all() only in the Xen case (perhaps by storing NULL into page->index in pgd_set_mm() when not running on Xen). This is utilizing the fact that there aren't (supposed to be - for non-pvops this is definitely the case) any TLB flush IPIs under Xen, and hence the race you're trying to fix doesn't exist there (while non-Xen doesn't need the extra locking).

That's sure ok with me. Can we use a global runtime to check if the guest is running under Xen paravirt, instead of passing that info through page->something?

If everyone's okay with putting a couple of "if (xen_pv_domain())" into mm/fault.c - sure. I would have thought that this wouldn't be liked, hence the suggestion to make this depend on seeing the backlink be non-NULL.

Quoted 4 lines · Jan Beulich

If everyone's okay with putting a couple of "if (xen_pv_domain())" into mm/fault.c - sure. I would have thought that this wouldn't be liked, hence the suggestion to make this depend on seeing the backlink be non-NULL.

I prefer the if (xen_pv_domain) so it also gets optimized away at build time when CONFIG_XEN=n. I think it's also more self explanatory to read.

Quoted 5 lines · Andrea Arcangeli

Quoted 4 lines · Jan Beulich

If everyone's okay with putting a couple of "if (xen_pv_domain())" into mm/fault.c - sure. I would have thought that this wouldn't be liked, hence the suggestion to make this depend on seeing the backlink be non-NULL.

Quoted 3 lines

I prefer the if (xen_pv_domain) so it also gets optimized away at build time when CONFIG_XEN=n. I think it's also more self explanatory to read.

Eh, very not keen about that. I'd only consider it as a last resort.

In practice CONFIG_XEN is always enabled in distros, so the conditional would always be done at runtime.

Quoted 10 lines

Quoted 1 line

Quoted 1 line · Andrea Arcangeli

Quoted 7 lines · Jan Beulich

A possible alternative would be to acquire the page table lock in vmalloc_sync_all() only in the Xen case (perhaps by storing NULL into page->index in pgd_set_mm() when not running on Xen). This is utilizing the fact that there aren't (supposed to be - for non-pvops this is definitely the case) any TLB flush IPIs under Xen, and hence the race you're trying to fix doesn't exist there (while non-Xen doesn't need the extra locking).

Quoted 3 lines

That's sure ok with me. Can we use a global runtime to check if the guest is running under Xen paravirt, instead of passing that info through page->something?

If everyone's okay with putting a couple of "if (xen_pv_domain())" into mm/fault.c - sure. I would have thought that this wouldn't be liked, hence the suggestion to make this depend on seeing the backlink be non-NULL.

Quoted 1 line

Quoted 1 line · Andrea Arcangeli

Quoted 10 lines · Jan Beulich

Quoted 10 lines

Quoted 1 line

Quoted 1 line · Andrea Arcangeli

Quoted 7 lines · Jan Beulich

A possible alternative would be to acquire the page table lock in vmalloc_sync_all() only in the Xen case (perhaps by storing NULL into page->index in pgd_set_mm() when not running on Xen). This is utilizing the fact that there aren't (supposed to be - for non-pvops this is definitely the case) any TLB flush IPIs under Xen, and hence the race you're trying to fix doesn't exist there (while non-Xen doesn't need the extra locking).

Quoted 8 lines

Quoted 3 lines

That's sure ok with me. Can we use a global runtime to check if the guest is running under Xen paravirt, instead of passing that info through page->something?

If everyone's okay with putting a couple of "if (xen_pv_domain())" into mm/fault.c - sure. I would have thought that this wouldn't be liked, hence the suggestion to make this depend on seeing the backlink be non-NULL.

What about this? The page->private logic gets optimized away at compile time with XEN=n.

The removal of _irqsave from pgd_lock, I'll delay it as it's no bugfix anymore.

===
Subject: xen: stop taking the page_table_lock with irq disabled

From: Andrea Arcangeli <[email protected]>

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

Only Xen needs the page_table_lock and Xen won't need IPI TLB flushes hence the deadlock doesn't exist for Xen.

Quoted 30 lines

Quoted 1 line

Quoted 1 line · Andrea Arcangeli

Quoted 10 lines · Jan Beulich

Quoted 10 lines

Quoted 1 line

Quoted 1 line · Andrea Arcangeli

Quoted 7 lines · Jan Beulich

A possible alternative would be to acquire the page table lock in vmalloc_sync_all() only in the Xen case (perhaps by storing NULL into page->index in pgd_set_mm() when not running on Xen). This is utilizing the fact that there aren't (supposed to be - for non-pvops this is definitely the case) any TLB flush IPIs under Xen, and hence the race you're trying to fix doesn't exist there (while non-Xen doesn't need the extra locking).

Quoted 8 lines

Quoted 3 lines

That's sure ok with me. Can we use a global runtime to check if the guest is running under Xen paravirt, instead of passing that info through page->something?

If everyone's okay with putting a couple of "if (xen_pv_domain())" into mm/fault.c - sure. I would have thought that this wouldn't be liked, hence the suggestion to make this depend on seeing the backlink be non-NULL.

What about this? The page->private logic gets optimized away at compile time with XEN=n.

The removal of _irqsave from pgd_lock, I'll delay it as it's no bugfix anymore.

===
Subject: xen: stop taking the page_table_lock with irq disabled

Quoted 8 lines

From: Andrea Arcangeli <[email protected]>

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

Only Xen needs the page_table_lock and Xen won't need IPI TLB flushes hence the deadlock doesn't exist for Xen.

Looks reasonable to me, except for the implementation no longer matching subject and description (the lock still gets taken with IRQs disabled, just that - as far as we can tell so far - doesn't matter for Xen).

With the conditional on the reader side I also wonder whether the conditional on the writer side is really a good thing to have, considering that generally distro kernels are likely to have XEN enabled.

Subject: fix pgd_lock deadlock

From: Andrea Arcangeli <[email protected]>

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

Apparently nobody takes the pgd_lock from irq so the _irqsave can be removed.

Signed-off-by Andrea Arcangeli <[email protected]>

Hello,

Quoted 4 lines · On Wednesday 16 February 2011 15:49:47 Andrea Arcangeli wrote:

Subject: fix pgd_lock deadlock

From: Andrea Arcangeli <[email protected]>

Quoted 8 lines

It's forbidden to take the page_table_lock with the irq disabled or if there's contention the IPIs (for tlb flushes) sent with the page_table_lock held will never run leading to a deadlock.

Apparently nobody takes the pgd_lock from irq so the _irqsave can be removed.

Signed-off-by Andrea Arcangeli <[email protected]>

This patch (original commit Id for 2.6.38 a79e53d85683c6dd9f99c90511028adc2043031f) needs to be back-ported to 2.6.32.x as well.
I observed a dead-lock problem when running a PAE enabled Debian 2.6.32.46+ kernel with 6 VCPUs as a KVM on (2.6.32, 3.2, 3.3) kernel, which showed the following behaviour:

1 VCPU is stuck in
pgd_alloc() ??? pgd_prepopulate_pmb() ???... ??? flush_tlb_others_ipi()
while (!cpumask_empty(to_cpumask(f->flush_cpumask)))

    cpu_relax();

(gdb) print f->flush_cpumask
$5 = {1}

while all other VCPUs are stuck in
pgd_alloc() ??? spin_lock_irqsave(pgd_lock)

I tracked it down to the commit
2.6.39-rc1: 4981d01eada5354d81c8929d5b2836829ba3df7b 2.6.32.34: ba456fd7ec1bdc31a4ad4a6bd02802dcaa730a33
x86: Flush TLB if PGD entry is changed in i386 PAE mode
which when reverted made the bug disappear.

Comparing 3.2 to 2.6.32.34 showed that the 'pgd-deadlock'-patch went into 2.6.38, that is before the 'PAE correctness'-patch, so the problem was probably never observed in the main development branch.
But for 2.6.32 the 'pgd-deadlock' patch is still missing, so the 'PAE corretness'-patch made the problem worse with 2.6.32.

The Patch was also back-ported to the OpenSUSE Kernel
<http://kernel.opensuse.org/cgit/kernel-source/commit/?id=ac27c01aa880c65d17043ab87249c613ac4c3635>,
Since the patch didn't apply cleanly on the current Debian kernel, I had to backport it for us and Debian. The patch is also available from our (German) Bugzilla <https://forge.univention.org/bugzilla/show_bug.cgi?id=26661> or from the Debian BTS at
<http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=669335>.