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

Groups > linux.kernel > #1740857 > unrolled thread

[RFC PATCH 0/2] Missing READ_ONCE in core and arch-specific pgtable code leading to crashes

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/2] Missing READ_ONCE in core and arch-specific pgtable code leading to crashes Will Deacon <will.deacon@arm.com> - 2017-09-27 18:00 +0200
    Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables Peter Zijlstra <peterz@infradead.org> - 2017-09-28 10:40 +0200
      Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables Will Deacon <will.deacon@arm.com> - 2017-09-28 10:50 +0200
        Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-28 17:50 +0200
          Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables Will Deacon <will.deacon@arm.com> - 2017-09-28 17:50 +0200
            Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-28 18:10 +0200
          Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables Michael Cree <mcree@orcon.net.nz> - 2017-09-28 21:40 +0200
            Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-29 03:00 +0200
              Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables Will Deacon <will.deacon@arm.com> - 2017-09-29 11:10 +0200
                Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables Will Deacon <will.deacon@arm.com> - 2017-09-29 18:40 +0200
                Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-29 18:40 +0200
    Re: [RFC PATCH 0/2] Missing READ_ONCE in core and arch-specific  pgtable code leading to crashes Richard Ruigrok <rruigrok@codeaurora.org> - 2017-09-28 19:40 +0200
    Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when  accessing page tables Timur Tabi <timur@codeaurora.org> - 2017-09-28 21:20 +0200
    Re: [RFC PATCH 0/2] Missing READ_ONCE in core and arch-specific  pgtable code leading to crashes Jon Masters <jcm@redhat.com> - 2017-09-28 21:40 +0200
      Re: [RFC PATCH 0/2] Missing READ_ONCE in core and arch-specific  pgtable code leading to crashes Will Deacon <will.deacon@arm.com> - 2017-09-29 11:00 +0200

#1740857 — [RFC PATCH 0/2] Missing READ_ONCE in core and arch-specific pgtable code leading to crashes

Hi,

We recently had a crash report[1] on arm64 that involved a bad dereference
in the page_vma_mapped code during ext4 writeback with THP active. I can
reproduce this on -rc2:

[  254.032812] PC is at check_pte+0x20/0x170
[  254.032948] LR is at page_vma_mapped_walk+0x2e0/0x540
[...]
[  254.036114] Process doio (pid: 2463, stack limit = 0xffff00000f2e8000)
[  254.036361] Call trace:
[  254.038977] [<ffff000008233328>] check_pte+0x20/0x170
[  254.039137] [<ffff000008233758>] page_vma_mapped_walk+0x2e0/0x540
[  254.039332] [<ffff000008234adc>] page_mkclean_one+0xac/0x278
[  254.039489] [<ffff000008234d98>] rmap_walk_file+0xf0/0x238
[  254.039642] [<ffff000008236e74>] rmap_walk+0x64/0xa0
[  254.039784] [<ffff0000082370c8>] page_mkclean+0x90/0xa8
[  254.040029] [<ffff0000081f3c64>] clear_page_dirty_for_io+0x84/0x2a8
[  254.040311] [<ffff00000832f984>] mpage_submit_page+0x34/0x98
[  254.040518] [<ffff00000832fb4c>] mpage_process_page_bufs+0x164/0x170
[  254.040743] [<ffff00000832fc8c>] mpage_prepare_extent_to_map+0x134/0x2b8
[  254.040969] [<ffff00000833530c>] ext4_writepages+0x484/0xe30
[  254.041175] [<ffff0000081f6ab4>] do_writepages+0x44/0xe8
[  254.041372] [<ffff0000081e5bd4>] __filemap_fdatawrite_range+0xbc/0x110
[  254.041568] [<ffff0000081e5e68>] file_write_and_wait_range+0x48/0xd8
[  254.041739] [<ffff000008324310>] ext4_sync_file+0x80/0x4b8
[  254.041907] [<ffff0000082bd434>] vfs_fsync_range+0x64/0xc0
[  254.042106] [<ffff0000082332b4>] SyS_msync+0x194/0x1e8

After digging into the issue, I found that we appear to be racing with
a concurrent pmd update in page_vma_mapped_walk, assumedly due a THP
splitting operation. Looking at the code there:

	pvmw->pmd = pmd_offset(pud, pvmw->address);
	if (pmd_trans_huge(*pvmw->pmd) || is_pmd_migration_entry(*pvmw->pmd)) {
		[...]
	} else {
		if (!check_pmd(pvmw))
			return false;
	}
	if (!map_pte(pvmw))
		goto next_pte;

what happens in the crashing scenario is that we see all zeroes for the
PMD in pmd_trans_huge(*pvmw->pmd), and so go to the 'else' case (migration
isn't enabled, so the test is removed at compile-time). check_pmd then does:

	pmde = READ_ONCE(*pvmw->pmd);
	return pmd_present(pmde) && !pmd_trans_huge(pmde);

and reads a valid table entry for the PMD because the splitting has completed
(i.e. the first dereference reads from the pmdp_invalidate in the splitting
code, whereas the second dereferenced reads from the following pmd_populate).
It returns true because we should descend to the PTE level in map_pte. map_pte
does:

	pvmw->pte = pte_offset_map(pvmw->pmd, pvmw->address);

which on arm64 (and this appears to be the same on x86) ends up doing:

	(pmd_page_paddr((*(pvmw->pmd))) + pte_index(pvmw->address) * sizeof(pte_t))

as part of its calculation. However, this is horribly broken because GCC
inlines everything and reuses the register it loaded for the initial
pmd_trans_huge check (when we loaded the value of zero) here, so we end up
calculating a junk pointer and crashing when we dereference it. Disassembly
at the end of the mail[2] for those who are curious.

The moral of the story is that read-after-read (same address) ordering *only*
applies if READ_ONCE is used consistently. This means we need to fix page
table dereferences in the core code as well as the arch code to avoid this
problem. The two RFC patches in this series fix arm64 (which is a bigger fix
that necessary since I clean things up too) and page_vma_mapped_walk.

Comments welcome.

Will

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-September/532786.html
[2]

// page_vma_mapped_walk
// pvmw->pmd = pmd_offset(pud, pvmw->address);
ldr     x0, [x19, #24]		// pvmw->pmd

// if (pmd_trans_huge(*pvmw->pmd) || is_pmd_migration_entry(*pvmw->pmd)) {
ldr     x1, [x0]		// *pvmw->pmd
cbz     x1, ffff0000082336a0 <page_vma_mapped_walk+0x228>
tbz     w1, #1, ffff000008233788 <page_vma_mapped_walk+0x310>	// pmd_trans_huge?

// else if (!check_pmd(pvmw))
ldr     x0, [x0]		// READ_ONCE in check_pmd
tst     x0, x24			// pmd_present?
b.eq    ffff000008233538 <page_vma_mapped_walk+0xc0>  // b.none
tbz     w0, #1, ffff000008233538 <page_vma_mapped_walk+0xc0>	// pmd_trans_huge?

// if (!map_pte(pvmw))
ldr     x0, [x19, #16]		// pvmw->address

// pvmw->pte = pte_offset_map(pvmw->pmd, pvmw->address);
and     x1, x1, #0xfffffffff000	// Reusing the old value of *pvmw->pmd!!!
[...]

--->8

Will Deacon (2):
  arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables
  mm: page_vma_mapped: Ensure pmd is loaded with READ_ONCE outside of
    lock

 arch/arm64/include/asm/hugetlb.h     |   2 +-
 arch/arm64/include/asm/kvm_mmu.h     |  18 +--
 arch/arm64/include/asm/mmu_context.h |   4 +-
 arch/arm64/include/asm/pgalloc.h     |  42 +++---
 arch/arm64/include/asm/pgtable.h     |  29 ++--
 arch/arm64/kernel/hibernate.c        | 148 +++++++++---------
 arch/arm64/mm/dump.c                 |  54 ++++---
 arch/arm64/mm/fault.c                |  44 +++---
 arch/arm64/mm/hugetlbpage.c          |  94 ++++++------
 arch/arm64/mm/kasan_init.c           |  62 ++++----
 arch/arm64/mm/mmu.c                  | 281 ++++++++++++++++++-----------------
 arch/arm64/mm/pageattr.c             |  30 ++--
 mm/page_vma_mapped.c                 |  25 ++--
 13 files changed, 427 insertions(+), 406 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1741284 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> In many cases, page tables can be accessed concurrently by either another
> CPU (due to things like fast gup) or by the hardware page table walker
> itself, which may set access/dirty bits. In such cases, it is important
> to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes
Alpha is the only one that cares about the difference between that and
READ_ONCE() and they do have the extra barrier, but if we're going to do
this, we might as well do it 'right' :-)

Also, a very long standing item on my TODO list is to see how much of it
we can unify across the various architectures, because there's a giant
amount of boiler plate involved with all this.

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

#1741302 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Thu, Sep 28, 2017 at 10:38:01AM +0200, Peter Zijlstra wrote:
> On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> > In many cases, page tables can be accessed concurrently by either another
> > CPU (due to things like fast gup) or by the hardware page table walker
> > itself, which may set access/dirty bits. In such cases, it is important
> > to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> > entries cannot be torn, merged or subject to apparent loss of coherence.
> 
> In fact, we should use lockless_dereference() for many of them. Yes
> Alpha is the only one that cares about the difference between that and
> READ_ONCE() and they do have the extra barrier, but if we're going to do
> this, we might as well do it 'right' :-)

I know this sounds daft, but I think one of the big reasons why
lockless_dereference() doesn't get an awful lot of use is because it's
such a mouthful! Why don't we just move the smp_read_barrier_depends()
into READ_ONCE? Would anybody actually care about the potential impact on
Alpha (which, frankly, is treading on thin ice given the low adoption of
lockless_dereference())?

> Also, a very long standing item on my TODO list is to see how much of it
> we can unify across the various architectures, because there's a giant
> amount of boiler plate involved with all this.

Yeah, I'd be happy to help with that as a separate series. I already tripped
over 5 or 6 page table walkers in arch/arm64/ alone :(

Will

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

#1741654 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Thu, Sep 28, 2017 at 09:45:35AM +0100, Will Deacon wrote:
> On Thu, Sep 28, 2017 at 10:38:01AM +0200, Peter Zijlstra wrote:
> > On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> > > In many cases, page tables can be accessed concurrently by either another
> > > CPU (due to things like fast gup) or by the hardware page table walker
> > > itself, which may set access/dirty bits. In such cases, it is important
> > > to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> > > entries cannot be torn, merged or subject to apparent loss of coherence.
> > 
> > In fact, we should use lockless_dereference() for many of them. Yes
> > Alpha is the only one that cares about the difference between that and
> > READ_ONCE() and they do have the extra barrier, but if we're going to do
> > this, we might as well do it 'right' :-)
> 
> I know this sounds daft, but I think one of the big reasons why
> lockless_dereference() doesn't get an awful lot of use is because it's
> such a mouthful! Why don't we just move the smp_read_barrier_depends()
> into READ_ONCE? Would anybody actually care about the potential impact on
> Alpha (which, frankly, is treading on thin ice given the low adoption of
> lockless_dereference())?

This is my cue to ask my usual question...  ;-)

Are people still running mainline kernels on Alpha?  (Added Alpha folks.)

As always, if anyone is, we must continue to support Alpha, but sounds
like time to check again.

							Thanx, Paul

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

#1741660 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Thu, Sep 28, 2017 at 08:43:54AM -0700, Paul E. McKenney wrote:
> On Thu, Sep 28, 2017 at 09:45:35AM +0100, Will Deacon wrote:
> > On Thu, Sep 28, 2017 at 10:38:01AM +0200, Peter Zijlstra wrote:
> > > On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> > > > In many cases, page tables can be accessed concurrently by either another
> > > > CPU (due to things like fast gup) or by the hardware page table walker
> > > > itself, which may set access/dirty bits. In such cases, it is important
> > > > to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> > > > entries cannot be torn, merged or subject to apparent loss of coherence.
> > > 
> > > In fact, we should use lockless_dereference() for many of them. Yes
> > > Alpha is the only one that cares about the difference between that and
> > > READ_ONCE() and they do have the extra barrier, but if we're going to do
> > > this, we might as well do it 'right' :-)
> > 
> > I know this sounds daft, but I think one of the big reasons why
> > lockless_dereference() doesn't get an awful lot of use is because it's
> > such a mouthful! Why don't we just move the smp_read_barrier_depends()
> > into READ_ONCE? Would anybody actually care about the potential impact on
> > Alpha (which, frankly, is treading on thin ice given the low adoption of
> > lockless_dereference())?
> 
> This is my cue to ask my usual question...  ;-)
> 
> Are people still running mainline kernels on Alpha?  (Added Alpha folks.)
> 
> As always, if anyone is, we must continue to support Alpha, but sounds
> like time to check again.

I'll be honest and say that I haven't updated mine for a while, but I do
have a soft spot for those machines :(

Will

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

#1741672 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Thu, Sep 28, 2017 at 04:49:54PM +0100, Will Deacon wrote:
> On Thu, Sep 28, 2017 at 08:43:54AM -0700, Paul E. McKenney wrote:
> > On Thu, Sep 28, 2017 at 09:45:35AM +0100, Will Deacon wrote:
> > > On Thu, Sep 28, 2017 at 10:38:01AM +0200, Peter Zijlstra wrote:
> > > > On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> > > > > In many cases, page tables can be accessed concurrently by either another
> > > > > CPU (due to things like fast gup) or by the hardware page table walker
> > > > > itself, which may set access/dirty bits. In such cases, it is important
> > > > > to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> > > > > entries cannot be torn, merged or subject to apparent loss of coherence.
> > > > 
> > > > In fact, we should use lockless_dereference() for many of them. Yes
> > > > Alpha is the only one that cares about the difference between that and
> > > > READ_ONCE() and they do have the extra barrier, but if we're going to do
> > > > this, we might as well do it 'right' :-)
> > > 
> > > I know this sounds daft, but I think one of the big reasons why
> > > lockless_dereference() doesn't get an awful lot of use is because it's
> > > such a mouthful! Why don't we just move the smp_read_barrier_depends()
> > > into READ_ONCE? Would anybody actually care about the potential impact on
> > > Alpha (which, frankly, is treading on thin ice given the low adoption of
> > > lockless_dereference())?
> > 
> > This is my cue to ask my usual question...  ;-)
> > 
> > Are people still running mainline kernels on Alpha?  (Added Alpha folks.)
> > 
> > As always, if anyone is, we must continue to support Alpha, but sounds
> > like time to check again.
> 
> I'll be honest and say that I haven't updated mine for a while, but I do
> have a soft spot for those machines :(

Let's see what the Alpha folks say.  I myself have had a close
relationship with Alpha for almost 20 years, but I suspect that in
my case it is more a hard spot on my head rather than a soft spot in
my heart.  ;-)

								Thanx,
								Paul

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

#1741794 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Thu, Sep 28, 2017 at 08:43:54AM -0700, Paul E. McKenney wrote:
> On Thu, Sep 28, 2017 at 09:45:35AM +0100, Will Deacon wrote:
> > On Thu, Sep 28, 2017 at 10:38:01AM +0200, Peter Zijlstra wrote:
> > > On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> > > > In many cases, page tables can be accessed concurrently by either another
> > > > CPU (due to things like fast gup) or by the hardware page table walker
> > > > itself, which may set access/dirty bits. In such cases, it is important
> > > > to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> > > > entries cannot be torn, merged or subject to apparent loss of coherence.
> > > 
> > > In fact, we should use lockless_dereference() for many of them. Yes
> > > Alpha is the only one that cares about the difference between that and
> > > READ_ONCE() and they do have the extra barrier, but if we're going to do
> > > this, we might as well do it 'right' :-)
> > 
> > I know this sounds daft, but I think one of the big reasons why
> > lockless_dereference() doesn't get an awful lot of use is because it's
> > such a mouthful! Why don't we just move the smp_read_barrier_depends()
> > into READ_ONCE? Would anybody actually care about the potential impact on
> > Alpha (which, frankly, is treading on thin ice given the low adoption of
> > lockless_dereference())?
> 
> This is my cue to ask my usual question...  ;-)
> 
> Are people still running mainline kernels on Alpha?  (Added Alpha folks.)

Yes.  I run two Alpha build daemons that build the unofficial
debian-alpha port.  Debian popcon reports nine machines running
Alpha, which are likely to be running the 4.12.y kernel which
is currently in debian-alpha, (and presumably soon to be 4.13.y
which is now built on Alpha in experimental).

Cheers
Michael

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

#1741880 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Fri, Sep 29, 2017 at 07:59:09AM +1300, Michael Cree wrote:
> On Thu, Sep 28, 2017 at 08:43:54AM -0700, Paul E. McKenney wrote:
> > On Thu, Sep 28, 2017 at 09:45:35AM +0100, Will Deacon wrote:
> > > On Thu, Sep 28, 2017 at 10:38:01AM +0200, Peter Zijlstra wrote:
> > > > On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> > > > > In many cases, page tables can be accessed concurrently by either another
> > > > > CPU (due to things like fast gup) or by the hardware page table walker
> > > > > itself, which may set access/dirty bits. In such cases, it is important
> > > > > to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> > > > > entries cannot be torn, merged or subject to apparent loss of coherence.
> > > > 
> > > > In fact, we should use lockless_dereference() for many of them. Yes
> > > > Alpha is the only one that cares about the difference between that and
> > > > READ_ONCE() and they do have the extra barrier, but if we're going to do
> > > > this, we might as well do it 'right' :-)
> > > 
> > > I know this sounds daft, but I think one of the big reasons why
> > > lockless_dereference() doesn't get an awful lot of use is because it's
> > > such a mouthful! Why don't we just move the smp_read_barrier_depends()
> > > into READ_ONCE? Would anybody actually care about the potential impact on
> > > Alpha (which, frankly, is treading on thin ice given the low adoption of
> > > lockless_dereference())?
> > 
> > This is my cue to ask my usual question...  ;-)
> > 
> > Are people still running mainline kernels on Alpha?  (Added Alpha folks.)
> 
> Yes.  I run two Alpha build daemons that build the unofficial
> debian-alpha port.  Debian popcon reports nine machines running
> Alpha, which are likely to be running the 4.12.y kernel which
> is currently in debian-alpha, (and presumably soon to be 4.13.y
> which is now built on Alpha in experimental).

I salute your dedication to Alpha!  ;-)

							Thanx, Paul

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

#1742001 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Thu, Sep 28, 2017 at 05:58:30PM -0700, Paul E. McKenney wrote:
> On Fri, Sep 29, 2017 at 07:59:09AM +1300, Michael Cree wrote:
> > On Thu, Sep 28, 2017 at 08:43:54AM -0700, Paul E. McKenney wrote:
> > > On Thu, Sep 28, 2017 at 09:45:35AM +0100, Will Deacon wrote:
> > > > On Thu, Sep 28, 2017 at 10:38:01AM +0200, Peter Zijlstra wrote:
> > > > > On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> > > > > > In many cases, page tables can be accessed concurrently by either another
> > > > > > CPU (due to things like fast gup) or by the hardware page table walker
> > > > > > itself, which may set access/dirty bits. In such cases, it is important
> > > > > > to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> > > > > > entries cannot be torn, merged or subject to apparent loss of coherence.
> > > > > 
> > > > > In fact, we should use lockless_dereference() for many of them. Yes
> > > > > Alpha is the only one that cares about the difference between that and
> > > > > READ_ONCE() and they do have the extra barrier, but if we're going to do
> > > > > this, we might as well do it 'right' :-)
> > > > 
> > > > I know this sounds daft, but I think one of the big reasons why
> > > > lockless_dereference() doesn't get an awful lot of use is because it's
> > > > such a mouthful! Why don't we just move the smp_read_barrier_depends()
> > > > into READ_ONCE? Would anybody actually care about the potential impact on
> > > > Alpha (which, frankly, is treading on thin ice given the low adoption of
> > > > lockless_dereference())?
> > > 
> > > This is my cue to ask my usual question...  ;-)
> > > 
> > > Are people still running mainline kernels on Alpha?  (Added Alpha folks.)
> > 
> > Yes.  I run two Alpha build daemons that build the unofficial
> > debian-alpha port.  Debian popcon reports nine machines running
> > Alpha, which are likely to be running the 4.12.y kernel which
> > is currently in debian-alpha, (and presumably soon to be 4.13.y
> > which is now built on Alpha in experimental).
> 
> I salute your dedication to Alpha!  ;-)

Ok, but where does that leave us wrt my initial proposal of moving
smp_read_barrier_depends() into READ_ONCE and getting rid of
lockless_dereference?

Michael (or anybody else running mainline on SMP Alpha) -- would you be
able to give the diff below a spin and see whether there's a measurable
performance impact?

Cheers,

Will

--->8

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index e95a2631e545..0ce21e25492a 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -340,6 +340,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 		__read_once_size(&(x), __u.__c, sizeof(x));		\
 	else								\
 		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
+	smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
 	__u.__val;							\
 })
 #define READ_ONCE(x) __READ_ONCE(x, 1)

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

#1742236 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Fri, Sep 29, 2017 at 09:29:39AM -0700, Paul E. McKenney wrote:
> On Fri, Sep 29, 2017 at 10:08:43AM +0100, Will Deacon wrote:
> > On Thu, Sep 28, 2017 at 05:58:30PM -0700, Paul E. McKenney wrote:
> > > On Fri, Sep 29, 2017 at 07:59:09AM +1300, Michael Cree wrote:
> > > > On Thu, Sep 28, 2017 at 08:43:54AM -0700, Paul E. McKenney wrote:
> > > > > On Thu, Sep 28, 2017 at 09:45:35AM +0100, Will Deacon wrote:
> > > > > > On Thu, Sep 28, 2017 at 10:38:01AM +0200, Peter Zijlstra wrote:
> > > > > > > On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> > > > > > > > In many cases, page tables can be accessed concurrently by either another
> > > > > > > > CPU (due to things like fast gup) or by the hardware page table walker
> > > > > > > > itself, which may set access/dirty bits. In such cases, it is important
> > > > > > > > to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> > > > > > > > entries cannot be torn, merged or subject to apparent loss of coherence.
> > > > > > > 
> > > > > > > In fact, we should use lockless_dereference() for many of them. Yes
> > > > > > > Alpha is the only one that cares about the difference between that and
> > > > > > > READ_ONCE() and they do have the extra barrier, but if we're going to do
> > > > > > > this, we might as well do it 'right' :-)
> > > > > > 
> > > > > > I know this sounds daft, but I think one of the big reasons why
> > > > > > lockless_dereference() doesn't get an awful lot of use is because it's
> > > > > > such a mouthful! Why don't we just move the smp_read_barrier_depends()
> > > > > > into READ_ONCE? Would anybody actually care about the potential impact on
> > > > > > Alpha (which, frankly, is treading on thin ice given the low adoption of
> > > > > > lockless_dereference())?
> > > > > 
> > > > > This is my cue to ask my usual question...  ;-)
> > > > > 
> > > > > Are people still running mainline kernels on Alpha?  (Added Alpha folks.)
> > > > 
> > > > Yes.  I run two Alpha build daemons that build the unofficial
> > > > debian-alpha port.  Debian popcon reports nine machines running
> > > > Alpha, which are likely to be running the 4.12.y kernel which
> > > > is currently in debian-alpha, (and presumably soon to be 4.13.y
> > > > which is now built on Alpha in experimental).
> > > 
> > > I salute your dedication to Alpha!  ;-)
> > 
> > Ok, but where does that leave us wrt my initial proposal of moving
> > smp_read_barrier_depends() into READ_ONCE and getting rid of
> > lockless_dereference?
> > 
> > Michael (or anybody else running mainline on SMP Alpha) -- would you be
> > able to give the diff below a spin and see whether there's a measurable
> > performance impact?
> 
> This will be a sensitive test.  The smp_read_barrier_depends() can be
> removed from lockless_dereference().  Without this removal Alpha will
> get two memory barriers from rcu_dereference() and friends.

Oh yes, good point. I was trying to keep the diff simple, but you're
right that this is packing too many barriers. Fixed diff below.

Thanks,

Will

--->8

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index e95a2631e545..c4ee9d6d8f2d 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -340,6 +340,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 		__read_once_size(&(x), __u.__c, sizeof(x));		\
 	else								\
 		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
+	smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
 	__u.__val;							\
 })
 #define READ_ONCE(x) __READ_ONCE(x, 1)
@@ -620,7 +621,6 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 ({ \
 	typeof(p) _________p1 = READ_ONCE(p); \
 	typeof(*(p)) *___typecheck_p __maybe_unused; \
-	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
 	(_________p1); \
 })
 

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

#1742239 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Fri, Sep 29, 2017 at 10:08:43AM +0100, Will Deacon wrote:
> On Thu, Sep 28, 2017 at 05:58:30PM -0700, Paul E. McKenney wrote:
> > On Fri, Sep 29, 2017 at 07:59:09AM +1300, Michael Cree wrote:
> > > On Thu, Sep 28, 2017 at 08:43:54AM -0700, Paul E. McKenney wrote:
> > > > On Thu, Sep 28, 2017 at 09:45:35AM +0100, Will Deacon wrote:
> > > > > On Thu, Sep 28, 2017 at 10:38:01AM +0200, Peter Zijlstra wrote:
> > > > > > On Wed, Sep 27, 2017 at 04:49:28PM +0100, Will Deacon wrote:
> > > > > > > In many cases, page tables can be accessed concurrently by either another
> > > > > > > CPU (due to things like fast gup) or by the hardware page table walker
> > > > > > > itself, which may set access/dirty bits. In such cases, it is important
> > > > > > > to use READ_ONCE/WRITE_ONCE when accessing page table entries so that
> > > > > > > entries cannot be torn, merged or subject to apparent loss of coherence.
> > > > > > 
> > > > > > In fact, we should use lockless_dereference() for many of them. Yes
> > > > > > Alpha is the only one that cares about the difference between that and
> > > > > > READ_ONCE() and they do have the extra barrier, but if we're going to do
> > > > > > this, we might as well do it 'right' :-)
> > > > > 
> > > > > I know this sounds daft, but I think one of the big reasons why
> > > > > lockless_dereference() doesn't get an awful lot of use is because it's
> > > > > such a mouthful! Why don't we just move the smp_read_barrier_depends()
> > > > > into READ_ONCE? Would anybody actually care about the potential impact on
> > > > > Alpha (which, frankly, is treading on thin ice given the low adoption of
> > > > > lockless_dereference())?
> > > > 
> > > > This is my cue to ask my usual question...  ;-)
> > > > 
> > > > Are people still running mainline kernels on Alpha?  (Added Alpha folks.)
> > > 
> > > Yes.  I run two Alpha build daemons that build the unofficial
> > > debian-alpha port.  Debian popcon reports nine machines running
> > > Alpha, which are likely to be running the 4.12.y kernel which
> > > is currently in debian-alpha, (and presumably soon to be 4.13.y
> > > which is now built on Alpha in experimental).
> > 
> > I salute your dedication to Alpha!  ;-)
> 
> Ok, but where does that leave us wrt my initial proposal of moving
> smp_read_barrier_depends() into READ_ONCE and getting rid of
> lockless_dereference?
> 
> Michael (or anybody else running mainline on SMP Alpha) -- would you be
> able to give the diff below a spin and see whether there's a measurable
> performance impact?

This will be a sensitive test.  The smp_read_barrier_depends() can be
removed from lockless_dereference().  Without this removal Alpha will
get two memory barriers from rcu_dereference() and friends.

							Thanx, Paul

> Cheers,
> 
> Will
> 
> --->8
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index e95a2631e545..0ce21e25492a 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -340,6 +340,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>  		__read_once_size(&(x), __u.__c, sizeof(x));		\
>  	else								\
>  		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
> +	smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
>  	__u.__val;							\
>  })
>  #define READ_ONCE(x) __READ_ONCE(x, 1)
> 

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

#1741730 — Re: [RFC PATCH 0/2] Missing READ_ONCE in core and arch-specific pgtable code leading to crashes

On 9/27/2017 9:49 AM, Will Deacon wrote:
> Hi,
>
> We recently had a crash report[1] on arm64 that involved a bad dereference
> in the page_vma_mapped code during ext4 writeback with THP active. I can
> reproduce this on -rc2:
>
> [  254.032812] PC is at check_pte+0x20/0x170
> [  254.032948] LR is at page_vma_mapped_walk+0x2e0/0x540
> [...]
> [  254.036114] Process doio (pid: 2463, stack limit = 0xffff00000f2e8000)
> [  254.036361] Call trace:
> [  254.038977] [<ffff000008233328>] check_pte+0x20/0x170
> [  254.039137] [<ffff000008233758>] page_vma_mapped_walk+0x2e0/0x540
> [  254.039332] [<ffff000008234adc>] page_mkclean_one+0xac/0x278
> [  254.039489] [<ffff000008234d98>] rmap_walk_file+0xf0/0x238
> [  254.039642] [<ffff000008236e74>] rmap_walk+0x64/0xa0
> [  254.039784] [<ffff0000082370c8>] page_mkclean+0x90/0xa8
> [  254.040029] [<ffff0000081f3c64>] clear_page_dirty_for_io+0x84/0x2a8
> [  254.040311] [<ffff00000832f984>] mpage_submit_page+0x34/0x98
> [  254.040518] [<ffff00000832fb4c>] mpage_process_page_bufs+0x164/0x170
> [  254.040743] [<ffff00000832fc8c>] mpage_prepare_extent_to_map+0x134/0x2b8
> [  254.040969] [<ffff00000833530c>] ext4_writepages+0x484/0xe30
> [  254.041175] [<ffff0000081f6ab4>] do_writepages+0x44/0xe8
> [  254.041372] [<ffff0000081e5bd4>] __filemap_fdatawrite_range+0xbc/0x110
> [  254.041568] [<ffff0000081e5e68>] file_write_and_wait_range+0x48/0xd8
> [  254.041739] [<ffff000008324310>] ext4_sync_file+0x80/0x4b8
> [  254.041907] [<ffff0000082bd434>] vfs_fsync_range+0x64/0xc0
> [  254.042106] [<ffff0000082332b4>] SyS_msync+0x194/0x1e8
>
> After digging into the issue, I found that we appear to be racing with
> a concurrent pmd update in page_vma_mapped_walk, assumedly due a THP
> splitting operation. Looking at the code there:
>
> 	pvmw->pmd = pmd_offset(pud, pvmw->address);
> 	if (pmd_trans_huge(*pvmw->pmd) || is_pmd_migration_entry(*pvmw->pmd)) {
> 		[...]
> 	} else {
> 		if (!check_pmd(pvmw))
> 			return false;
> 	}
> 	if (!map_pte(pvmw))
> 		goto next_pte;
>
> what happens in the crashing scenario is that we see all zeroes for the
> PMD in pmd_trans_huge(*pvmw->pmd), and so go to the 'else' case (migration
> isn't enabled, so the test is removed at compile-time). check_pmd then does:
>
> 	pmde = READ_ONCE(*pvmw->pmd);
> 	return pmd_present(pmde) && !pmd_trans_huge(pmde);
>
> and reads a valid table entry for the PMD because the splitting has completed
> (i.e. the first dereference reads from the pmdp_invalidate in the splitting
> code, whereas the second dereferenced reads from the following pmd_populate).
> It returns true because we should descend to the PTE level in map_pte. map_pte
> does:
>
> 	pvmw->pte = pte_offset_map(pvmw->pmd, pvmw->address);
>
> which on arm64 (and this appears to be the same on x86) ends up doing:
>
> 	(pmd_page_paddr((*(pvmw->pmd))) + pte_index(pvmw->address) * sizeof(pte_t))
>
> as part of its calculation. However, this is horribly broken because GCC
> inlines everything and reuses the register it loaded for the initial
> pmd_trans_huge check (when we loaded the value of zero) here, so we end up
> calculating a junk pointer and crashing when we dereference it. Disassembly
> at the end of the mail[2] for those who are curious.
>
> The moral of the story is that read-after-read (same address) ordering *only*
> applies if READ_ONCE is used consistently. This means we need to fix page
> table dereferences in the core code as well as the arch code to avoid this
> problem. The two RFC patches in this series fix arm64 (which is a bigger fix
> that necessary since I clean things up too) and page_vma_mapped_walk.
>
> Comments welcome.
Hi Will,
This fix works for me, tested with LTP rwtest 15 iterations on Qualcomm Centiq2400.
Compiler: gcc version 5.2.1 20151005 (Linaro GCC 5.2-2015.11-1)

Tested-by: Richard Ruigrok <rruigrok@codeaurora.org>

Thanks,
Richard
> Will
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-September/532786.html
> [2]
>
> // page_vma_mapped_walk
> // pvmw->pmd = pmd_offset(pud, pvmw->address);
> ldr     x0, [x19, #24]		// pvmw->pmd
>
> // if (pmd_trans_huge(*pvmw->pmd) || is_pmd_migration_entry(*pvmw->pmd)) {
> ldr     x1, [x0]		// *pvmw->pmd
> cbz     x1, ffff0000082336a0 <page_vma_mapped_walk+0x228>
> tbz     w1, #1, ffff000008233788 <page_vma_mapped_walk+0x310>	// pmd_trans_huge?
>
> // else if (!check_pmd(pvmw))
> ldr     x0, [x0]		// READ_ONCE in check_pmd
> tst     x0, x24			// pmd_present?
> b.eq    ffff000008233538 <page_vma_mapped_walk+0xc0>  // b.none
> tbz     w0, #1, ffff000008233538 <page_vma_mapped_walk+0xc0>	// pmd_trans_huge?
>
> // if (!map_pte(pvmw))
> ldr     x0, [x19, #16]		// pvmw->address
>
> // pvmw->pte = pte_offset_map(pvmw->pmd, pvmw->address);
> and     x1, x1, #0xfffffffff000	// Reusing the old value of *pvmw->pmd!!!
> [...]
>
> --->8
>
> Will Deacon (2):
>   arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables
>   mm: page_vma_mapped: Ensure pmd is loaded with READ_ONCE outside of
>     lock
>
>  arch/arm64/include/asm/hugetlb.h     |   2 +-
>  arch/arm64/include/asm/kvm_mmu.h     |  18 +--
>  arch/arm64/include/asm/mmu_context.h |   4 +-
>  arch/arm64/include/asm/pgalloc.h     |  42 +++---
>  arch/arm64/include/asm/pgtable.h     |  29 ++--
>  arch/arm64/kernel/hibernate.c        | 148 +++++++++---------
>  arch/arm64/mm/dump.c                 |  54 ++++---
>  arch/arm64/mm/fault.c                |  44 +++---
>  arch/arm64/mm/hugetlbpage.c          |  94 ++++++------
>  arch/arm64/mm/kasan_init.c           |  62 ++++----
>  arch/arm64/mm/mmu.c                  | 281 ++++++++++++++++++-----------------
>  arch/arm64/mm/pageattr.c             |  30 ++--
>  mm/page_vma_mapped.c                 |  25 ++--
>  13 files changed, 427 insertions(+), 406 deletions(-)
>

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

#1741784 — Re: [RFC PATCH 1/2] arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables

On Wed, Sep 27, 2017 at 10:49 AM, Will Deacon <will.deacon@arm.com> wrote:
> This patch consistently uses these macros in the arch
> code, as well as explicitly namespacing pointers to page table entries
> from the entries themselves by using adopting a 'p' suffix for the former
> (as is sometimes used elsewhere in the kernel source).

Would you consider splitting up this patch into two, where the second
patch makes all the cosmetic changes?  That would make the "meatier"
patch easier to back-port and review.

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

#1741795 — Re: [RFC PATCH 0/2] Missing READ_ONCE in core and arch-specific pgtable code leading to crashes

On 09/27/2017 11:49 AM, Will Deacon wrote:

> The moral of the story is that read-after-read (same address) ordering *only*
> applies if READ_ONCE is used consistently. This means we need to fix page
> table dereferences in the core code as well as the arch code to avoid this
> problem. The two RFC patches in this series fix arm64 (which is a bigger fix
> that necessary since I clean things up too) and page_vma_mapped_walk.
> 
> Comments welcome.

Thanks for this Will. I'll echo Timur's comment that it would be ideal
to split this up into the critical piece needed for ordering
access/update to the PMD in the face of a THP split and separately have
the cosmetic cleanups. Needless to say, we've got a bunch of people who
are tracking this one and tracking it ready for backport. We just got
THP re-enabled so I'm pretty keen that we not have to disable again.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

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

#1741997 — Re: [RFC PATCH 0/2] Missing READ_ONCE in core and arch-specific pgtable code leading to crashes

[+ Timur]

On Thu, Sep 28, 2017 at 03:38:00PM -0400, Jon Masters wrote:
> On 09/27/2017 11:49 AM, Will Deacon wrote:
> 
> > The moral of the story is that read-after-read (same address) ordering *only*
> > applies if READ_ONCE is used consistently. This means we need to fix page
> > table dereferences in the core code as well as the arch code to avoid this
> > problem. The two RFC patches in this series fix arm64 (which is a bigger fix
> > that necessary since I clean things up too) and page_vma_mapped_walk.
> > 
> > Comments welcome.
> 
> Thanks for this Will. I'll echo Timur's comment that it would be ideal
> to split this up into the critical piece needed for ordering
> access/update to the PMD in the face of a THP split and separately have
> the cosmetic cleanups. Needless to say, we've got a bunch of people who
> are tracking this one and tracking it ready for backport. We just got
> THP re-enabled so I'm pretty keen that we not have to disable again.

Yeah, of course. I already posted a point diff to Yury in the original
thread:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-September/533299.html

so I'd like to queue that as an arm64 fix after we've worked out the general
direction of the full fix. I also don't see why other architectures
(including x86) can't be hit by this, so an alternative (completely
untested) approach would just be to take patch 2 of this series.

The full fix isn't just cosmetic; it's also addressing the wider problem
of unannotated racing page table accesses outside of the specific failure
case we've run into.

Will

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web