netdev - RE: [PATCH v2] rcu: fix a race in hlist_nulls_for_each_entry

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]

Date:	Wed, 22 May 2013 14:27:12 +0100
From:	"David Laight" <David.Laight@...LAB.COM>
To:	"Eric Dumazet" <eric.dumazet@...il.com>,
	"Roman Gushchin" <klamm@...dex-team.ru>
Cc:	<paulmck@...ux.vnet.ibm.com>,
	"Dipankar Sarma" <dipankar@...ibm.com>, <zhmurov@...dex-team.ru>,
	<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	"Alexey Kuznetsov" <kuznet@....inr.ac.ru>,
	"James Morris" <jmorris@...ei.org>,
	"Hideaki YOSHIFUJI" <yoshfuji@...ux-ipv6.org>,
	"Patrick McHardy" <kaber@...sh.net>
Subject: RE: [PATCH v2] rcu: fix a race in hlist_nulls_for_each_entry_rcu macro

> So yes, the patch appears to fix the bug, but it sounds not logical to
> me.

I was confused because the copy of the code I found was different
(it has some checks for reusaddr - which force a function call in the
loop).

The code being compiled is:

begin:
	result = NULL;
	badness = -1;
	udp_portaddr_for_each_entry_rcu(sk, node, &hslot2->head) {
		score = compute_score2(sk, net, saddr, sport,
				      daddr, hnum, dif);
		if (score > badness) {
			result = sk;
			badness = score;
			if (score == SCORE2_MAX)
				goto exact_match;
		}
	}
	/*
	 * if the nulls value we got at the end of this lookup is
	 * not the expected one, we must restart lookup.
	 * We probably met an item that was moved to another chain.
	 */
	if (get_nulls_value(node) != slot2)
		goto begin;

Which is entirely inlined - so the compiler is allowed to assume
that no other code modifies any of the data.
Hence it is allowed to cache the list head value.
Indeed it could convert the last line to "for (;;);".

A asm volatile ("":::"memory") somewhere would fix it.

	David