Revision e2093926a098a8ccf0f1d10f6df8dad452cb28d3 authored by Ross Zwisler on 02 June 2017, 21:46:37 UTC, committed by Linus Torvalds on 02 June 2017, 22:07:37 UTC
We currently have two related PMD vs PTE races in the DAX code. These can both be easily triggered by having two threads reading and writing simultaneously to the same private mapping, with the key being that private mapping reads can be handled with PMDs but private mapping writes are always handled with PTEs so that we can COW. Here is the first race: CPU 0 CPU 1 (private mapping write) __handle_mm_fault() create_huge_pmd() - FALLBACK handle_pte_fault() passes check for pmd_devmap() (private mapping read) __handle_mm_fault() create_huge_pmd() dax_iomap_pmd_fault() inserts PMD dax_iomap_pte_fault() does a PTE fault, but we already have a DAX PMD installed in our page tables at this spot. Here's the second race: CPU 0 CPU 1 (private mapping read) __handle_mm_fault() passes check for pmd_none() create_huge_pmd() dax_iomap_pmd_fault() inserts PMD (private mapping write) __handle_mm_fault() create_huge_pmd() - FALLBACK (private mapping read) __handle_mm_fault() passes check for pmd_none() create_huge_pmd() handle_pte_fault() dax_iomap_pte_fault() inserts PTE dax_iomap_pmd_fault() inserts PMD, but we already have a PTE at this spot. The core of the issue is that while there is isolation between faults to the same range in the DAX fault handlers via our DAX entry locking, there is no isolation between faults in the code in mm/memory.c. This means for instance that this code in __handle_mm_fault() can run: if (pmd_none(*vmf.pmd) && transparent_hugepage_enabled(vma)) { ret = create_huge_pmd(&vmf); But by the time we actually get to run the fault handler called by create_huge_pmd(), the PMD is no longer pmd_none() because a racing PTE fault has installed a normal PMD here as a parent. This is the cause of the 2nd race. The first race is similar - there is the following check in handle_pte_fault(): } else { /* See comment in pte_alloc_one_map() */ if (pmd_devmap(*vmf->pmd) || pmd_trans_unstable(vmf->pmd)) return 0; So if a pmd_devmap() PMD (a DAX PMD) has been installed at vmf->pmd, we will bail and retry the fault. This is correct, but there is nothing preventing the PMD from being installed after this check but before we actually get to the DAX PTE fault handlers. In my testing these races result in the following types of errors: BUG: Bad rss-counter state mm:ffff8800a817d280 idx:1 val:1 BUG: non-zero nr_ptes on freeing mm: 15 Fix this issue by having the DAX fault handlers verify that it is safe to continue their fault after they have taken an entry lock to block other racing faults. [ross.zwisler@linux.intel.com: improve fix for colliding PMD & PTE entries] Link: http://lkml.kernel.org/r/20170526195932.32178-1-ross.zwisler@linux.intel.com Link: http://lkml.kernel.org/r/20170522215749.23516-2-ross.zwisler@linux.intel.com Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> Reported-by: Pawel Lebioda <pawel.lebioda@intel.com> Reviewed-by: Jan Kara <jack@suse.cz> Cc: "Darrick J. Wong" <darrick.wong@oracle.com> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christoph Hellwig <hch@lst.de> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Dave Hansen <dave.hansen@intel.com> Cc: Matthew Wilcox <mawilcox@microsoft.com> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com> Cc: Pawel Lebioda <pawel.lebioda@intel.com> Cc: Dave Jiang <dave.jiang@intel.com> Cc: Xiong Zhou <xzhou@redhat.com> Cc: Eryu Guan <eguan@redhat.com> Cc: <stable@vger.kernel.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
1 parent d0f0931
lp.h
/*
* usr/include/linux/lp.h c.1991-1992 James Wiegand
* many modifications copyright (C) 1992 Michael K. Johnson
* Interrupt support added 1993 Nigel Gamble
* Removed 8255 status defines from inside __KERNEL__ Marcelo Tosatti
*/
#ifndef _LINUX_LP_H
#define _LINUX_LP_H
#include <linux/wait.h>
#include <linux/mutex.h>
#include <uapi/linux/lp.h>
/* Magic numbers for defining port-device mappings */
#define LP_PARPORT_UNSPEC -4
#define LP_PARPORT_AUTO -3
#define LP_PARPORT_OFF -2
#define LP_PARPORT_NONE -1
#define LP_F(minor) lp_table[(minor)].flags /* flags for busy, etc. */
#define LP_CHAR(minor) lp_table[(minor)].chars /* busy timeout */
#define LP_TIME(minor) lp_table[(minor)].time /* wait time */
#define LP_WAIT(minor) lp_table[(minor)].wait /* strobe wait */
#define LP_IRQ(minor) lp_table[(minor)].dev->port->irq /* interrupt # */
/* PARPORT_IRQ_NONE means polled */
#ifdef LP_STATS
#define LP_STAT(minor) lp_table[(minor)].stats /* statistics area */
#endif
#define LP_BUFFER_SIZE PAGE_SIZE
#define LP_BASE(x) lp_table[(x)].dev->port->base
#ifdef LP_STATS
struct lp_stats {
unsigned long chars;
unsigned long sleeps;
unsigned int maxrun;
unsigned int maxwait;
unsigned int meanwait;
unsigned int mdev;
};
#endif
struct lp_struct {
struct pardevice *dev;
unsigned long flags;
unsigned int chars;
unsigned int time;
unsigned int wait;
char *lp_buffer;
#ifdef LP_STATS
unsigned int lastcall;
unsigned int runchars;
struct lp_stats stats;
#endif
wait_queue_head_t waitq;
unsigned int last_error;
struct mutex port_mutex;
wait_queue_head_t dataq;
long timeout;
unsigned int best_mode;
unsigned int current_mode;
unsigned long bits;
};
/*
* The following constants describe the various signals of the printer port
* hardware. Note that the hardware inverts some signals and that some
* signals are active low. An example is LP_STROBE, which must be programmed
* with 1 for being active and 0 for being inactive, because the strobe signal
* gets inverted, but it is also active low.
*/
/*
* defines for 8255 control port
* base + 2
* accessed with LP_C(minor)
*/
#define LP_PINTEN 0x10 /* high to read data in or-ed with data out */
#define LP_PSELECP 0x08 /* inverted output, active low */
#define LP_PINITP 0x04 /* unchanged output, active low */
#define LP_PAUTOLF 0x02 /* inverted output, active low */
#define LP_PSTROBE 0x01 /* short high output on raising edge */
/*
* the value written to ports to test existence. PC-style ports will
* return the value written. AT-style ports will return 0. so why not
* make them the same ?
*/
#define LP_DUMMY 0x00
/*
* This is the port delay time, in microseconds.
* It is used only in the lp_init() and lp_reset() routine.
*/
#define LP_DELAY 50
#endif
![swh spinner](/static/img/swh-spinner.gif)
Computing file changes ...