linux-kernel - Re: [PATCH] cpuset: mm: Reduce large amounts of memory barrier related damage v3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130823181546.GA31370@twins.programming.kicks-ass.net>
Date:	Fri, 23 Aug 2013 20:15:46 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Mel Gorman <mgorman@...e.de>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Miao Xie <miaox@...fujitsu.com>,
	David Rientjes <rientjes@...gle.com>,
	Christoph Lameter <cl@...ux.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, riel@...hat.com
Subject: Re: [PATCH] cpuset: mm: Reduce large amounts of memory barrier
 related damage v3

On Fri, Aug 23, 2013 at 03:03:32PM +0200, Peter Zijlstra wrote:
> So I think this patch is broken (still). Suppose we have an
> INTERLEAVE mempol like 0x3 and change it to 0xc.
> 
> Original:	0x3
> Rebind Step 1:	0xf /* set bits */
> Rebind Step 2:	0xc /* clear bits */
> 
> Now look at what can happen with offset_il_node() when its ran
> concurrently with step 2:
> 
>   nnodes = nodes_weight(pol->v.nodes); /* observes 0xf and returns 4 */
> 
>   /* now we clear the actual bits */
>   
>   target = (unsigned int)off % nnodes; /* assume target >= 2 */
>   c = 0;
>   do {
>   	nid = next_node(nid, pol->v.nodes);
> 	c++;
>   } while (c <= target);
> 
>   /* here nid := MAX_NUMNODES */
> 
> 
> This nid is then blindly inserted into node_zonelist() which does an
> NODE_DATA() array access out of bounds and off we go.
> 
> This would suggest we put the whole seqcount thing inside
> offset_il_node().

Oh bloody grrr. Its not directly related at all, the patch in question
fixes a cpuset task_struct::mems_allowed problem while the above is a
mempolicy issue and of course the cpuset and mempolicy code are
completely bloody different :/

So I guess the quick and ugly solution is something like the below. 

--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1762,19 +1762,21 @@ unsigned slab_node(void)
 static unsigned offset_il_node(struct mempolicy *pol,
 		struct vm_area_struct *vma, unsigned long off)
 {
-	unsigned nnodes = nodes_weight(pol->v.nodes);
-	unsigned target;
-	int c;
-	int nid = -1;
+	unsigned nnodes, target;
+	int c, nid;
 
+again:
+	nnodes = nodes_weight(pol->v.nodes);
 	if (!nnodes)
 		return numa_node_id();
+
 	target = (unsigned int)off % nnodes;
-	c = 0;
-	do {
+	for (c = 0, nid = -1; c <= target; c++)
 		nid = next_node(nid, pol->v.nodes);
-		c++;
-	} while (c <= target);
+
+	if (unlikely((unsigned)nid >= MAX_NUMNODES))
+		goto again;
+
 	return nid;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/