On Thu, 17 May 2012 16:13:56 +0200
Andrea Arcangeli [off-list ref] wrote:

When holding the mmap_sem for reading, pmd_offset_map_lock should only
run on a pmd_t that has been read atomically from the pmdp
pointer, otherwise we may read only half of it leading to this crash.

Do you think this is serious enough to warrant backporting the fix into
-stable?  The patch is pretty simple..

A couple of nits:

- read_pmd_atomic() should be called pmd_read_atomic() - check out
  "grep pmd include/asm-generic/pgtable.h".

- A somewhat neat convention we use is to do

	static inline void foo(...)
	{
		...
	}
	#define foo foo

  so then other code can do

	#ifndef foo
	...
	#endif

  This avoids having to create (and remember!) a second identifier.

+/*
+ * pte_offset_map_lock on 32bit PAE kernels was reading the pmd_t with
+ * a "*pmdp" dereference done by gcc.

I spent some time trying to find exactly where pte_offset_map_lock does
this dereference then gave up, because it shouldn't have been this
hard!

Can we be specific here, so that others can more easily work out what's
going on?

Problem is, in certain places
+ * where pte_offset_map_lock is called, concurrent page faults are
+ * allowed, if the mmap_sem is hold for reading. An example is mincore
+ * vs page faults vs MADV_DONTNEED. On the page fault side
+ * pmd_populate rightfully does a set_64bit, but if we're reading the
+ * pmd_t with a "*pmdp" on the mincore side, a SMP race can happen
+ * because gcc will not read the 64bit of the pmd atomically. To fix
+ * this all places running pmd_offset_map_lock() while holding the
+ * mmap_sem in read mode, shall read the pmdp pointer using this
+ * function to know if the pmd is null nor not, and in turn to know if
+ * they can run pmd_offset_map_lock or pmd_trans_huge or other pmd
+ * operations.
+ *
+ * Without THP if the mmap_sem is hold for reading, the
+ * pmd can only transition from null to not null while read_pmd_atomic runs.
+ * So there's no need of literally reading it atomically.
+ *
+ * With THP if the mmap_sem is hold for reading, the pmd can become
+ * THP or null or point to a pte (and in turn become "stable") at any
+ * time under read_pmd_atomic, so it's mandatory to read it atomically
+ * with cmpxchg8b.

This all seems terribly subtle and fragile.

+ */
+#ifndef CONFIG_TRANSPARENT_HUGEPAGE
+static inline pmd_t read_pmd_atomic(pmd_t *pmdp)
+{
+	pmdval_t ret;
+	u32 *tmp = (u32 *)pmdp;
+
+	ret = (pmdval_t) (*tmp);
+	if (ret) {
+		/*
+		 * If the low part is null, we must not read the high part
+		 * or we can end up with a partial pmd.

This is the core part of the fix, and I don't understand it :( What is
the significance of the zeroness of the lower half of the pmdval_t? 
How, exactly, does this prevent races?

At a guess, I'd say that we're making three assumptions here:

a) that gcc will write lower-word-first when doing a 64-bit write

b) that a valid pmdval_t never has all zeroes in the lower 32 bits and

c) that any code which this function is racing against will only
   ever be writing to a pmdval_t which has the all-zeroes pattern.  ie:
   that we never can race against code which is modifying an existing
   pmdval_t.

Can we spell out and justify all the assumptions here?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>